Merge lp:~mterry/indicator-appmenu/less-racy-window-closing into lp:indicator-appmenu/0.3

[Michael Terry]

I noticed an issue with shotwell menus over several opens and closes. It appeared that indicator-appmenu was not noticing when it closed, and when it opened again, giving it the old stale menus which was not desired.

Looking into it, it appeared to be a race condition between appmenu-gtk calling RegisterWindow and bamf knowing about the window from libwnck.

To make this less racy, I moved the window-close watching up a level, from the WindowMenus object itself to the IndicatorAppmenu object. Now all window-closes are watched, and the windows are removed from the hash table if they exist.

There's still a slight race here, that they could be opened and closed before RegisterWindow hits us, but that seems low-risk and certainly better than the current situation where every open is a race unto itself.

[Michael Terry (mterry)]

To be clear, the race before was that bamf_matcher_get_application_for_xid might return NULL if bamf didn't know about the xid yet. Thus, the WindowMenus object wouldn't watch for its own destruction and would hang around forever.

[Ted Gould (ted)]

review: Approve

=== modified file 'src/indicator-appmenu.c'
--- src/indicator-appmenu.c	2011-01-25 16:39:51 +0000
+++ src/indicator-appmenu.c	2011-02-02 20:48:58 +0000
@@ -776,13 +776,17 @@
 		return;
 	}
 
+	IndicatorAppmenu * iapp = INDICATOR_APPMENU(user_data);
 	BamfWindow * window = BAMF_WINDOW(view);
-	if (bamf_window_get_window_type(window) != BAMF_WINDOW_DESKTOP) {
-		return;
-	}
+	guint32 xid = bamf_window_get_xid(window);
 
-	IndicatorAppmenu * iapp = INDICATOR_APPMENU(user_data);
-	g_hash_table_remove(iapp->desktop_windows, GUINT_TO_POINTER(bamf_window_get_xid(window)));
+	if (bamf_window_get_window_type(window) == BAMF_WINDOW_DESKTOP) {
+		g_hash_table_remove(iapp->desktop_windows, GUINT_TO_POINTER(xid));
+	}
+	else {
+		g_debug("Window removed for window: %d", xid);
+		g_hash_table_remove(iapp->apps, GUINT_TO_POINTER(xid));
+	}
 
 	return;
 }

=== modified file 'src/window-menus.c'
--- src/window-menus.c	2011-01-26 22:21:20 +0000
+++ src/window-menus.c	2011-02-02 20:48:58 +0000
@@ -26,7 +26,6 @@
 #include <libdbusmenu-gtk/menu.h>
 #include <glib.h>
 #include <gio/gio.h>
-#include <libbamf/bamf-matcher.h>
 
 #include "window-menus.h"
 #include "indicator-appmenu-marshal.h"
@@ -47,8 +46,6 @@
 	gchar * retry_name;
 	GVariant * retry_data;
 	guint   retry_timestamp;
-	BamfApplication *app;
-	gulong window_removed_id;
 };
 
 typedef struct _WMEntry WMEntry;
@@ -81,7 +78,6 @@
 static void window_menus_init       (WindowMenus *self);
 static void window_menus_dispose    (GObject *object);
 static void window_menus_finalize   (GObject *object);
-static void window_removed          (GObject * gobject, BamfView * view, gpointer user_data);
 static void root_changed            (DbusmenuClient * client, DbusmenuMenuitem * new_root, gpointer user_data);
 static void menu_entry_added        (DbusmenuMenuitem * root, DbusmenuMenuitem * newentry, guint position, gpointer user_data);
 static void menu_entry_removed      (DbusmenuMenuitem * root, DbusmenuMenuitem * oldentry, gpointer user_data);
@@ -167,12 +163,6 @@
 
 	WindowMenusPrivate * priv = WINDOW_MENUS_GET_PRIVATE(object);
 
-	if (priv->app != NULL) {
-		g_signal_handler_disconnect(priv->app, priv->window_removed_id);
-		g_object_unref(G_OBJECT(priv->app));
-		priv->app = NULL;
-	}
-
 	if (priv->root != NULL) {
 		g_object_unref(G_OBJECT(priv->root));
 		priv->root = NULL;
@@ -424,12 +414,6 @@
 		root_changed(DBUSMENU_CLIENT(priv->client), root, newmenu);
 	}
 
-	priv->app = bamf_matcher_get_application_for_xid(bamf_matcher_get_default(), windowid);
-	if (priv->app) {
-		g_object_ref(priv->app);
-		priv->window_removed_id = g_signal_connect(G_OBJECT(priv->app), "window-removed", G_CALLBACK(window_removed), newmenu);
-	}
-
 	return newmenu;
 }
 
@@ -464,24 +448,6 @@
 	return;
 }
 
-static void
-window_removed (GObject * gobject, BamfView * view, gpointer user_data)
-{
-	WindowMenus * wm = WINDOW_MENUS(user_data);
-	WindowMenusPrivate * priv = WINDOW_MENUS_GET_PRIVATE(wm);
-
-	if (!BAMF_IS_WINDOW(view)) {
-		return;
-	}
-
-	BamfWindow * window = BAMF_WINDOW(view);
-
-	if (bamf_window_get_xid(window) == priv->windowid) {
-		g_debug("Window removed for window: %d", priv->windowid);
-		g_object_unref(G_OBJECT(wm));
-	}
-}
-
 /* Get the location of this entry */
 guint
 window_menus_get_location (WindowMenus * wm, IndicatorObjectEntry * entry)