Merge lp://qastaging/~3v1n0/bamf/libreoffice-fixes into lp://qastaging/bamf/0.4

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?

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

Yes, I'm sure that there are also some other leaks around, but I've not read everything yet. :)

Now for some comments:

> 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?

Done. Free'd also on dispose.

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

Done.

> 94 +void
> 95 +bamf_legacy_window_reopen (BamfLegacyWindow *self)
>
> Same as the last new function :-)

Done.

> 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?

Ok.

> 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.

Done.

> 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.

Well, this code was already there and I just kept the old behavior alive. However it seems to work well, also because generally a pid is always connected to just one application, except particular cases like this one (libreoffice or chromium).
However, I'll study more on this and eventually I'll change this 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.

Right, moving to finalize().

> 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?

Ehehe... Really it was there, but I don't know why I've not committed it.
Re-added! ;)

Ok, looking good now. Awesome work! Just one line to fix before I can approve this:

773 + object_class->dispose = bamf_matcher_finalize;

You mean

 + object_class->finalize = bamf_matcher_finalize;

:-)

I am happy! :-D