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

Review of dbusmenu-collector.

MUST

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

 * remove_underline() needs to be static.

SHOULD

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

        static gchar *
        remove_underline (const gchar * input)
        {
                const gunichar underline = g_utf8_get_char("_");
                GString * output = g_string_sized_new (strlen(input)+1);

                const gchar * begin = input;
                const gchar * end;
                while ((end = g_utf8_strchr (begin, -1, underline))) {
                        g_string_append_len (output, begin, end-begin);
                        begin = g_utf8_next_char (end);
                }
                g_string_append (output, begin);
                return g_string_free (output, FALSE);
        }

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

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

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

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

 * 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;"

 * 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)

MAY

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

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

 * 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);"

 * 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())