GNOME Bugzilla – Bug 783901
Restoring maximized window state in Wayland results in tiny window
Last modified: 2018-01-11 10:10:33 UTC
Tilix restores the previous window state similar to gedit, however under Wayland this restoring the maximized window results in a tiny window and the following error message: Gtk-CRITICAL **: gtk_box_gadget_distribute: assertion 'size >= 0' failed in GtkNotebook Testing gedit shows the exact same issue but a slightly different error message: Gtk-WARNING **: Negative content width -6 (allocation 18, extents 12x12) while allocating gadget (node label, owner GtkLabel) To reproduce the issue with gedit: 1. Open gedit 2. Maximize it 3. While maximized close the gedit window 4. Start gedit again, starts maximized 5. Double-click the headerbar to restore it 6. Window is tiny and error message appears in console
Odd, in works fine with gedit and mutter mutter-3.22.4 here, using the steps in comment 0
Olivier Fourdan: It only happens for me if the window was automaximized when opening. So, exact steps to reproduce: 1. Open terminal, run gedit It starts unmaximized in normal size 2. Maximize it and close it 3. Run gedit again 4. Unmaximize it.
Oops, I've actually said exactly same thing as in comment 0.
Yes, I fail to reproduce that.
Hm. Maybe it was fixed in master. I'm on Fedora 26 with mutter 3.24.2-2.fc26.x86_64 and gtk3-3.22.15-2.fc26.x86_64 However, IIRC it didn't happen in a guest system in Boxes. I'll check it again.
I'm on Arch Linux and have the following versions installed: mutter 3.24.2+18+g0f7c3f367-1 gtk3 3.22.15-1 The original tilix issue is here in case it helps: https://github.com/gnunn1/tilix/issues/969 The fellow who reported this originally was also on Arch.
Ok, so I've checked it in Boxes and on a live system, and it actually seems to be a mutter thing rather than a gtk3 one: it works fine with mutter-3.24.2-1.fc26.x86_64, but breaks after updating to 3.24.2-2.fc26.x86_64, even if I update only that package and nothing more. I wonder what could be different between these versions.
Ok, so the difference is that the newer package updates to a newer commit in gnome-3-24 branch. (e22c75377b4bb0be0bc21ed7a1f2f017672edbfe, was 01b6e32e8796a96298ea388a557ab0c1df329d37) So, is it a recent regression in Mutter?
Oh, you're speaking rpm packages there :) Well, the difference between mutter-3.24.2-1.fc26 and mutter-3.24.2-2.fc26 is the addition of a patch that brings the code to commit e22c753 in branch gnome-3-24. Which means you have commit 7801df7 in the 3.24.2-2 build
Umm, yes. So, can you reproduce it with the commit you linked?
Yes, I'm pretty sure it's commit 7801df7 that broke it.
Yes, it is. I've rebuilt the package with that line commented out in the patch, and it works. :)
Well, the reason is pretty obvious, calling meta_window_force_placement() from xdg_toplevel_set_maximized() will set "window->placed" to TRUE prior to call meta_window_maximize() and thus we shall not return early: https://git.gnome.org/browse/mutter/tree/src/core/window.c#n2729 As a result, the "old" geometry is saved with a zero size... But reverting commit 7801df7 will re-introduce bug 782183 and bug 781353 so we should fix those differently, so we ought to find a different fix.
(In reply to Olivier Fourdan from comment #13) > Well, the reason is pretty obvious, calling meta_window_force_placement() > from xdg_toplevel_set_maximized() will set "window->placed" to TRUE prior to > call meta_window_maximize() and thus we shall not return early: > > https://git.gnome.org/browse/mutter/tree/src/core/window.c#n2729 > > As a result, the "old" geometry is saved with a zero size... > > But reverting commit 7801df7 will re-introduce bug 782183 and bug 781353 so > we should fix those differently, so we ought to find a different fix. What size is sent on unmaximize? If it's anything other than 0x0, I think we should change it so 0x0 is sent. We probably unmaximize to some calculated "smart" fallback size, and that should probably be disabled on Wayland as the client has a better idea of what to unmaximize to.
Right, indeed, mutter tries to use a smart fallback whic is wrong on Wayland. But using 0x0 on unmaximize results in the client resizing itself to the same size as when it was maximized, which makes sense because it was started maximized. If we reckon that's better, I have the patch...
Well, that depends on the client, totem seems to behave better than gedit in this regard (so it's already an improvement) and resize to some sensible smaller size.
Created attachment 354146 [details] [review] [PATCH 1/3] core: Add new wayland unmaximize flag Wayland clients know their size better, so for Wayland we'd rather not try to resize the client on un-maximize, but for this to work we need a new MetaMoveResizeFlags.
Created attachment 354147 [details] [review] [PATCH 2/3] wayland: Do not enforce a size on un-maximize When un-maximizing, use a zero size to pass to the client so that it can use the right un-maximized size that fits.
Created attachment 354148 [details] [review] [PATCH 3/3] wayland: update location prior to maximize When maximizing a window, the previous location is saved so that un-maximize would restore the same original window location. However, if a Wayland client starts with a window maximized, the previous location will be 0x0, so if we have to force placement in xdg_toplevel_set_maximized(), we should update the location as well so that the window is placed on the right monitor when un-maximizing.
FWIW it still looks like a hack, doesn't seem right... But it improves things.
(In reply to Olivier Fourdan from comment #20) > FWIW it still looks like a hack, doesn't seem right... But it improves > things. Yet another reason for (in the future) reworking window state tracking more thoroughly. Anyhow, I think this is reasonable as solution for now, but I wonder if we can't deal with the placement on unmaximize, by knowing that we don't have a saved rect to restore to, instead of coming up with one now. I mean, things might have changed while the window was maximized, and it makes more sense to calculate a "good" position close to when its to be positioned, not when it was initially maximized.
Yes, I would agree, but it's a bit difficult to do that in the Wayland backend (i.e. not in core) because meta_window_unmaximize() restores the old size/position and also does the meta_window_move_resize_internal(). Besides, the size which would be needed to compute the new placement is only known when the clients has updated it following the xdg_surface.configure with size 0x0
(In reply to Olivier Fourdan from comment #22) > Yes, I would agree, but it's a bit difficult to do that in the Wayland > backend (i.e. not in core) because meta_window_unmaximize() restores the old > size/position and also does the meta_window_move_resize_internal(). > > Besides, the size which would be needed to compute the new placement is only > known when the clients has updated it following the xdg_surface.configure > with size 0x0 True. The placement would have to be done at the unmaximize ack_configure commit when the size is known. I guess one problem with the current patches is that the placement will assume the window is 1x1 (right?), so the saved position might be very bad. For example, if there is a small space to the right of screen, mutter would be clever and position the window over here since it'll fit just fine. But then when actually showing the unmaximized window, it'll be positioned mostly outside of the screen. (disclaimer, I haven't checked if this is the case).
Should we instead change the "cleverly computer saved rect" to be clever about computing the position, but omit computing the size, when a maximize is for a Wayland window?
(In reply to Jonas Ådahl from comment #24) > Should we instead change the "cleverly computer saved rect" to be clever > about computing the position, but omit computing the size, when a maximize > is for a Wayland window? That's a good idea, worth a try.
Humm, is there such a thing as a "cleverly computer saved rect"? From what I can see, it's just a saved rect, i.e. the size before maximization (as saved in meta_window_maximize()). In meta_window_unmaximize(), the saved rect is restored, it can be shrunk to be less than 80% of the workarea though. But the point is, the saved rect is not cleverly computed anywhere. Besides, it's difficult to compute the position right without having the size being set, and the problem is precisely that when we get to xdg_toplevel_set_maximized() the window size is not known and the placement is not done yet. Prior to commit 561d71b, that wasn't a problem because the window not being placed yet would have trigger the fallback code path in meta_window_maximize() to simply set "maximize_*_after_placement" and return, without actually saving neither the size nor position. https://git.gnome.org/browse/mutter/tree/src/core/window.c#n2729 With commit 561d71b, we make sure the window is (forcibly) placed in xdg_toplevel_set_maximized(), meaning that the placement is done based on the wrong (unknown) size, and the size is also saved in meta_window_maximize() as window->placed is not set, thus avoiding the fallback path as before. So, the real appropriate fix would be to fix bug 782183 and bug 781353 without forcing the placement. IMHO the real issue with bug 782183 and bug 781353 is that we show the window too "early", ie meta_window_show() gets called from idle_calc_showing() *before* the window is actually placed and maximized, leading to the ugly wrongly placed animation. I would rather fix this properly rather than adding even more "fixes" to placement as this may cause some other subtile breackage elsewhere... I just fail to understand why we end up calling meta_window_show() from idle_calc_showing() in this particular case...
(In reply to Olivier Fourdan from comment #26) > [...] > meta_window_maximize() as window->placed is not set, thus avoiding the > fallback path as before. > [...] Sorry that should read as "window->placed is _now_ set, thus avoiding the fallback path as before"
(In reply to Olivier Fourdan from comment #26) > In meta_window_unmaximize(), the saved rect is restored, it can be shrunk to be > less than 80% of the workarea though. Isn't this the way we get the unmaximize size? Or where else do we get the "incorrect" unmaximized size? > I just fail to understand why we end up calling meta_window_show() from > idle_calc_showing() in this particular case... IIRC the problem was that the client didn't get the xdg_toplevel.configure(maximized, width, height) but a xdg_toplevel.configure(normal, 0, 0) causing it to show up non-maximized, and when we ended up showing (because well it was "ready") we "maximized-after-placed", and it took a while for the client to provide the actual maximized buffer. If we can fix the original problem causing this regression messing less with forcing placement and what not, I agree that it would probably be better.
Review of attachment 354146 [details] [review]: ::: src/core/window-private.h @@ +80,3 @@ + META_MOVE_RESIZE_WAYLAND_RESIZE = 1 << 4, + META_MOVE_RESIZE_STATE_CHANGED = 1 << 5, + META_MOVE_RESIZE_WAYLAND_UNMAXIMIZE = 1 << 6, Could take the opportunity and unalign these, so we don't have to relalign next time we add anything ::: src/core/window.c @@ +3093,3 @@ META_MOVE_RESIZE_RESIZE_ACTION | + META_MOVE_RESIZE_STATE_CHANGED | + META_MOVE_RESIZE_WAYLAND_UNMAXIMIZE), This doesn't look Wayland related, why not just call it META_MOVE_RESIZE_UNMAXIMIZE?
Review of attachment 354147 [details] [review]: Makes sense.
Review of attachment 354148 [details] [review]: ::: src/wayland/meta-wayland-xdg-shell.c @@ +355,3 @@ + /* Make sure the position is up-to-date prior to call maximize */ + window->rect.x = window->unconstrained_rect.x; + window->rect.y = window->unconstrained_rect.y; This looks like its working around something in force_placement(). Shouldn't we fix this in meta_window_wayland_move_resize_internal()? It looks like we should be able to set "can_move_now" to TRUE if we are "!placed" (which will be set to TRUE after the move resize call), or possibly adding another MOVE_RESIZE flag "force" that forces the placement?
(In reply to Jonas Ådahl from comment #31) > Review of attachment 354148 [details] [review] [review]: > > ::: src/wayland/meta-wayland-xdg-shell.c > @@ +355,3 @@ > + /* Make sure the position is up-to-date prior to call maximize */ > + window->rect.x = window->unconstrained_rect.x; > + window->rect.y = window->unconstrained_rect.y; > > This looks like its working around something in force_placement(). Well, it;s been a while since I wrote this patch, but I reckon this is not, actually. > Shouldn't we fix this in meta_window_wayland_move_resize_internal()? It > looks like we should be able to set "can_move_now" to TRUE if we are > "!placed" (which will be set to TRUE after the move resize call), or > possibly adding another MOVE_RESIZE flag "force" that forces the placement? That wouldn't be the same as saving the x/y when maximizing from xdg_toplevel_set_maximized(), trying to address the issue in meta_window_wayland_move_resize_internal() would either place the window initially at 0/0 (thus defeating initial placement), or restore the location on the wrong output, or play the initial mapping animation on the wrong output (depending on which ig/then/else case we force "can_move_now" to TRUE) - Well at least from what I saw when trying to experimenting with that, but I could be wrong.
(In reply to Olivier Fourdan from comment #32) > (In reply to Jonas Ådahl from comment #31) > > Review of attachment 354148 [details] [review] [review] [review]: > > > > ::: src/wayland/meta-wayland-xdg-shell.c > > @@ +355,3 @@ > > + /* Make sure the position is up-to-date prior to call maximize */ > > + window->rect.x = window->unconstrained_rect.x; > > + window->rect.y = window->unconstrained_rect.y; > > > > This looks like its working around something in force_placement(). > > Well, it;s been a while since I wrote this patch, but I reckon this is not, > actually. The reason why I think it looks like this is that meta_window_force_placement() will call meta_window_wayland_move_resize_internal() which conditionally will set the window->rect.x/y. It seems to be that the condition should cover also this case. > > > Shouldn't we fix this in meta_window_wayland_move_resize_internal()? It > > looks like we should be able to set "can_move_now" to TRUE if we are > > "!placed" (which will be set to TRUE after the move resize call), or > > possibly adding another MOVE_RESIZE flag "force" that forces the placement? > > That wouldn't be the same as saving the x/y when maximizing from > xdg_toplevel_set_maximized(), trying to address the issue in > meta_window_wayland_move_resize_internal() would either place the window > initially at 0/0 (thus defeating initial placement), or restore the location > on the wrong output, or play the initial mapping animation on the wrong > output (depending on which ig/then/else case we force "can_move_now" to > TRUE) - Well at least from what I saw when trying to experimenting with > that, but I could be wrong. How so? What difference would it make to cause the wayland_move_resize_internal() to save the rect.x/y compared to doing it outside? I'd only ever immediately save the position with the exact same conditions as in the attached patch, i.e. when not already placed before maximizing (by adding the MOVE_RESIZE_FORCE flag).
Created attachment 366609 [details] [review] [PATCH 1/3] core: Add new unmaximize flag Updated patch after review
Created attachment 366610 [details] [review] [PATCH 3/3] wayland: update location prior to maximize Updated patch, add a flag and a new parameter to meta_window_force_placement() to be able to for the window location fields from meta_window_wayland_move_resize_internal()
Review of attachment 366609 [details] [review]: lgtm
Review of attachment 366610 [details] [review]: ::: src/wayland/meta-window-wayland.c @@ +339,3 @@ + window->rect.x = configured_x; + window->rect.y = configured_y; + } Would it make any sense to use "can_move_now"? The buffer_rect and rect will now be out-of-synch.
Created attachment 366644 [details] [review] [PATCH 3/3 v4] wayland: update location prior to maximize (In reply to Jonas Ådahl from comment #37) > Review of attachment 366610 [details] [review] [review]: > > ::: src/wayland/meta-window-wayland.c > @@ +339,3 @@ > + window->rect.x = configured_x; > + window->rect.y = configured_y; > + } > > Would it make any sense to use "can_move_now"? The buffer_rect and rect will > now be out-of-synch. Sure, works as well :)
Review of attachment 366644 [details] [review]: ::: src/wayland/meta-window-wayland.c @@ +237,3 @@ + window->rect.x = configured_x; + window->rect.y = configured_y; + can_move_now = TRUE; Not exactly what I meant. can_move_now being true will make the x/y = .. above happen later in this function (under "if (can_move_now)". Thus should be enough to just do can_move_now = TRUE; here.
Created attachment 366645 [details] [review] [PATCH 3/3 v5] wayland: update location prior to maximize (In reply to Jonas Ådahl from comment #39) > Review of attachment 366644 [details] [review] [review]: > > ::: src/wayland/meta-window-wayland.c > @@ +237,3 @@ > + window->rect.x = configured_x; > + window->rect.y = configured_y; > + can_move_now = TRUE; > > Not exactly what I meant. can_move_now being true will make the x/y = .. > above happen later in this function (under "if (can_move_now)". Thus should > be enough to just do can_move_now = TRUE; here. Ah blighmey, right! Note: I kept the curly braces on purpose for readability, even though these are not strictly necessary as there is only one statement left.
Review of attachment 366645 [details] [review]: (In reply to Olivier Fourdan from comment #40) ... > > Note: I kept the curly braces on purpose for readability, even though these > are not strictly necessary as there is only one statement left. It's also the coding style (if any branch of a condition needs brackets, then add brackets on all so they are horizontally aligned). Anyhow, lgtm!
Comment on attachment 366609 [details] [review] [PATCH 1/3] core: Add new unmaximize flag attachment 366609 [details] [review] pushed to git master as commit 1139ace - core: Add new unmaximize flag
Comment on attachment 354147 [details] [review] [PATCH 2/3] wayland: Do not enforce a size on un-maximize attachment 354147 [details] [review] pushed to git master as commit 6cf7d2d - wayland: Do not enforce a size on un-maximize
Comment on attachment 366645 [details] [review] [PATCH 3/3 v5] wayland: update location prior to maximize attachment 366645 [details] [review] pushed to git master as commit 5f05112 - wayland: update location prior to maximize