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

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