Bug 27751

Summary: Add a pile of API to WockyNodeTree
Product: Wocky Reporter: Will Thompson <will>
Component: GeneralAssignee: Sjoerd Simons <sjoerd>
Status: RESOLVED FIXED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: normal    
Priority: medium Keywords: patch
Version: unspecified   
Hardware: Other   
OS: All   
URL: http://git.collabora.co.uk/?p=user/sjoerd/wocky.git;a=shortlog;h=refs/heads/wocky-node-tree-updated-api
Whiteboard:
i915 platform: i915 features:

Comment 1 Will Thompson 2010-04-20 06:49:55 UTC
this is a branch which i will review
Comment 2 Will Thompson 2010-04-20 07:09:26 UTC
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?
Comment 3 Sjoerd Simons 2010-04-20 07:23:22 UTC
(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.
Comment 4 Will Thompson 2010-04-20 08:47:26 UTC
(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 ++
Comment 5 Sjoerd Simons 2010-04-20 09:15:19 UTC
merged

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.