GNOME Bugzilla – Bug 335524
Warn when apps set WM_TRANSIENT_FOR to a non-top-level window
Last modified: 2006-04-19 21:01:49 UTC
Please describe the problem: When calculating the stacking, metacity attempts to place transient windows above their parents. Normally this works just fine. However when a transient window is transient for a non-top-level window, metacity just treats it as a popup which it sinks to prevent focus stealing. This is very bad when a dialog is transient on something which is, for example, XEmbed(ded) via a GTK plug/socket arrangement. Steps to reproduce: Embed an app which does a transient dialog into another window, get it to pop up its transient dialog. Actual results: Metacity will sink the dialog so that it does not steal the focus Expected results: Metacity should allow the dialog to be on the top of the window containing the embedded app Does this happen every time? Yes Other information:
Created attachment 61764 [details] [review] Proposed fix by walking the parent tree when setting xtransient_for This patch is what I added to the ubuntu package of metacity in an attempt to correct the problem. It works by walking the parent tree of the designated transient window until it finds a window managed by metacity (or the root window) whereupon it records that window instead. This appears to behave as desired in my tests.
The 'MetaWindow *mw;' at the top of the patch is vestigial and can be dropped of course.
This isn't a bug, really -- not in metacity anyway. According to section 4.1.2.6 of the ICCCM, "The WM_TRANSIENT_FOR property (of type WINDOW) contains the ID of another top-level window. The implication is that this window is a pop-up on behalf of the named window, and window managers...may treat them differently..." So, it sounds like you should file a bug against the relevant app and request that they do the XQueryTree in order to obtain a toplevel window -- they will likely run into this bug or similar ones in other window managers besides just Metacity. I'll leave this bug open so we can discuss whether it's a good idea to try to handle these braindead apps or whether doing so just encourages more dumb behavior. I'll need to get Havoc to weigh in if others think it's a good idea.
On IRC, it was discovered that this patch also causes a crash, though there's apparently an upstream Ubuntu bug about this somewhere. Useful to keep in mind if we decide we do want this workaround behavior. If we don't want this workaround behavior, we should probably modify the clear the WM_TRANSIENT_FOR property for these windows and spit out a warning.
The crash was diagnosed as happening when the WM_TRANSIENT_FOR property was set to zero which seemed to occur when a window was killed before a window which was transient on it. The solution was to check that case in the condition of the while loop. The new patch is in ubuntu package numbered 2.14.1-0ubuntu3
Cool, could you add a link to the Ubuntu bug with the new patch and/or attach it here? I've pinged Havoc to ask him about which route he wants to take. I'm leaning slightly towards clearing the WM_TRANSIENT_FOR setting and spitting out a warning because I'm afraid we'd encourage buggy apps if we just worked around them in this way, but I think Havoc would have a better feel for that.
There is a lot to be said for discouraging incorrect usage of things like WM_TRANSIENT_FOR, however there is also a lot to be said for carrying a patch for a while which obeys (by correcting) the incorrect usage and spits out a warning. It strikes me that a large number of the "buggy apps" could be fixed up by a patch to GTK+ to ensure that when it sets a WM_TRANSIENT_FOR hint, it does so by performing the XQueryTree loop itself. Then once GTK+ is patched and released, metacity can decide on a day to remove its support for other incorrect apps. I will extract the patch from the ubuntu package and attach it soon.
I don't think the XQueryTree is a good idea for several reasons: - it's an expensive operation (round trips) - it's a race unless you grab the server (you can't walk up the tree atomically unless you do), if you do it's even slower - we'd have to push an error trap also since any of the windows in the query tree can be destroyed at any time -> more penalty - it's code complexity - the app is doing something clearly broken (and incorrect since the ICCCM over a decade ago), that other WMs also won't handle
Okay, so updating the patch accordingly. It'd probably be good to spit out a warning message when an app sets WM_TRANSIENT_FOR to something that isn't a valid toplevel window. Should be a good project for someone looking to get involved in Gnome; they'd just need to look at reload_transient_for(), meta_warning(), and meta_display_lookup_x_window(). Updating the title to match and adding the gnome-love keyword.
Is it possible that this bug is a problem with how Metacity treats transients of transients? This bug seem to be similar to #336184 - transients of transiets was treated differently from transients of main windows. The fix there was to use meta_window_find_root_ancestor() to find the grouping window instead of relying on xtransient_for. Could a similar fix be applied here?
(In reply to comment #10) > The fix [in bug 336184] was to use meta_window_find_root_ancestor() to find > the grouping window instead of relying on xtransient_for. That was part of the fix but there was a much more important fix in that bug -- distinguishing the group_leader window from the window itself (the root ancestor and root ancestor's group_leader need not be the same window and usually aren't). > Could a similar fix be applied here? No. This bug is about windows that set a "parent" that is invalid as far as Metacity is concerned, which makes us unable to (reasonably) trace back to the "root ancestor". In X, each "window" (meaning top-level window) is composed of dozens, if not hundreds, of what the toolkit would call "widgets"; in X, each widget is an Xwindow (though not a top-level window). This bug is about setting the transient_for hint to a "widget" instead of a "window". While there are ways to trace up a widget hierarchy to get to the top-level window, Havoc points out why it's a bad idea to have the WM try to do so for the app. The app should be the one to do that. I think all we should do here is check if the WM_TRANSIENT_FOR hint points to a valid window (using meta_display_lookup_x_window()), and if it's invalid, spit out a warning and ignore the hint.
Created attachment 63831 [details] [review] Ignore hint and spit warning if transient_for is not a top-level window patch according to comment #11 I don't know of an app that does this so the patch is untested. It does however compile and does not give warnings to valid cases. This is my first patch to metacity. Apply at own risk ;)
Looks good overall, thanks. :) I just have a couple nitpicks: - It looks like you used the -u option to diff which is good. But -p in addition to -u is even better ;-) Please use both when creating future patches (i.e. 'cvs diff -up' instead of 'cvs diff -u'), as it makes it easier to review. - Metacity coding style is to have braces on their own line, indented two spaces past the if and with the code in the block two more spaces past the if. - We also try to avoid lines longer than 80 columns, where possible.
Created attachment 63836 [details] [review] updated patch oh off cause. I should have been more carefull with the coding style. Sorry about that. The warning is still past the 80 columns. This happens numerous times in this file when dealing with meta_warning/meta_verbose. Is this okay?
Yeah, that'll work. :) Can you add an entry for the ChangeLog (just post it in a comment here) and I'll commit it?
2006-04-19 Thomas Andersen <phomes@gmail.com> * src/window-props.c (reload_transient_for) warn and ignore if transient_for is set to a non-top-level window
Committed, thanks!