GNOME Bugzilla – Bug 766566
Wayland: gdk_screen_get_monitor_at_window() unreliable under Wayland
Last modified: 2016-05-20 15:03:49 UTC
Description: Wayland has no global coordinates, unfortunately this is what gdk_screen_get_monitor_at_window() and gdk_display_get_monitor_at_window() use to identify the monitor where the window resides. How reproducible: 100% Steps to reproduce: 1. Build gtk+ 2. Run ./tests/testxinerama under Wayland with a dual monitor setup 3. Move the window from monitor to the other and press "Query current monitor" Actual results: The values displays by "Query current monitor" are always the same, even if the window is moved from a monitor to the other. Expected results: The monitor is updated when the window is queried from different monitors. Additional info: gdk_screen_get_monitor_at_window()/gdk_display_get_monitor_at_window() use the window location to identify the monitor, but there is no such thing in Wayland, as a result, it's always the first monitor which is returned. But Wayland compositors do notify the surfaces when they enter and leave outputs, so the idea would be to add a vfunc to let the backends return a monitor for a given GdkWindow, and if the vfunc is not available, use the current method of testing coordinates. Proposed patches to follow.
Created attachment 328079 [details] [review] [PATCH 1/3] display: Add vfunc for get_monitor_at_window Some backends (namely Wayland) do not support global coordinates so using the window position to determine the monitor will always fail on such backends. In such cases, the backend itself might be better suited to identify the monitor a given window resides on. Add a vfunc get_monitor_at_window() to the display class so that we can use the backend to retrieve the monitor, if the backend implements it.
Created attachment 328080 [details] [review] [PATCH 2/3] wayland: Add API to retrieve the Wayland output In Wayland, surfaces get an enter/leave notification each time they enter or leave an output. Add an API to GdkWaylandWindow to retrieve the output the window has last entered.
Created attachment 328081 [details] [review] [PATCH 3/3] wayland: Add get_monitor_at_window to Wayland backend Given that Wayland has no global coordinate, the only way for gdk to retrieve the monitor a window last entered is to retrieve it from the GdkWaylandWindow itself. Implement the backend specific get_monitor_at_window() to return the monitor that was last entered by the window.
Review of attachment 328081 [details] [review]: ::: gdk/wayland/gdkdisplay-wayland.c @@ +853,3 @@ + + output = gdk_wayland_window_get_wl_output (window); + struct wl_output *output; Instead of nesting the for loop under the if, you can flip the condition check and bail out early: if (output == NULL) return NULL; for (i = 0; ... { ...
Created attachment 328083 [details] [review] [PATCH 3/3] wayland: Add get_monitor_at_window to Wayland backend Updated patch as per review in comment 4
Review of attachment 328079 [details] [review]: ok
Review of attachment 328080 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +2966,3 @@ + * Since: 3.22 + */ +struct wl_output * To make best use of this doc comment, you should also add the function to the wayland section in docs/reference/gdk/gdk3-sections.txt
Review of attachment 328083 [details] [review]: This looks good to me; the only question I have is under what circumstances the output can be NULL. Are we guaranteed to get an enter event right after a surface is created ?
(In reply to Matthias Clasen from comment #8) > Review of attachment 328083 [details] [review] [review]: > > This looks good to me; the only question I have is under what circumstances > the output can be NULL. Are we guaranteed to get an enter event right after > a surface is created ? No. The surface may for example be placed on a non-active workspace, or minimized, or whatever, and if so the surface will have entered no output.
Created attachment 328113 [details] [review] [PATCH 2/3] wayland: Add API to retrieve the Wayland output Updated patch as per comment 7
Review of attachment 328113 [details] [review]: .
Review of attachment 328083 [details] [review]: ok then
attachment 328079 [details] [review] pushed as commit d288a13 - display: Add vfunc for get_monitor_at_window attachment 328113 [details] [review] pushed as commit ca77de0 - wayland: Add API to retrieve the Wayland output attachment 328083 [details] [review] pushed as commit b03784e - wayland: Add get_monitor_at_window to Wayland backend
Review of attachment 328113 [details] [review]: ::: gdk/wayland/gdkwindow-wayland.c @@ +2956,3 @@ +/** + * gdk_wayland_window_get_wl_output: I don't really think this API addition makes much sense, as it just happens to tell the what output the window entered most recently. What this output actually is, in relation to the window, can be entirely arbitrary.
The existing gdk API needs a way to identify the output from a gdkwindow. Using the last entered output is probably sub-optimal, but bit it's still an improvement over the (previous) current situation where the function would always return the first monitor because its heuristic was based on global coordinates. Granted, it's not perfect but it's a lot less wrong than before. With current xdg-shell protocol v5, there is no better way to even have a hint on which output the surface resides. If we extend that in xdg-shell v6 to get a more precise value (like the output the window is mostly in, for example), then we can use the same gdk_wayland_window_get_wl_output() API to return that (better) value, so I don't agree the API addition in itself doesn't make sense.
But do we still need to add the API? I can understand that we need a "best effort" approximation of the GdkWindow vfunc, but I don't see we need to expose the gdk_wayland_* API part.
Oh, you mean you would have kept it private? We do have gdk_wayland_window_get_wl_surface() so it sounded OK to have gdk_wayland_window_get_wl_output() in gdkwaylandwindow.h as well, but if you reckon we should make it private to have it in gdkprivate-wayland.h only, we can move it, no objection from me.
Yea, it makes sense to keep it private, so that no new place starts to rely on something that isn't reliable. Should add the "get_monitor_at_window" to the list of things to deprecate for GTK4 I guess.
Created attachment 328173 [details] [review] [PATCH] wayland: Make gdk_wayland_window_get_wl_output() private As per comment 18: There is no need to make it a public API, move it to the private header instead.
Review of attachment 328173 [details] [review]: This looks good to me.
(In reply to Jonas Ådahl from comment #18) > Yea, it makes sense to keep it private, so that no new place starts to rely > on something that isn't reliable. But again, there is no reason for it to be unreliable, we "just" have to add the relevant missing protocol bits to make it reliable. > Should add the "get_monitor_at_window" to the list of things to deprecate > for GTK4 I guess. Yet I am not entirely convinced "get_monitor_at_window()" is not useful, even more when there is no global coordinates like in Wayland, it's pretty much the only remaining way to identify the monitor, unless we reckon identifying the monitor is not something clients shouldn't be able to do. Meanwhile: attachment 328173 [details] [review] has been pushed as commit 0b58c96 - wayland: Make gdk_wayland_window_get_wl_output() private
(In reply to Olivier Fourdan from comment #21) > (In reply to Jonas Ådahl from comment #18) > > Yea, it makes sense to keep it private, so that no new place starts to rely > > on something that isn't reliable. > > But again, there is no reason for it to be unreliable, we "just" have to add > the relevant missing protocol bits to make it reliable. > > > Should add the "get_monitor_at_window" to the list of things to deprecate > > for GTK4 I guess. > > Yet I am not entirely convinced "get_monitor_at_window()" is not useful, > even more when there is no global coordinates like in Wayland, it's pretty > much the only remaining way to identify the monitor, unless we reckon > identifying the monitor is not something clients shouldn't be able to do. True. We should figure out the use cases where it is needed and see what protocol would fit that use case. For example the adaptive-menu-height would not directly benefit from from knowing the wl_output since the work area is still unknown, but could use for example a "max size" hint as part of the post-position configuration. We wouldn't be able to just use a "you're mostly here" kind of monitor state, because if a menu is opened on a monitor which a window is a overlapping a bit less, we'd still use the wrong monitor for deciding the size. As for toplevels, I'm guessing it mostly matters before being mapped, and then it won't know exactly where either, but I suppose that could also be done as some kind of max size configuration event as part of the xdg_toplevel configuration. So in the end, I'm not sure we'd end up with just a simple "get monitor where its mostly at" kind of API. > > Meanwhile: > > attachment 328173 [details] [review] [review] has been pushed as commit 0b58c96 - > wayland: Make gdk_wayland_window_get_wl_output() private Thanks! I think its the safest for now at least.
For the record, these patches break some menu positioning in master with multi-monitor setups because, in various places (including gtkcombobox), gtk compares the monitor returned from the device event's root coordinates against the monitor's position and size returned by get_monitor_at_window(). Since there is no such thing as global root coordinates in Wayland (just fake root coordinates which are toplevel relative), that will not match now that get_monitor_at_window() returns the real monitor. After discussing with Jonas on irc, I'd rather not revert these patches though (they do return the right monitor, whereas before it worked by chance as both get_monitor_at_window() and get_monitor_at() where based on root coordinates, so were equally wrong), because the menu positioning will be entirely reworked to be done in gdk backends so this is will be fixed with bug 756579 that will be discussed soon at the forthcoming gtk hackfest...