Code review comment for lp:~3v1n0/bamf/libreoffice-fixes

Firstly - I LOVE you for getting rid of all the 'key = g_new (gint, 1);' that just gross in so many ways :-) Now for some comments:

8 - icon = g_icon_to_string (gicon);
9 +
10 + if (gicon)
11 + icon = g_icon_to_string (gicon);

The function you're updating here, bamf_application_setup_icon_and_name(), seems to leak self->priv->icon right at the end if it's set before we assign it. Can you plug that while you're here?

30 void
31 +bamf_legacy_screen_inject_window (BamfLegacyScreen *self, guint xid)

It's not totally clear to me what this function is intended to do. Can you add a short docstring? (I assume most ppl wont bother to dig out of the revision history like me ;-))

94 +void
95 +bamf_legacy_window_reopen (BamfLegacyWindow *self)

Same as the last new function :-)

101 + g_object_weak_ref (G_OBJECT (self), (GWeakNotify) handle_destroy_notify,
102 + GUINT_TO_POINTER (xid));

It's not very clear what the magic behind this weak ref is, could you add an explanatory comment?

373 + g_list_free_full (possible_apps, g_free);

The introduction of g_list_free_full() is nice, but requires glib >=2.28. Please add that check to configure.in.

423 + if (!g_hash_table_lookup (registered_pids, key))
424 + {
425 + g_hash_table_insert (registered_pids, key, g_strdup (window_hint));
426 + }

Only updating if we don't already know the pid seems like it could cause things to go askew? If that's not the case I think this logic requires a comment in the code.

628 static void
629 +bamf_matcher_dispose (GObject *object)

This function does not look like it's safe to run N times on the same object. That is the contract of dispose(). Fx. looking at this block makes me suspicious:

642 + g_array_free (priv->bad_prefixes, TRUE);
643 + g_array_free (priv->known_pids, TRUE);
644 + g_hash_table_destroy (priv->desktop_id_table);
645 + g_hash_table_destroy (priv->desktop_file_table);
646 + g_hash_table_destroy (priv->desktop_class_table);
647 + g_hash_table_destroy (priv->registered_pids);

You must add NULL-ification and NULL-checking on all members. If you don't want that override finalize() instead which is guaranteed to run only once.

695 + GObject *old_object = dbus_g_connection_lookup_g_object (bus, path);
696 + if (G_IS_OBJECT (old_object))
697 + {
698 + dbus_g_connection_unregister_g_object (bus, old_object);
699 + }

The commit message for this chunk mentions that you log a g_critical()when you replace an object. I don't see that critical being logged - is that intentional?