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 766722 - Wayland: gtkcombox menu misplaced in master
Wayland: gtkcombox menu misplaced in master
Status: RESOLVED DUPLICATE of bug 756579
Product: gtk+
Classification: Platform
Component: Backend: Wayland
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on: 766566
Blocks:
 
 
Reported: 2016-05-20 15:03 UTC by Olivier Fourdan
Modified: 2016-09-28 07:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH RFC] wayland: use get_monitor_at_point() consistently (7.95 KB, patch)
2016-05-20 15:06 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/4] display: Add vfunc for has_global_coords() (2.60 KB, patch)
2016-05-25 07:06 UTC, Olivier Fourdan
none Details | Review
[PATCH 2/4] wayland: Add has_global_coords() hook in Wayland (1.22 KB, patch)
2016-05-25 07:07 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4] gtkprivate: Add gtk_translate_fake_coordinates() (3.20 KB, patch)
2016-05-25 07:08 UTC, Olivier Fourdan
none Details | Review
[PATCH 4/4] menu: translate fake coords for positioning (10.55 KB, patch)
2016-05-25 07:09 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4 v2] gtkprivate: Add gtk_translate_fake_coordinates() (3.05 KB, patch)
2016-05-25 07:16 UTC, Olivier Fourdan
none Details | Review
[PATCH 3/4 v3] gtkprivate: Add gtk_translate_fake_coordinates() (3.11 KB, patch)
2016-05-25 07:24 UTC, Olivier Fourdan
none Details | Review
[PATCH 1/4 v2] display: Add vfunc for has_global_coords() (2.69 KB, patch)
2016-05-25 07:48 UTC, Olivier Fourdan
none Details | Review

Description Olivier Fourdan 2016-05-20 15:03:49 UTC
Description:

The fix for bug 766566 introduced a discrepancy for Wayland, because gtk+ compares the size and position of 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 relative to the toplevel surface), and those values that may not match in a multi monitor layout now that get_monitor_at_window() returns the right monitor.

How reproducible:

100%

Steps to reproduce:

1. Build gtk+
2. Run "gtk3-demo --run combobox" on a multi-monitor setup
3. Open combo box menus.

Actual results:

The menu is placed at a screen edge

Expected results:

The menu is placed next to the combo box

Additional info:

One possibility would be to use gdk_display_get_monitor_at_point() every time to have consistent (either right or wrong) results on Wayland as well as other backends.

Proposed patch to follow, mostly an RFC. We can as well wait for the fixes for  bug 756579 to land.
Comment 1 Olivier Fourdan 2016-05-20 15:06:43 UTC
Created attachment 328265 [details] [review]
[PATCH RFC] wayland: use get_monitor_at_point() consistently

Mixing get_monitor_at_window() and get_monitor_at_point() cannot work
on Wayland because the latter uses global coordinates.

Use get_monitor_at_point() everywhere to avoid possible discrepancies.
Comment 2 Matthias Clasen 2016-05-24 11:23:04 UTC
Review of attachment 328265 [details] [review]:

