Merge lp:~ted/indicator-appmenu/destruction-will-happen-in-time into lp:indicator-appmenu/0.3

[Ted Gould]

So there's a glitch in the information coming from BAMF. It's a small glitch, but we were responding to it harshly. Destroying everything when it told us to. But, now we know we can't trust it. So this branch removes that trust.

It solves it the same way that you'd solve audio glitches, with a low pass filter. Basically a timeout for the computer scientists. We're averaging the signals. Which means we're only freeing the menus after we're told, *and* there's been enough time we're sure that BAMF has it's head on straight. If BAMF changes it's mind in that time, we assume it's a glitch.

Unfortunately this has a set of reprocussions. One of those is that we can get registrations for menus that we already "have" as we're still waiting to see if BAMF changed it's mind. To handle that we're allowing reregistration and destroying the previous menus. This shouldn't happen regularly, so it's not going to be that expensive.

[Neil J. Patel (njpatel)]

guint xid = bamf_window_get_xid(window) needs to be guint32 but that's it really, nice work. Approving to avoid another review cycle.

review: Approve

=== modified file 'src/indicator-appmenu.c'
--- src/indicator-appmenu.c	2011-03-16 03:49:25 +0000
+++ src/indicator-appmenu.c	2011-04-06 20:01:00 +0000
@@ -106,6 +106,8 @@
 	GDBusConnection * bus;
 	guint owner_id;
 	guint dbus_registration;
+
+	GHashTable * destruction_timers;
 };
 
 
@@ -213,6 +215,9 @@
                                                                       gpointer user_data);
 static void menus_destroyed                                          (GObject * menus,
                                                                       gpointer user_data);
+static void source_unregister                                        (gpointer user_data);
+static GVariant * unregister_window                                  (IndicatorAppmenu * iapp,
+                                                                      guint windowid);
 
 /* Unique error codes for debug interface */
 enum {
@@ -302,6 +307,9 @@
 	self->desktop_windows = g_hash_table_new(g_direct_hash, g_direct_equal);
 	self->desktop_menu = NULL; /* Starts NULL until found */
 
+	/* Set up the hashtable of destruction timers */
+	self->destruction_timers = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, source_unregister);
+
 	build_window_menus(self);
 
 	/* Get the default BAMF matcher */
@@ -424,6 +432,14 @@
 		iapp->dbus_registration = 0;
 	}
 
+	if (iapp->destruction_timers != NULL) {
+		/* These are in dispose and not finalize becuase the dereference
+		   function removes timers that could need the object to be in
+		   a valid state, so it's better to have them in dispose */
+		g_hash_table_destroy(iapp->destruction_timers);
+		iapp->destruction_timers = NULL;
+	}
+
 	if (iapp->bus != NULL) {
 		g_object_unref(iapp->bus);
 		iapp->bus = NULL;
@@ -755,12 +771,16 @@
 	}
 
 	BamfWindow * window = BAMF_WINDOW(view);
+	IndicatorAppmenu * iapp = INDICATOR_APPMENU(user_data);
+	guint xid = bamf_window_get_xid(window);
+
+	/* Make sure we don't destroy it later */
+	g_hash_table_remove(iapp->destruction_timers, GUINT_TO_POINTER(xid));
+
 	if (bamf_window_get_window_type(window) != BAMF_WINDOW_DESKTOP) {
 		return;
 	}
 
-	IndicatorAppmenu * iapp = INDICATOR_APPMENU(user_data);
-	guint xid = bamf_window_get_xid(window);
 	g_hash_table_insert(iapp->desktop_windows, GUINT_TO_POINTER(xid), GINT_TO_POINTER(TRUE));
 
 	g_debug("New Desktop Window: %X", xid);
@@ -778,6 +798,32 @@
 	return;
 }
 
+typedef struct _destroy_data_t destroy_data_t;
+struct _destroy_data_t {
+	IndicatorAppmenu * iapp;
+	guint32 xid;
+};
+
+/* Timeout to finally cleanup the window.  Causes is to ignore glitches that
+   come from BAMF/WNCK. */
+static gboolean
+destroy_window_timeout (gpointer user_data)
+{
+	destroy_data_t * destroy_data = (destroy_data_t *)user_data;
+	g_hash_table_steal(destroy_data->iapp->destruction_timers, GUINT_TO_POINTER(destroy_data->xid));
+	unregister_window(destroy_data->iapp, destroy_data->xid);
+	return FALSE; /* free's data through source deregistration */
+}
+
+/* Unregisters the source in the hash table when it gets removed.  This ensure
+   we don't leave any timeouts around */
+static void
+source_unregister (gpointer user_data)
+{
+	g_source_remove(GPOINTER_TO_UINT(user_data));
+	return;
+}
+
 /* When windows leave us, this function gets called */
 static void
 old_window (BamfMatcher * matcher, BamfView * view, gpointer user_data)
@@ -790,25 +836,12 @@
 	BamfWindow * window = BAMF_WINDOW(view);
 	guint32 xid = bamf_window_get_xid(window);
 
