Merge lp:~ted/indicator-appmenu/lp743404 into lp:indicator-appmenu/0.3

[Ted Gould]

This is bug about a signal, a signal that didn't have an object. And object that had abondoned it, and didn't bother to tell the signal. This is a sad story.

But it has a happy ending.

We've now protected that signal against objects that abondon it with further checks. We've now set up the situation so that the signals get removed when they're no longer needed. And we've delt with the reprocussions of having two places to remove the signal can cause. So in a nutshell:

We've earned the right to party.

[Jay Taoko (jaytaoko)]

review: Approve

[Conor Curran (cjcurran)]

Looks good to me Ted

review: Approve

=== modified file 'src/window-menus.c'
--- src/window-menus.c	2011-03-08 17:10:26 +0000
+++ src/window-menus.c	2011-04-03 04:12:30 +0000
@@ -226,8 +226,8 @@
 	}
 
 	if (priv->root != NULL) {
-		g_object_unref(G_OBJECT(priv->root));
-		priv->root = NULL;
+		root_changed(DBUSMENU_CLIENT(priv->client), NULL, object);
+		g_warn_if_fail(priv->root == NULL);
 	}
 
 	if (priv->client != NULL) {
@@ -520,6 +520,18 @@
 	return;
 }
 
+/* Remove the various signals that we attach to menuitems to
+   ensure they don't pop up later. */
+static void
+remove_menuitem_signals (DbusmenuMenuitem * mi, gpointer user_data)
+{
+	g_signal_handlers_disconnect_by_func(G_OBJECT(mi), G_CALLBACK(menu_entry_realized), user_data);
+	g_signal_handlers_disconnect_matched (mi, G_SIGNAL_MATCH_FUNC, 0, 0, 0, menu_child_realized, NULL);
+	g_signal_handlers_disconnect_matched (mi, G_SIGNAL_MATCH_FUNC, 0, 0, 0, menu_prop_changed, NULL);
+
+	return;
+}
+
 /* Respond to the root menu item on our client changing */
 static void
 root_changed (DbusmenuClient * client, DbusmenuMenuitem * new_root, gpointer user_data)
@@ -531,6 +543,8 @@
 	free_entries(G_OBJECT(user_data), TRUE);
 
 	if (priv->root != NULL) {
+		dbusmenu_menuitem_foreach(priv->root, remove_menuitem_signals, client);
+
 		g_signal_handlers_disconnect_by_func(G_OBJECT(priv->root), G_CALLBACK(menu_entry_added), user_data);
 		g_signal_handlers_disconnect_by_func(G_OBJECT(priv->root), G_CALLBACK(menu_entry_removed), user_data);
 		g_object_unref(priv->root);
@@ -564,6 +578,7 @@
 static void
 menu_entry_added (DbusmenuMenuitem * root, DbusmenuMenuitem * newentry, guint position, gpointer user_data)
 {
+	g_return_if_fail(IS_WINDOW_MENUS(user_data));
 	WindowMenusPrivate * priv = WINDOW_MENUS_GET_PRIVATE(user_data);
 
 	g_signal_connect(G_OBJECT(newentry), DBUSMENU_MENUITEM_SIGNAL_REALIZED, G_CALLBACK(menu_entry_realized), user_data);
@@ -575,11 +590,23 @@
 	return;
 }
 
+/* A small clean up function to ensure that the data
+   gets free'd and the ref lost in all cases. */
+static void
+child_realized_data_cleanup (gpointer user_data, GClosure * closure)
+{
+	gpointer * data = (gpointer *)user_data;
+	g_object_unref(data[1]);
+	g_free(user_data);
+	return;
+}
+
 /* React to the menuitem when we know that it's got all the data
    that we really need. */
 static void
 menu_entry_realized (DbusmenuMenuitem * newentry, gpointer user_data)
 {
+	g_return_if_fail(IS_WINDOW_MENUS(user_data));
 	WindowMenusPrivate * priv = WINDOW_MENUS_GET_PRIVATE(user_data);
 	GtkMenu * menu = dbusmenu_gtkclient_menuitem_get_submenu(priv->client, newentry);
 
@@ -590,7 +617,7 @@
 			data[0] = user_data;
 			data[1] = g_object_ref(newentry);
 
-			g_signal_connect(G_OBJECT(children->data), DBUSMENU_MENUITEM_SIGNAL_REALIZED, G_CALLBACK(menu_child_realized), data);
+			g_signal_connect_data(G_OBJECT(children->data), DBUSMENU_MENUITEM_SIGNAL_REALIZED, G_CALLBACK(menu_child_realized), data, child_realized_data_cleanup, 0);
 		} else {
 			g_warning("Entry has no children!");
 		}
@@ -641,14 +668,25 @@
 static void
 menu_child_realized (DbusmenuMenuitem * child, gpointer user_data)
 {
-	DbusmenuMenuitem * newentry = (DbusmenuMenuitem *)(((gpointer *)user_data)[1]);
+	/* Grab our values out to stack variables */
+	DbusmenuMenuitem * newentry = DBUSMENU_MENUITEM(((gpointer *)user_data)[1]);
+	WindowMenus * wm = WINDOW_MENUS(((gpointer *)user_data)[0]);
+
+	g_return_if_fail(newentry != NULL);
+	g_return_if_fail(wm != NULL);
+
+	/* Disconnection below will drop the ref for this signal
+	   handler, let's make sure that's not a problem */
+	g_object_ref(G_OBJECT(newentry));
 
 	/* Only care about the first */
+	/* This will cause the cleanup function attached to the signal
+	   handler to be run. */
 	g_signal_handlers_disconnect_by_func(G_OBJECT(child), menu_child_realized, user_data);
 
-	WindowMenusPrivate * priv = WINDOW_MENUS_GET_PRIVATE((((gpointer *)user_data)[0]));
+	WindowMenusPrivate * priv = WINDOW_MENUS_GET_PRIVATE(wm);
 	WMEntry * wmentry = g_new0(WMEntry, 1);
-	wmentry->wm = WINDOW_MENUS((((gpointer *)user_data)[0]));
+	wmentry->wm = wm;
 	IndicatorObjectEntry * entry = &wmentry->ioentry;
 
 	wmentry->mi = newentry;
@@ -691,10 +729,9 @@
 
 	g_array_append_val(priv->entries, wmentry);
 
-	g_signal_emit(G_OBJECT((((gpointer *)user_data)[0])), signals[ENTRY_ADDED], 0, entry, TRUE);
+	g_signal_emit(G_OBJECT(wm), signals[ENTRY_ADDED], 0, entry, TRUE);
 
 	g_object_unref(newentry);
-	g_free(user_data);
 
 	return;
 }