I'm not convinced that this is really right - the semantics of monitor_at_window and monitor_at_point are not inconsistent, and at_point has even less of a chance to work without global coordinates
Comment 3 Olivier Fourdan 2016-05-24 11:46:26 UTC
(In reply to Matthias Clasen from comment #2)
> Review of attachment 328265 [details] [review] [review]:
> 
> I'm not convinced that this is really right - the semantics of
> monitor_at_window and monitor_at_point are not inconsistent, and at_point
> has even less of a chance to work without global coordinates

The patch is right in the sense that it makes the result consistently wrong just as before bug 766566.

And I completely agree, this patch was mostly an RFC and a placeholder for people who would complain about menu placement under multi-screen in Wayland using gtk+ from master.

The best solution is to wait for the fixes for bug 756579 to land.
Comment 4 Olivier Fourdan 2016-05-24 11:49:30 UTC
Note, if we want to improve things (and actually get consistent but also better placement under Wayland) until we get menu placement in the gdk backend, we could as well only use get_monitor_at_window() for menu constraints. A bit of rework but I can give this a try if it's worth it.
Comment 5 Olivier Fourdan 2016-05-25 07:00:55 UTC
Ok, here's the idea, when we deal with a backend that doesn't have global coordinates, all coordinates are relative to the toplevel, which breaks the monitor detection. Basically (0,0) is the toplevel top left corner and not the screen top left corner.

So the idea is to translate the coordinates to the relevant monitor, by finding the monitor based on the window (thanks to bug 766566) and add the monitor location to the given (relative) coordinates, to fix the computation to constrain menus within the current monitor, then translate it back to local coordinates once the computation is done.

For this, we'd need to:

1. Add an API to tell whether or not the current display (and backend) supports global coordinates.,so we do not need to check which backend is actually in use - I think it could even be a worthy addition anyway.

2. Add a private helper function to gtkprivate to do the coordinates translation back and forth when the backend doesn't support global coordinates.

3. Use that in various menu positioning and sizing routines.

From my initial testing here, that seems to work well enough, it fixes this bug and bug 764310 as well.

That could be a good addition while we wait for the fixes for bug 756579  to land, and we could even consider this as a fix for gtk+-3.20 where xdg-shell v6 is unlikely to be backported, who knows.

Patch series to follow.
Comment 6 Olivier Fourdan 2016-05-25 07:06:30 UTC
Created attachment 328469 [details] [review]
[PATCH 1/4] display: Add vfunc for has_global_coords()

Not all gdk backends support global coordinates (e.g. Wayland or Mir do
not) but applications have no way to tell if it's supported or not,
other than by checking the underlying windowing system and backend used.

Add a vfunc to the gdkdisplay so that each backend can optionally
declare itself supporting or not global coordinates.
Comment 7 Olivier Fourdan 2016-05-25 07:07:22 UTC
Created attachment 328470 [details] [review]
[PATCH 2/4] wayland: Add has_global_coords() hook in Wayland

Implement hook for Wayland backend.
Comment 8 Olivier Fourdan 2016-05-25 07:08:52 UTC
Created attachment 328471 [details] [review]
[PATCH 3/4] gtkprivate: Add gtk_translate_fake_coordinates()

Add a convenient private function to help translating to/from fake
global coordinates where the display doesn't support global coordinates.

This function translates the given coordinates by adding or subtracting
the current monitor position where the window resides, so that these
can be compared against the monitor position and size.
Comment 9 Olivier Fourdan 2016-05-25 07:09:34 UTC
Created attachment 328472 [details] [review]
[PATCH 4/4] menu: translate fake coords for positioning

Use the newly added private API to translate back and forth from fake
root coordinates when comparing against the monitor's position and
size.
Comment 10 Olivier Fourdan 2016-05-25 07:16:48 UTC
Created attachment 328474 [details] [review]
[PATCH 3/4 v2] gtkprivate: Add gtk_translate_fake_coordinates()

Add a convenient private function to help translating to/from fake
global coordinates where the display doesn't support global coordinates.

This function translates the given coordinates by adding or subtracting
the current monitor position where the window resides, so that these
can be compared against the monitor position and size.

---
 v2: Remove the GDK_WAYLAND test/inclusion, we don't need to test actual
     backend
Comment 11 Olivier Fourdan 2016-05-25 07:24:43 UTC
Created attachment 328475 [details] [review]
[PATCH 3/4 v3] gtkprivate: Add gtk_translate_fake_coordinates()

Add a convenient private function to help translating to/from fake
global coordinates where the display doesn't support global coordinates.

This function translates the given coordinates by adding or subtracting
the current monitor position where the window resides, so that these
can be compared against the monitor position and size.

---
 v2: Remove the GDK_WAYLAND test/inclusion, we don't need to test actual
     backend
 v3: No need to look for the monitor if we support global coords.
Comment 12 Olivier Fourdan 2016-05-25 07:48:01 UTC
Created attachment 328476 [details] [review]
[PATCH 1/4 v2] display: Add vfunc for has_global_coords()

Not all gdk backends support global coordinates (e.g. Wayland or Mir do
not) but applications have no way to tell if it's supported or not,
other than by checking the underlying windowing system and backend used.

Add a vfunc to the gdkdisplay so that each backend can optionally
declare itself supporting or not global coordinates.

---
 v2: Fix function name (_coord instead of _coordinates) in documentation 
     header.
Comment 13 Olivier Fourdan 2016-05-25 10:00:28 UTC
FWIW, I have the patches from bug 766566 and this bug 766722 backported to gtk-3-20 (except the API addition of course, so we don't break compat) and it seems to work well and greatly improves menu positioning under Wayland with multi-monitor setup (basically, it fixes bug 764310)
Comment 14 Matthias Clasen 2016-05-30 22:08:50 UTC
I'm sorry to say that this looks wrong to me.

If we can have 'fake coordinates', why not make those the real coordinates inside gdk and avoid patching 30 places in gtk to manually discriminate between fake and real coordinates ?!
Comment 15 Olivier Fourdan 2016-05-31 04:58:11 UTC
(In reply to Matthias Clasen from comment #14)
> I'm sorry to say that this looks wrong to me.
> 
> If we can have 'fake coordinates', why not make those the real coordinates
> inside gdk and avoid patching 30 places in gtk to manually discriminate
> between fake and real coordinates ?!

Simply because fake coordinates are relative to the toplevel, there is no global coordinates in Wayland.
Comment 16 Olivier Fourdan 2016-05-31 06:15:40 UTC
So to make it clear, there is no global coordinates in Wayland, so you just cannot make real coordinates inside gdk, there is no way a client can know it's global position on the screen in Wayland, therefore there is no way for gdk_display_get_monitor_at_point() to work.

Considering that gdk_display_get_monitor_at_window() works though, the idea was to translate the fake coordinate in the appropriate monitor so that gdk_display_get_monitor_at_point() returns the correct monitor and menus can be placed and sized correctly.

Another solution is to use the new xdg_positioner, for declarative positioning of child surfaces. AFAIK, this work is being done in bug 756579 and will be discussed at the hackfest in Toronto (as Jonas indicated).

Meanwhile these patches here fix the problem without requiring the new protocol, so it may come useful either in master until the patches from bug 756579 eventually land in master, or in gtk-3-20 where the new protocol and xdg-shell v6 might not be backported.

Anyway, I am not pushing either way, if someone complains about the menu being misplaced in Wayland (as in bug 764310) these patches here can help.

I am fine with closing this bug as a dupe of bug 756579 if you prefer.
Comment 17 Timm Bäder 2016-09-03 11:20:17 UTC
Is this still a problem with master and the menu positioning in gdk?
Comment 18 Olivier Fourdan 2016-09-28 07:41:14 UTC
Nope, as expected, xdg-shell v6 and the new menu positioning fixed this.

*** This bug has been marked as a duplicate of bug 756579 ***