-	/* See if it's in our list of desktop windows, if
-	   so remove it from that list. */
-	if (bamf_window_get_window_type(window) == BAMF_WINDOW_DESKTOP) {
-		g_hash_table_remove(iapp->desktop_windows, GUINT_TO_POINTER(xid));
-	}
-
-	/* Now let's see if we've got a WM object for it then
-	   we need to mark it as destroyed and unreference to
-	   actually destroy it. */
-	gpointer wm = g_hash_table_lookup(iapp->apps, GUINT_TO_POINTER(xid));
-	if (wm != NULL) {
-		GObject * wmo = G_OBJECT(wm);
-
-		/* Using destroyed so that if the menus are shown
-		   they'll be switch and the current window gets
-		   updated as well. */
-		menus_destroyed(wmo, iapp);
-		g_object_unref(wmo);
-	}
+	destroy_data_t * destroy_data = g_new0(destroy_data_t, 1);
+	destroy_data->iapp = iapp;
+	destroy_data->xid = xid;
+
+	guint source_id = g_timeout_add_seconds_full(G_PRIORITY_LOW, 5, destroy_window_timeout, destroy_data, g_free);
+	g_hash_table_replace(iapp->destruction_timers, GUINT_TO_POINTER(xid), GUINT_TO_POINTER(source_id));
 
 	return;
 }
@@ -1272,6 +1305,9 @@
 {
 	g_debug("Registering window ID %d with path %s from %s", windowid, objectpath, sender);
 
+	/* Shouldn't do anything, but let's be sure */
+	g_hash_table_remove(iapp->destruction_timers, GUINT_TO_POINTER(windowid));
+
 	if (g_hash_table_lookup(iapp->apps, GUINT_TO_POINTER(windowid)) == NULL && windowid != 0) {
 		WindowMenus * wm = window_menus_new(windowid, sender, objectpath);
 		g_return_val_if_fail(wm != NULL, FALSE);
@@ -1294,7 +1330,19 @@
 		if (windowid == 0) {
 			g_warning("Can't build windows for a NULL window ID %d with path %s from %s", windowid, objectpath, sender);
 		} else {
-			g_warning("Already have a menu for window ID %d with path %s from %s", windowid, objectpath, sender);
+			g_warning("Already have a menu for window ID %d with path %s from %s, unregistering that one", windowid, objectpath, sender);
+			unregister_window(iapp, windowid);
+
+			/* NOTE: So we're doing a lookup here.  That seems pretty useless
+			   now doesn't it.  It's for a good reason.  We're going recursive
+			   with a pretty complex set of functions we want to ensure that
+			   we're not going to end up infinitely recursive otherwise things
+			   could go really bad. */
+			if (g_hash_table_lookup(iapp->apps, GUINT_TO_POINTER(windowid)) == NULL) {
+				return register_window(iapp, windowid, objectpath, sender);
+			}
+
+			g_warning("Unable to unregister window!");
 		}
 	}
 
@@ -1309,35 +1357,26 @@
 	g_return_val_if_fail(IS_INDICATOR_APPMENU(iapp), NULL);
 	g_return_val_if_fail(iapp->matcher != NULL, NULL);
 
-	/* Get the application that uses that XID */
-	BamfApplication * app = bamf_matcher_get_application_for_xid(iapp->matcher, windowid);
-	if (app == NULL) {
-		return NULL;
-	}
-	g_object_ref_sink(G_OBJECT(app));
-
-	/* Figure out which window is associated with it */
-	GList * windows = bamf_application_get_windows(app);
-	GList * lwindow;
-	BamfWindow * window = NULL;
-	for (lwindow = windows; lwindow != NULL; lwindow = g_list_next(lwindow)) {
-		BamfWindow * thiswindow = BAMF_WINDOW(lwindow->data);
-		if (bamf_window_get_xid(thiswindow) == windowid) {
-			window = thiswindow;
-			break;
-		}
-	}
-	g_list_free(windows);
-
-	if (window == NULL) {
-		g_object_unref(app);
-		return NULL;
-	}
-
-	/* Call the old_window handler for that window */
-	old_window(iapp->matcher, BAMF_VIEW(window), iapp);
-
-	g_object_unref(app);
+	/* Make sure we don't destroy it later */
+	g_hash_table_remove(iapp->destruction_timers, GUINT_TO_POINTER(windowid));
+
+	/* If it's a desktop window remove it from that table as well */
+	g_hash_table_remove(iapp->desktop_windows, GUINT_TO_POINTER(windowid));
+
+	/* Now let's see if we've got a WM object for it then
+	   we need to mark it as destroyed and unreference to
+	   actually destroy it. */
+	gpointer wm = g_hash_table_lookup(iapp->apps, GUINT_TO_POINTER(windowid));
+	if (wm != NULL) {
+		GObject * wmo = G_OBJECT(wm);
+
+		/* Using destroyed so that if the menus are shown
+		   they'll be switch and the current window gets
+		   updated as well. */
+		menus_destroyed(wmo, iapp);
+		g_object_unref(wmo);
+	}
+
 	return NULL;
 }