GNOME Bugzilla – Bug 766722
Wayland: gtkcombox menu misplaced in master
Last modified: 2016-09-28 07:41:14 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.
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.
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
(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.
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.
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.
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.
Created attachment 328470 [details] [review] [PATCH 2/4] wayland: Add has_global_coords() hook in Wayland Implement hook for Wayland backend.
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.
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.
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
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.
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.
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)
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 ?!
(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.
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.
Is this still a problem with master and the menu positioning in gdk?
Nope, as expected, xdg-shell v6 and the new menu positioning fixed this. *** This bug has been marked as a duplicate of bug 756579 ***