Merge lp:~ted/indicator-appmenu/transient-windows into lp:indicator-appmenu/0.3

[Ted Gould]

Use the new transient window functions in bamf to try and choose the right menu.

[David Barth (dbarth)]

I don't like that the 'while' may loop endlessly if there is an issue with the stack of parent and transient windows, especially for unsupported applications which will have no menus.
I would have expected the transient window to be the focused one, and the code to look for the /parent/ menu (ie bamf_get_parent?).

If bamf already takes care of mapping app windows and their transient windows, why is a traversal of the stack required?

review: Needs Information

[Ted Gould (ted)]

On Wed, 2010-06-16 at 14:07 +0000, David Barth wrote:
> I don't like that the 'while' may loop endlessly if there is an issue with
> the stack of parent and transient windows, especially for unsupported
> applications which will have no menus.

I'm not sure what you want to do here. Some sort of max number of
parented windows? What would that max be?

> I would have expected the transient window to be the focused one, and the
> code to look for the /parent/ menu (ie bamf_get_parent?).
>
> If bamf already takes care of mapping app windows and their transient
> windows, why is a traversal of the stack required?

My understanding is that you can be transient for a transient window in
theory. So we should be able to have about on top of the GIMP toolbox
which is on top of the GIMP image window.

[Jason Smith (jassmith)]

From a technical standpoint there is no potential for infinite loop with this. Bamf clearly separates transient windows from non-transient windows. Excluding cases where WM_TRANSIENT_FOR is set (which wont contain loops) bamf only allows transient windows to be transients for non-transient windows to avoid just this kind of loop.

Excluding a couple rare conditions with misbehaving applications, the parent transient tree will not be more than 1 deep.

[Jason Smith (jassmith)]

Except gimp doesn't set the transient hint HUZZAH!

=== modified file 'src/indicator-appmenu.c'
--- src/indicator-appmenu.c	2010-06-12 02:30:44 +0000
+++ src/indicator-appmenu.c	2010-06-16 04:36:27 +0000
@@ -408,10 +408,20 @@
 
 	IndicatorAppmenu * appmenu = INDICATOR_APPMENU(user_data);
 
-	guint32 xid = bamf_window_get_xid(window);
-	g_debug("window changed to %d", xid);
+	WindowMenus * menus = NULL;
+	guint32 xid = 0;
+
+	while (window != NULL && menus == NULL) {
+		xid = bamf_window_get_xid(window);
 	
-	WindowMenus * menus = g_hash_table_lookup(appmenu->apps, GINT_TO_POINTER(xid));
+		menus = g_hash_table_lookup(appmenu->apps, GUINT_TO_POINTER(xid));
+
+		if (menus == NULL) {
+			window = bamf_window_get_transient(window);
+		}
+	}
+
+	g_debug("Switching to windows from XID %d", xid);
 
 	/* Note: This function can handle menus being NULL */
 	switch_default_app(appmenu, menus);
@@ -457,14 +467,8 @@
 
 		/* Node: Does not cause ref */
 		BamfWindow * win = bamf_matcher_get_active_window(iapp->matcher);
-		guint32 xid = 0;
-		if (BAMF_IS_WINDOW(win)) {
-			xid = bamf_window_get_xid(win);
-		}
 
-		if (xid != 0 && xid == windowid) {
-			switch_default_app(iapp, wm);
-		}
+		active_window_changed(iapp->matcher, NULL, BAMF_VIEW(win), iapp);
 	} else {
 		g_warning("Already have a menu for window ID %d with path %s from %s", windowid, objectpath, dbus_g_method_get_sender(method));
 	}