GNOME Bugzilla – Bug 634781
StFocusManager needs to ref the groups added
Last modified: 2010-11-14 20:28:21 UTC
The internal GHashTable has g_object_unref as the destroy notify, so of course we need to ref the objects we add. This seems to fix the obnoxious bug from http://mail.gnome.org/archives/gnome-shell-list/2010-October/msg00070.html. (At least, I hope so. Tracking it has be an huge pain, as it could be anywhere in gobject, gjs, clutter or gnome-shell itself, and I don't want to do it again).
Created attachment 174408 [details] [review] StFocusManager: ref the added groups Take a ref when adding, so it can be dropped when removing.
Review of attachment 174408 [details] [review]: Good catch! The fix looks backwards to me ... the "destroy" signal on a ClutterActor is guaranteed to be run before it is freed (it's emitted from clutter_actor_dispose(), and any GObject is always reliably disposed at least once before finalization) so just using g_hash_table_new() without a DestroyNotify seems better to me. We don't want the focus manager to keep around an actor that would otherwise be freed.
Created attachment 174455 [details] [review] StFocusManager: don't unref removed groups It is not referencing them when adding, and also it is connecting to the "destroy" signal, emitted on dispose, so there is no risk of storing finalized objects.
Review of attachment 174455 [details] [review]: + manager->priv->groups = g_hash_table_new_full (NULL, NULL, NULL, NULL); Is the same as + manager->priv->groups = g_hash_table_new (NULL, NULL); which is a little more readable. OK to commit with that change.