After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 766566 - Wayland: gdk_screen_get_monitor_at_window() unreliable under Wayland
Wayland: gdk_screen_get_monitor_at_window() unreliable under Wayland
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Backend: Wayland
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 766722
 
 
Reported: 2016-05-17 13:39 UTC by Olivier Fourdan
Modified: 2016-05-20 15:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH 1/3] display: Add vfunc for get_monitor_at_window (2.19 KB, patch)
2016-05-17 13:43 UTC, Olivier Fourdan
committed Details | Review
[PATCH 2/3] wayland: Add API to retrieve the Wayland output (2.29 KB, patch)
2016-05-17 13:44 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/3] wayland: Add get_monitor_at_window to Wayland backend (2.29 KB, patch)
2016-05-17 13:44 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/3] wayland: Add get_monitor_at_window to Wayland backend (2.30 KB, patch)
2016-05-17 14:08 UTC, Olivier Fourdan
committed Details | Review
[PATCH 2/3] wayland: Add API to retrieve the Wayland output (2.79 KB, patch)
2016-05-18 06:21 UTC, Olivier Fourdan
committed Details | Review
[PATCH] wayland: Make gdk_wayland_window_get_wl_output() private (2.82 KB, patch)
2016-05-19 07:51 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2016-05-17 13:39:52 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.
Comment 1 Olivier Fourdan 2016-05-17 13:43:41 UTC
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.
Comment 2 Olivier Fourdan 2016-05-17 13:44:08 UTC
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.
Comment 3 Olivier Fourdan 2016-05-17 13:44:34 UTC
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.
Comment 4 Emmanuele Bassi (:ebassi) 2016-05-17 13:47:59 UTC
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; ...
    {
      ...
Comment 5 Olivier Fourdan 2016-05-17 14:08:37 UTC
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
Comment 6 Matthias Clasen 2016-05-18 02:43:07 UTC
Review of attachment 328079 [details] [review]:

ok
Comment 7 Matthias Clasen 2016-05-18 02:44:53 UTC
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
Comment 8 Matthias Clasen 2016-05-18 02:52:53 UTC
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 ?
Comment 9 Jonas Ådahl 2016-05-18 03:25:02 UTC
(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.
Comment 10 Olivier Fourdan 2016-05-18 06:21:54 UTC
Created attachment 328113 [details] [review]
[PATCH 2/3] wayland: Add API to retrieve the Wayland output

Updated patch as per comment 7
Comment 11 Matthias Clasen 2016-05-18 14:23:30 UTC
Review of attachment 328113 [details] [review]:

.
Comment 12 Matthias Clasen 2016-05-18 14:24:51 UTC
Review of attachment 328083 [details] [review]:

ok then
Comment 13 Olivier Fourdan 2016-05-18 17:12:11 UTC
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
Comment 14 Jonas Ådahl 2016-05-19 07:00:14 UTC
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.
Comment 15 Olivier Fourdan 2016-05-19 07:24:15 UTC
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.
Comment 16 Jonas Ådahl 2016-05-19 07:30:44 UTC
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.
Comment 17 Olivier Fourdan 2016-05-19 07:40:07 UTC
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.
Comment 18 Jonas Ådahl 2016-05-19 07:46:18 UTC
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.
Comment 19 Olivier Fourdan 2016-05-19 07:51:56 UTC
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.
Comment 20 Jonas Ådahl 2016-05-19 07:56:31 UTC
Review of attachment 328173 [details] [review]:

This looks good to me.
Comment 21 Olivier Fourdan 2016-05-19 08:24:14 UTC
(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
Comment 22 Jonas Ådahl 2016-05-19 08:37:18 UTC
(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.
Comment 23 Olivier Fourdan 2016-05-20 08:46:17 UTC
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...