Code review comment for lp:~ted/indicator-appmenu/hud

> * in dbusmenu_collector_dispose(), priv->bus needs to be unreffed and
> set to NULL (/after/ priv->signal is cleared)

Fixed r189

> * remove_underline() needs to be static.

Fixed r190

> * as remove_underline()'s TODO comment requested, here's a bulk copy
> implementation

Cool, thanks! r191

> * in just_do_it(), results should be declared on the stack and
> initialized to NULL, not passed in as an argument

Ah, refactoring debt. It used to be just_do_it()'s got chained
together. Also cleaned up indicator_name which is unused for the same
reason. Fixed r192

> * indicator_name appears to always be NULL everywhere and should be
> removed unless you have future plans for it

Oh, should have read this before that last commit :-)

> * in dbusmenu_collector_search(), should add g_return_val_if_fail
> (IS_DBUSMENU_COLLECTOR(collector), NULL);

Fixed. r193

> * in menuitem_to_tokens(), pull the PROP_TYPE string once into a
> local variable, instead of pulling it **9 times** :)

Heh, cut-and-paste got out of hand :-) r194

> * in process_client(), no need to walk the menuitem's hashtable twice
> for DBUSMENU_MENUITEM_PROP_LABEL -- remove the _property_exist() call
> and add an "if (label == NULL) continue;"

Actually the label will never be NULL. If there isn't a value the
default value will be given. Which in this case will be "Label Empty".
This means that most dbusmenu code needs to not worry about whether a
core property is set or not, they'll just get a value that they can use.
And defaults are never sent over the bus.

> * in tokens_to_children(), no need to walk the menuitem's hashtable
> twice for DBUSMENU_MENUITEM_PROP_TYPE -- pull the string to a stack
> variable, and stack for !type || !g_strcmp0(type,
> DBUSMENU_CLIENT_TYPES_DEFAULT)

Same basic reason. But, we could probably handle that here... not sure
that I don't want to use it just because it keeps the pattern the same
everywhere...

> * in just_do_it(), having each search failure walk the hashtable for
> g_debug() smells like overkill?

Yeah, but it really should never happen. So if it does, we need some
sort of debugging information. Really, *never*.

> * trivial comment typo in update_layout_cb: s/becasue/because/

Fixed r195

> * in dbusmenu_collector_found_new(), g_strdup() handles NULL
> correctly, so there's no need for the "if (foo != NULL)" in "if (foo !
> = NULL) bar = g_strdup (foo);"

Okay, I think we deleted that code with the indicator_name change above
anyway.

> * in tokens_to_children(), a comment explaining why we skip
> disabled/invisible rootitems would be nice-to-have
>
> * no glib comment annotation anywhere (example: transfer &
> element-type in dbusmenu_collector_search())

Fixed r196