GNOME Bugzilla – Bug 759161
wayland: dialog stacking without parents (RFC)
Last modified: 2016-01-08 11:00:42 UTC
Created attachment 316915 [details] Attaching simple program to demonstrate the issue Description: Some applications use the type hint "dialog" without specifying a transient. On X11, most fine window managers like mutter will treat the dialog window as a "transient for the group" and keep the dialog above the other windows of the same group. On Wayland, xdg-shell features the set_parent() [1] method to specify that a dialog is related to a parent window, but unfortunately there is no such thing as "transient for group" in Wayland. On wayland-devel mailing list [2], I was told that "the client must have absolute final control, with no ambiguity, over the stacking order and visibility of every surface" (unfortunately th reply does not show in the ML archives so I cannot link it...). So that means if we want to replicate the behaviour with dialogs without transient (parent) in Wayland, we have to manage the stacking in the applications, thus gtk+. I reckon that would add some complexity to gtk+/gdk and its Wayland back-end. So. is this something we want for gtk+? Steps to reproduce: 1. Save and build the attach program 2. Run it on X11 and on Wayland Actual result: On X11, the dialog type window is kept above the two other windows. On Wayland, all windows and can be raised independently. Expected result: Consistent stacking between Wayland and X11. Additional info: [1] http://cgit.freedesktop.org/wayland/wayland-protocols/tree/unstable/xdg-shell/xdg-shell-unstable-v5.xml#n171 [2] http://lists.freedesktop.org/archives/wayland-devel/2015-November/025726.html
One possibility to solve this problem would be for gtk+/gdk/gdkwayland to set_parent() on the current active window.
we'll need the patch from bug 755606 for this to work, as Jonas pointed out on irc.
It's worth noting that the reply given there (not on the list) isn't actually representative of the community's opinion in general: it's one person's opinion, and every time this has come up, people haven't much agreed with him. I wouldn't pay it much attention.
Created attachment 316949 [details] [review] wayland: Update parent of dialogs without transient This requires attachment 312099 [details] [review] in mutter to work. X11 has the notions of "transient for group", and while it's an ICCCM violation, it's commonly used and documented that a window manager would treat a window with transient_for set to None to transient for all windows of its group. gtk+ uses this when an application set a dialog type window but does not specify an explicit transient. While this works on X11, there is no such thing as groups in Wayland and the closest equivalent which is set_parent() in xdg-shell takes only one parent. This is what is used for modal dialogs. To get something similar in behavior to what is available on X11, a solution is to update the parent() of the dialogs without transients when the active surface changes.
(In reply to Daniel Stone from comment #3) > It's worth noting that the reply given there (not on the list) isn't > actually representative of the community's opinion in general: it's one > person's opinion, and every time this has come up, people haven't much > agreed with him. I wouldn't pay it much attention. OK, thanks for clarifying! :-)
If you want my opinion, dialogs should always have parents. That is why GTK+ spews an annoying message nowadays if you don't set a parent.
Err, nope, gtk+ doesn't necessarily complain, see the code I posted in attachment 316915 [details] it's all using valid gtk+ routines.
Sure, you can create windows that look a bit like dialogs, and we won't complain. For the purpose of this discussion, I consider as dialogs the things that are returned by gtk_dialog_new()...
Well, my point was that gtk+ API allows for such things to happen and apps do make use of it (e.g. gnome-terminal's About dialog). We can certainly fix the apps, but as long the API allows it (even if discouraged with an annoying message), apps may use it and we might as well handle it, thus this RFC bug/patch.
Review of attachment 316949 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +713,3 @@ static void +gdk_wayland_window_sync_parent (GdkWindow *window, + GdkWindow *parent) You add a parent argument here, but all callers pass NULL ? @@ +1972,3 @@ impl->transient_for = parent; + gdk_wayland_window_sync_parent (window, NULL); Did you mean to pass the parent in here ?
(In reply to Matthias Clasen from comment #10) > Review of attachment 316949 [details] [review] [review]: > > ::: gdk/wayland/gdkwindow-wayland.c > @@ +713,3 @@ > static void > +gdk_wayland_window_sync_parent (GdkWindow *window, > + GdkWindow *parent) > > You add a parent argument here, but all callers pass NULL ? Nope, not all. The parent is used in gdk_wayland_window_update_dialogs() precisely when no transient is set: + /* Update the parent relationship only for dialogs without transients */ + if (!impl->transient_for) + gdk_wayland_window_sync_parent (w, window); > @@ +1972,3 @@ > impl->transient_for = parent; > > + gdk_wayland_window_sync_parent (window, NULL); > > Did you mean to pass the parent in here ? No, not here (although we could), there is no need as impl->transient_for is set just above.
Created attachment 317059 [details] [review] [PATCH v2] wayland: Update parent of dialogs without transient Changes in v2: - Don't leak the toplevel list. - Don't apply set_parent() between dialogs otherwize mutter/gnome-shell will freeze in an infinite loop. Note: There is (small, imho) caveat in this approach, inherent to the Wayland protocol: If there is more than one "transient-less" dialog (which is rare in a real life scenario), the resulting stacking order might be wrong between the dialogs. That's because there is no stacking info of the toplevels on Wayland (i.e. gdk_screen_get_window_stack() won't work so we're bound to use gdk_screen_get_toplevel_windows() to traverse the window tree, and it doesn't follow the actual current window stacking order).
Review of attachment 317059 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +1024,3 @@ + if (impl->hint != GDK_WINDOW_TYPE_HINT_DIALOG && + new_state & GDK_WINDOW_STATE_FOCUSED) + gdk_wayland_window_update_dialogs (window); So whenever a non-dialog is focused, we create and iterate a list of all toplevels. Not sure I like that. Could we instead keep track of whether there are any orphan dialogs, and only do the work if we have any ?
(In reply to Matthias Clasen from comment #13) > So whenever a non-dialog is focused, we create and iterate a list of all > toplevels. Not sure I like that. Could we instead keep track of whether > there are any orphan dialogs, and only do the work if we have any ? Yes, we could do that, we could either maintain a list of orphans dialogs (ie dialogs without transients), where would this list be kept, gdkscreen-wayland?
Note, this can crash mutter, see bug 759297
(In reply to Olivier Fourdan from comment #14) > (In reply to Matthias Clasen from comment #13) > > So whenever a non-dialog is focused, we create and iterate a list of all > > toplevels. Not sure I like that. Could we instead keep track of whether > > there are any orphan dialogs, and only do the work if we have any ? > > Yes, we could do that, we could either maintain a list of orphans dialogs > (ie dialogs without transients), where would this list be kept, > gdkscreen-wayland? I was thinking just a count would be enough ? or just a boolean, really. Everytime an orphan dialog appears, you set it to true, and everytime you walk the list, you check if there's any orphans left after you're done.
(In reply to Olivier Fourdan from comment #15) > Note, this can crash mutter, see bug 759297 Sounds like a good reason to way with landing this... Either gdkscreen-wayland.c or gdkwindow-wayland.c sound ok for code tracking orphan dialogs.
s/way/wait/
Sure, no hurry here, it's been like that for years and it's not a crasher bug.
Created attachment 317371 [details] [review] [PATCH v3] wayland: Update parent of dialogs without transient New patch maintaining a list of orphan dialogs.
Review of attachment 317371 [details] [review]: ::: gdk/wayland/gdkscreen-wayland.c @@ +1275,3 @@ + GdkWaylandScreen *screen_wayland = GDK_WAYLAND_SCREEN (screen); + + return g_list_copy (screen_wayland->orphan_dialogs); I would prefer to return the list itself here, without a copy. This is not public api, and we know the only caller, so seems quite safe. Alternatively (and maybe preferrably) keep the entire orphan dialog business static in gdkwindow-wayland.c - there's only one screen object anyway, and we avoid adding these (internal) apis.
(In reply to Olivier Fourdan from comment #9) > Well, my point was that gtk+ API allows for such things to happen and apps > do make use of it (e.g. gnome-terminal's About dialog). > > We can certainly fix the apps, but as long the API allows it (even if > discouraged with an annoying message), apps may use it and we might as well > handle it, thus this RFC bug/patch. FWIW, it was a conscious decision to not have modal dialogs in gnome-terminal, to the extent that those "dialogs" have now been changed to GtkWindows to suppress the WARNINGs. For the specific case of the about dialog see: https://git.gnome.org/browse/gnome-terminal/commit/?id=222239546871ff88a2fe7205e89eb1bf8b31c43a
Modals and dialogs are not exactly the same thing, you can have non-modal dialogs.
Sorry, I mean, I know you know that, I just wanted to mention that here for the clarity of the discussion and the scope/purpose of this bug/patch :)
Created attachment 317628 [details] [review] [PATCH v4] wayland: Update parent of dialogs without transient Updated patch following review.
Review of attachment 317628 [details] [review]: Looks good to me, otherwise ::: gdk/wayland/gdkwindow-wayland.c @@ +190,3 @@ +{ + if (!g_list_find (orphan_dialogs, window)) + orphan_dialogs = g_list_append (orphan_dialogs, window); If order doesn't matter, we normally prefer g_list_prepend to avoid an unnecessary list traversal
Review of attachment 317628 [details] [review]: Replaced g_list_append() with g_list_prepend() as suggested and pushed attachment 317628 [details] [review] as commit 120088b wayland: Update parent of dialogs without transient