GNOME Bugzilla – Bug 782213
xdg-shell client's window geometry can be wrong
Last modified: 2017-05-10 08:53:50 UTC
When a xdg-shell client doesn't set the window geometry explicitly (xdg_surface.set_window_geometry) mutter doesn't calculate a valid geometry for it. According to the xdg-shell-unstable-v6 spec [0], set_window_geometry "If never set, the value is the full bounds of the surface". This bug can be reproduced very easily by launching an xdg-shell client that doesn't call set_window_geometry (like weston-simple-egl or weston-simple-shm for example). Here are some screenshots showing this bug: https://i.imgur.com/qps0PM5.png https://i.imgur.com/AFyAG8c.png 0: https://cgit.freedesktop.org/wayland/wayland-protocols/tree/unstable/xdg-shell/xdg-shell-unstable-v6.xml#n449
Created attachment 351187 [details] Mutter log
Created attachment 351190 [details] [review] [PATCH] wayland: Make sure we have a pending geometry If the client doesn't set a geometry using xdg_shell, we'll compute its geometry based on its surface and subsurfaces. Yet, we translate that as a window (re)size only when there is a pending geometry, that we don't have when we computed the geometry by ourself. Make sure we set the pending new geometry flag when computing the geometry.
(In reply to Olivier Fourdan from comment #2) > Created attachment 351190 [details] [review] [review] > [PATCH] wayland: Make sure we have a pending geometry > > If the client doesn't set a geometry using xdg_shell, we'll compute its > geometry based on its surface and subsurfaces. > > Yet, we translate that as a window (re)size only when there is a pending > geometry, that we don't have when we computed the geometry by ourself. > > Make sure we set the pending new geometry flag when computing the > geometry. Works fine. Push it (and backport it to 3.24, please)!
Thanks for the feedback, however "working fine" is not sufficient as a criteria for landing a patch, I'd rather see a review of the patch itself (even though it looks trivial).
Review of attachment 351190 [details] [review]: ::: src/wayland/meta-wayland-xdg-shell.c @@ +1280,3 @@ &priv->geometry, 0, 0); + pending->has_new_geometry = TRUE; This means that for every commit on a surface that doesn't ever set the geometry we'll go through the move/resize code, which might be a bit costly, so I'd rather see we only set this state if priv->geometry actually changed (or the size only really). BTW, I spot a bug here. In this case here, it looks like the geometry can never ever shrink, only become larger, as calculate_window_geometry() takes geometry passed, and unions it with the current state. If the surface at first is 100x100, then resized to 50x50, the union of 50x50 and 100x100 will still be 100x100. So we should actually pass an empty rectangle to calculate_window_geometry(). That'll be convienient, since we can then compare that with priv->geometry and determine whether to set has_new_geometry or not.
BTW, another bug I spotted when reviewing this: if we set the min/max size while not also setting a new geometry, we'll ignore the min/max in xdg_toplevel_commit(). So, maybe the early out should be changed to if (pending->has_new_geometry) { move_resize(...); }
Created attachment 351509 [details] [review] [PATCH 1/2] wayland: Make sure we have a pending geometry If the client doesn't set a geometry using xdg_shell, we'll compute its geometry based on its surface and subsurfaces. Yet, we translate that as a window (re)size only when there is a pending geometry, that we don't have when we computed the geometry by ourself. Make sure we set the pending new geometry flag when computing the geometry when it actually changed.
Created attachment 351511 [details] [review] [PATCH 2/2] wayland: Apply size hints regardless of geometry Previously we would bail out early in xdg_toplevel_role_commit() if no geometry change was set, ignoring the possible min/max size hints changes. But setting a min/max size hint without changing the geometry is perfectly valid, so we ought to apply the min/max changes regardless of a geometry change.
Review of attachment 351509 [details] [review]: With the style nit fixed, this lgtm. ::: src/wayland/meta-wayland-xdg-shell.c @@ +1284,2 @@ 0, 0); + if (!meta_rectangle_equal(&new_geometry, &priv->geometry)) style nit: space between _equal and (
Review of attachment 351511 [details] [review]: ::: src/wayland/meta-wayland-xdg-shell.c @@ +638,1 @@ return; Don't think we should return here; we'll still ignore the new min/max. The comment about dx/dy is that we don't handle the resize, which is handled in the other if branch already, so no need to early out.
Created attachment 351513 [details] [review] [PATCH 2/2 v2] wayland: Apply size hints regardless of geometry v2: don't return
Review of attachment 351513 [details] [review]: lgtm.
Thanks for the reviews! I guess those are fine to backport in gnome-3-22 branch as well, right?
(In reply to Olivier Fourdan from comment #13) > Thanks for the reviews! > > I guess those are fine to backport in gnome-3-22 branch as well, right? Sounds reasonable.
Comment on attachment 351509 [details] [review] [PATCH 1/2] wayland: Make sure we have a pending geometry attachment 351509 [details] [review] pushed to git master as commit 410d66c - wayland: Make sure we have a pending geometry attachment 351509 [details] [review] pushed to branch gnome-3-22 as commit ca31a94 - wayland: Make sure we have a pending geometry
Comment on attachment 351513 [details] [review] [PATCH 2/2 v2] wayland: Apply size hints regardless of geometry attachment 351513 [details] [review] pushed to git master as commit f241bdb - wayland: Apply size hints regardless of geometry attachment 351513 [details] [review] pushed to branch gnome-3-22 as commit 40a3d67 - wayland: Apply size hints regardless of geometry