GNOME Bugzilla – Bug 771841
[Wayland] Drop-down menus are broken in position and size on HiDPI screens
Last modified: 2016-10-14 16:11:11 UTC
Created attachment 336090 [details] Screencast showing the strange behaviour [Please, reassign to the right component in charge for this, I'm probably mistaking it] Attached, you'll find a screencast I made to explain visually the issue encountered. The same steps followed under Xorg don't show any kind of that weirdness. Seems like this happens only on HiDPI screens. Cheers.
Created attachment 336140 [details] [review] wayland/xdg-shell: Scale positioner coordinates When the monitor is scaled (i.e. HiDPI scaling) the placement coordinates are still in unscaled xdg_surface window geometry coordinate space. Fix this by simply scaling the coordinates by the monitor scale of the parent toplevel window. This is inherently racy, but since we won't move the toplevel window before the popup is placed, and we won't move the window without unmapping the popup, there is little point in introducing more complex adaptive scaling, especially when the end goal is to get rid of all these types of scaling hacks.
Created attachment 336142 [details] With patch Works for me, but if possible, look at the oversize in the attached screenshot. This is in any case much better than what we have :-) so stil +1 from me
(In reply to Bjørn Lie from comment #2) > Created attachment 336142 [details] > With patch > > Works for me, but if possible, look at the oversize in the attached > screenshot. > > This is in any case much better than what we have :-) so stil +1 from me Ah, some more coordinates that need the scale hack probably.
This fixes the issue for me on my hidpi screen, however when using mixed setup (hidpi laptop + normal dpi monitor) the issue still occurs on the non-hidpi screen.
Created attachment 336145 [details] [review] wayland/xdg-popup: Always use monitor of toplevel Always use the monitor of the toplevel surface's window, so that the popup menu and the parent will always have the same scale. This fixes the dimensions sent in the xdg_popup configure event.
Created attachment 336146 [details] [review] wayland/xdg-shell: Scale configure relative popup coordinate The parent local popup coordinate needs to be scaled according to the monitor scale it is assigned.
Seems that even with these two, the configure on the non-hdpi screen still gets a width/height half of the requested one
(In reply to Sjoerd Simons from comment #7) > Seems that even with these two, the configure on the non-hdpi screen still > gets a width/height half of the requested one The configure event of the pop-up? I can't reproduce that. Could you send WAYLAND_DEBUG=1 log?
Created attachment 336172 [details] [review] wayland/xdg-popup: Force monitor of the top-level Directly set the monitor of the toplevel window for the popup to avoid the change not being applied due to later constraints calculation. Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
Created attachment 336173 [details] [review] wayland/xdg-shell: update popup window monitor early As meta_window_place_with_placement_rule will trigger a configure event being sent ensure that the popup is placed on the correct monitor first to ensure the right scale factor is applied. Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
With the last 2 patches on top of those from Jonas the issue is fixed for me. The reason why i was apparently still hitting it is that the configure event is triggered by meta_window_place_with_placement_rule in finish_popup_setup which was before the popup monitor was synced with that of the toplevel, hence the configure event using the wrong monitors scale. Would be nice if in the long-term mutter kept the buffer coordinates (pixels) and surface coordinates seperate so all this fragile scaling can go away :)
(In reply to Sjoerd Simons from comment #11) > With the last 2 patches on top of those from Jonas the issue is fixed for > me. > > The reason why i was apparently still hitting it is that the configure event > is triggered by meta_window_place_with_placement_rule in finish_popup_setup > which was before the popup monitor was synced with that of the toplevel, > hence the configure event using the wrong monitors scale. > > > Would be nice if in the long-term mutter kept the buffer coordinates > (pixels) and surface coordinates seperate so all this fragile scaling can go > away :) The long term plan is to have the stage coordinate space to be in logical pixels, meaning that indeed all these hacks will go away.
Great to hear. FWIW i reproduce the issue by running mutter with: MUTTER_DEBUG_NUM_DUMMY_MONITORS=2 MUTTER_DEBUG_DUMMY_MONITOR_SCALES=2,1 iotw dual monitor setup, left (primary) monitor hidpi. The issue can be seen by starting e.g. gtk3-demo and moving it to the right monitor before opening a popup.
Review of attachment 336172 [details] [review]: A popup can potentially change scale: if its not a grabbing popup, and the parent window is moved, the popup should move with the parent surface. With that said, we don't handle this situation yet (since we don't have a non-grabbing popup used by a client yet) so it probably wouldn't work to begin with. With that said, why is this patch needed? from == to should always hold (except for the initial set, which is handled) in the use cases we currently have.
Review of attachment 336173 [details] [review]: Looks reasonable to me.
Review of attachment 336172 [details] [review]: The initial case block doesn't actually happen as _meta_window_shared_new will set the initial monitor via meta_screen_calculate_monitor_for_window, which means from is never NULL. So the first round through this function actually has from != to. The problem this patch avoids is not syncing the popup monitor to the top-level monitor as the scaled_new monitor doesn't match the top-level monitor.
Review of attachment 336173 [details] [review]: fwiw i don't have commit access on git.gnome anymore, so please commit for me ;)
(In reply to Sjoerd Simons from comment #17) > Review of attachment 336173 [details] [review] [review]: > > fwiw i don't have commit access on git.gnome anymore, so please commit for > me ;) OT, probably https://www.dragonsreach.it/2014/10/07/the-gnome-infrastructure-is-now-powered-by-freeipa/ has instructions that may help you.
(In reply to Sjoerd Simons from comment #16) > Review of attachment 336172 [details] [review] [review]: > > The initial case block doesn't actually happen as _meta_window_shared_new > will set the initial monitor via meta_screen_calculate_monitor_for_window, > which means from is never NULL. So the first round through this function > actually has from != to. The problem this patch avoids is not syncing the > popup monitor to the top-level monitor as the scaled_new monitor doesn't > match the top-level monitor. Ah, I see. I guess we can go with that approach then, and fix it properly after I've reworked how MetaWindow's are constructed.
(In reply to Jonas Ådahl from comment #19) > (In reply to Sjoerd Simons from comment #16) > > Review of attachment 336172 [details] [review] [review] [review]: > > > > The initial case block doesn't actually happen as _meta_window_shared_new > > will set the initial monitor via meta_screen_calculate_monitor_for_window, > > which means from is never NULL. So the first round through this function > > actually has from != to. The problem this patch avoids is not syncing the > > popup monitor to the top-level monitor as the scaled_new monitor doesn't > > match the top-level monitor. > > Ah, I see. I guess we can go with that approach then, and fix it properly > after I've reworked how MetaWindow's are constructed. Could you commit these in that case ;). Would be great to have that sorted in git
I installed a built with attachment 336172 [details] [review], and for me it sadly doesn't work for me. Any news i missed?
(In reply to Daniel Buch from comment #21) > I installed a built with attachment 336172 [details] [review] [review], and for me it > sadly doesn't work for me. Any news i missed? Did you build with all the other related patches? They seem to be required: (In reply to Sjoerd Simons from comment #11) > With the last 2 patches on top of those from Jonas the issue is fixed for > me.
(In reply to djb from comment #22) > (In reply to Daniel Buch from comment #21) > > I installed a built with attachment 336172 [details] [review] [review] [review], and for me it > > sadly doesn't work for me. Any news i missed? > > Did you build with all the other related patches? They seem to be required: > > (In reply to Sjoerd Simons from comment #11) > > With the last 2 patches on top of those from Jonas the issue is fixed for > > me. I managed to apply all attached patches. But problem still persists. This thread is now moved to Gtk so i guess the problem lies there and not in mutter.
Daniel can you indicate how exactly you rpreoduce the issue and what your setup is, there might be some subtilities that these patches haven't covere ofcourse.
Created attachment 337452 [details] Daniel setup info
Created attachment 337453 [details] GtkMenu-Click-1 This is when i click once, next will be shrink further by factor two. Next attachment will show that
Created attachment 337454 [details] GtkMenu-Click-2
Created attachment 337455 [details] Popup positioning This shows that popup position (x,y) is _NOT_ multiplied by scaling factor. Look at cursor and popup
This behavior persist even though i patch these mutter patches. As a last comment, i discovered that the shrinking problem, is widget instance-wise. So clicking one button and the popup shrink to half the size exponentially for each click. From my understanding, i believe this logic is now in gdk, not gtk and not mutter - is that correct?
Eventually the program will crash with this: (gnome-calculator:9872): Gtk-WARNING **: Negative content width -7 (allocation 1, extents 4x4) while allocating gadget (node arrow, owner GtkTreeMenu) (gnome-calculator:9872): Gdk-WARNING **: Error 71 (Protocol error) dispatching to Wayland display.
Daniel i meant what gtk+ version, what mutter version did you apply the patches on what's your screen setup (one hidpi, other normal dpi? or?) The patches from Jonas are enough to solve the issue for a single hidpi screen, mine are needed on top to fix the issue for mixed dpi setups.
Perfect, i managed to make it work with mutter 3.22.1 and gtk 3.22.1 including these patches.
Created attachment 337556 [details] [review] wayland/xdg-shell: Scale positioner coordinates When the monitor is scaled (i.e. HiDPI scaling) the placement coordinates ere still in unscaled xdg_surface window geometry coordinate space when used to place the window. Fix this by scaling the coordinates by the monitor scale of the parent toplevel window before using them. ----- Rewrote the first patch to scale the coordinates as a more sane place. Also added a comment documenting the coordinate space of the stored placement rule fields.
Review of attachment 336172 [details] [review]: Marking as a-c-n. Ihis patch doesn't stop the re-scaling would the parent window move, since the scaling that is left out is the not-to-be-run "avoid jumping-back-and-forth-between-scales" scaling, so looks good to me.
Review of attachment 337556 [details] [review]: Looks good to me, tested the set with this patch as opposed to its previous iteration and everything seems happy as expected
Review of attachment 337556 [details] [review]: Tested on mutter-3.22.1 on arch 64, works well.
Attachment 336145 [details] pushed as c0c132a - wayland/xdg-popup: Always use monitor of toplevel Attachment 336146 [details] pushed as a3d7ae6 - wayland/xdg-shell: Scale configure relative popup coordinate Attachment 336172 [details] pushed as 8a6fa72 - wayland/xdg-popup: Force monitor of the top-level Attachment 336173 [details] pushed as d2f79af - wayland/xdg-shell: update popup window monitor early Attachment 337556 [details] pushed as 68645df - wayland/xdg-shell: Scale positioner coordinates