Bug 27751

this is a branch which i will review

I wonder whether WockyNodeTree:top-node stealing the thing you give it (rather than copying it) will confuse bindings?

+          DEBUG ("Stanza without a namespace !?, using dummy namespace");

When will this happen?

+ * <example><programlisting>
+ * wocky_node_tree_new ("html", "http://www.w3.org/1999/xhtml",
+ *    "alice@<!-- -->collabora.co.uk", "bob@<!-- -->collabora.co.uk",
+ *    '(', "html", ':', "http://www.w3.org/1999/xhtml",
+ *      '(', "body", '@', "textcolor", "red",
+ *         '$', "Wocky wooo",
+ *      ')'
+ *   NULL);
+ * </programlisting></example>

Following in the glorious tradition of getting these examples wrong, you have not closed the html tag.

+ * This function may only be called in non-streaming mode.
...
+  g_return_if_fail (!writer->priv->stream_mode);

I think the non-streaming mode should be a subclass. Having methods that may or may not blow up is kind of weird.

+  destination = wocky_node_tree_new ("mainlain europe",

mainland. can you really have spaces in tag names?

(In reply to comment #2)
> I wonder whether WockyNodeTree:top-node stealing the thing you give it (rather
> than copying it) will confuse bindings?
> 
> +          DEBUG ("Stanza without a namespace !?, using dummy namespace");
> 
> When will this happen?

In non-streaming mode when the top node doesn't have a namespace
 
> + * <example><programlisting>
> + * wocky_node_tree_new ("html", "http://www.w3.org/1999/xhtml",
> + *    "alice@<!-- -->collabora.co.uk", "bob@<!-- -->collabora.co.uk",
> + *    '(', "html", ':', "http://www.w3.org/1999/xhtml",
> + *      '(', "body", '@', "textcolor", "red",
> + *         '$', "Wocky wooo",
> + *      ')'
> + *   NULL);
> + * </programlisting></example>
> 
> Following in the glorious tradition of getting these examples wrong, you have
> not closed the html tag.

fixed!

> + * This function may only be called in non-streaming mode.
> ...
> +  g_return_if_fail (!writer->priv->stream_mode);
> 
> I think the non-streaming mode should be a subclass. Having methods that may or
> may not blow up is kind of weird.

Yes, but it would mean loads of boilerplate for not much win.. In all but very special cases non-streaming mode and serializing node-trees is kind of weird, so while i agree with you i'd prefer keeping it this way.

> +  destination = wocky_node_tree_new ("mainlain europe",
> 
> mainland. can you really have spaces in tag names?

You can't.. fixed now. In the glorious future wocky should check this and yell at you if needed.

(In reply to comment #3)
> (In reply to comment #2)
> > I wonder whether WockyNodeTree:top-node stealing the thing you give it (rather
> > than copying it) will confuse bindings?
> > 
> > +          DEBUG ("Stanza without a namespace !?, using dummy namespace");
> > 
> > When will this happen?
> 
> In non-streaming mode when the top node doesn't have a namespace

Could you document that in the code? The "!?" makes it look more worrying than it actually is, probably.

> > + * This function may only be called in non-streaming mode.
> > ...
> > +  g_return_if_fail (!writer->priv->stream_mode);
> > 
> > I think the non-streaming mode should be a subclass. Having methods that may or
> > may not blow up is kind of weird.
> 
> Yes, but it would mean loads of boilerplate for not much win.. In all but very
> special cases non-streaming mode and serializing node-trees is kind of weird,
> so while i agree with you i'd prefer keeping it this way.

I think making it a subclass would make the base writer's API less weird. But okay, if you insist.

Otherwise ++

merged