GNOME Bugzilla – Bug 770345
fullscreen transition coordinates are incorrect on wayland
Last modified: 2016-11-23 18:01:16 UTC
When setting an application to fullscreen on Wayland the coordinates for the transition do not match that of Xorg. This causes the transition to be a bit jarring. In my test, it looks like the animation origin is from coordinates like W,H.
Created attachment 336282 [details] [review] MetaPlugin: add a size_changed vfunc This will be used to let plugins know when a previous size change actually becomes effective. This is needed to handle wayland client resizing properly since, unlike X, it's async.
Created attachment 336283 [details] [review] window: Inform the compositor when a window effectively changes size In order for the compositor plugin to be able to animate window size changes properly we need to let it know of the starting and final window sizes. For X clients this can be done synchronously and thus with a single call into the compositor plugin since it's us (the window manager) who's in charge of the final window size. Wayland clients though, have the final say over their window size since it's determined from the client allocated buffer. This patch moves the meta_compositor_size_change_window() calls before move_resize_internal() which lets the compositor plugin know the old window size and freezes the MetaWindowActor. Then we get rid of the META_MOVE_RESIZE_DONT_SYNC_COMPOSITOR flag since it's not needed anymore as the window actor is frozen and that means we can use meta_compositor_sync_window_geometry() as the point where we inform the compositor plugin of the final window size.
Created attachment 336284 [details] [review] Revert "wayland: Mark pending moved as moved" This reverts commit 989ec7fc60d534a8167535795de141590be406bb. We now rely on accurately knowing if a window moved and/or resized in meta_window_move_resize_internal() so the wayland implementation can't lie any longer.
Created attachment 336285 [details] [review] window: Use the target rect for the grab anchor position on unmaximize A window's unconstrained_rect is essentially just the target rectangle we hand to meta_window_move_resize_internal() except it's not updated until the window actually moves or resizes. As such, for wayland client resizes, since they're async, using window->unconstrained_rect right after calling move_resize_internal() to update the grab anchor position on unmaximize doesn't work as it does for X clients. To fix this, we can just use the target rectangle for the grab anchor. Note that comment here was already wrong since it says we should be taking constraints into account and yet the code used the unconstrained rect anyway.
Created attachment 336286 [details] [review] shell-wm: Add a size-changed signal to handle the new plugin vfunc
Created attachment 336287 [details] [review] windowManager: Fix fullscreen animations of wayland clients Wayland clients are in control of their window size so the existing mutter plugin API, which assumes size changes are synchronous, doesn't work for them since when our size-change handler runs the MetaWindow's size isn't final yet. To fix this, the mutter plugin API was extended with a size-changed vfunc that lets us know when the MetaWindow size has actually changed. This way we can make the window snapshot and get the old window size on the existing size-change handler and later, on the new size-changed handler, get the new size and start the animation.
Created attachment 339335 [details] [review] windowManager: Fix fullscreen animations of wayland clients -- v2: Fixed a crash that could happen if _sizeChangedWindow() was called again before the previous animation ended and without an interveening _fullscreenAnimation() by clearing the fullscreen info at the end of _sizeChangedWindow().
Created attachment 339336 [details] [review] windowManager: Handle fullscreening from zero sized windows Wayland clients can request their surfaces to be fullscreened before commiting a buffer which means that we need to handle fullscreen requests for which the old size is 0x0, preferably without warnings.
In testing, those patches fix the animation glitch, but I can end up with "stuck" windows: 1. Hold F11 for a while (assuming that's what toggles fullscreen) 2. Make sure to unfullscreen the window The result is a window that is floating, but cannot be moved. Resizing does not work as expected (drag left edge, observe window growing to the right), and menus from a menubar pop up in unexpected locations.
Created attachment 340043 [details] [review] windowManager: Fix fullscreen animations of wayland clients -- (In reply to Florian Müllner from comment #9) > In testing, those patches fix the animation glitch, but I can end up with > "stuck" windows: > > 1. Hold F11 for a while (assuming that's what toggles fullscreen) > 2. Make sure to unfullscreen the window > > The result is a window that is floating, but cannot be moved. Resizing does > not work as expected (drag left edge, observe window growing to the right), > and menus from a menubar pop up in unexpected locations. Ok, this is because I wasn't unfreezing the actor if we got the first API signal consecutively without the second call in between. I moved the completed_size_change() to unfreeze the actor into the _clearFullscreenInfo() helper and I can't reproduce the stuck window anymore. There's still a problem when doing those steps though: after a few cycles, we "forget" the original window size and when we unfullscreen the window takes all the work area instead of its original size. Still chasing this one, but it seems to be in mutter's wayland protocol code.
(In reply to Rui Matos from comment #10) > There's still a problem when doing those steps though: after a few > cycles, we "forget" the original window size and when we unfullscreen > the window takes all the work area instead of its original size. Yes, I've seen this as well, but it happens with or without the patch set applied, so not directly related to this bug.
Review of attachment 336282 [details] [review]: ::: src/meta/meta-plugin.h @@ +104,3 @@ + void (*size_changed) (MetaPlugin *plugin, + MetaWindowActor *actor); Having both size_change() and size_changed() is a bit unfortunate as the names are bound to get mixed up, but I don't have a better suggestion.
Review of attachment 336283 [details] [review]: OK
Review of attachment 336284 [details] [review]: OK
Review of attachment 336285 [details] [review]: OK
Review of attachment 336286 [details] [review]: Sure
Review of attachment 340043 [details] [review]: ::: js/ui/windowManager.js @@ +1262,2 @@ else if (whichChange == Meta.SizeChange.UNFULLSCREEN) + this._fullscreenAnimation(shellwm, actor, oldFrameRect, false); Or maybe if (whichChange == Meta.SizeChange.FULLSCREEN || whichChange == Meta.SizeChange.UNFULLSCREEN) this._fullscreenAnimation(wm, actor, oldFrameRect, whichChange); to avoid the "magic" boolean? (It's an internal function just used here, so not a big deal to keep it like it is)
Review of attachment 339336 [details] [review]: ::: js/ui/windowManager.js @@ +1306,3 @@ + // Now scale the actor to size it as the clone + actor.scale_x = 1 / scaleX; + actor.scale_y = 1 / scaleY; I don't know how to actually test this case, but don't we end up with the actor popping up full-sized (and possibly sliding across monitors)? Should we make the animation match the map effect instead? Or maybe it makes sense to put this._clearFullscreenInfo(actor); if (oldFrameRect.width == 0 || oldFrameRect.height == 0) { shellwm.completed_size_change(actor); return; } at the beginning of _fullscreenAnimation() to just make this case a no-op?
Created attachment 340617 [details] [review] windowManager: Fix fullscreen animations of wayland clients -- Ok, while looking at this again, I noticed another bug that I introduced with the fix in the previous version: calling _clearFullscreenInfo() at the end of _sizeChangedWindow() means that the clone would get destroyed and we'd never see it. I fixed the previous problem (window actors getting stuck frozen) by just unfreezing the actor on _fullscreenAnimation() if there was a previous actor.__fullscreenInfo when it gets called.
Created attachment 340619 [details] [review] windowManager: Handle fullscreening from zero sized windows Wayland clients can request their surfaces to be fullscreened before commiting a buffer which means that we need to handle fullscreen requests for which the old size is 0x0, preferably without warnings. Since the mapping animation also runs for these windows, we can simply bail out and ignore the fullscreen size change. -- (In reply to Florian Müllner from comment #18) > I don't know how to actually test this case, but don't we end up with the > actor popping up full-sized (and possibly sliding across monitors)? Should > we make the animation match the map effect instead? Yeah, I think that could happen but I found out that we get the fullscreen call before the map call in these cases which means the fullscreen animation would immediately get replaced with the map animation which looks fine. > Or maybe it makes sense > to put > > this._clearFullscreenInfo(actor); > if (oldFrameRect.width == 0 || oldFrameRect.height == 0) { > shellwm.completed_size_change(actor); > return; > } > > at the beginning of _fullscreenAnimation() to just make this case a no-op? Right, I did something like this since we get the map animation anyway.
Review of attachment 340617 [details] [review]: (disclaimer: I looked at the patch and the diff to the previous version, but didn't do any testing) LGTM
Review of attachment 340619 [details] [review]: Ah, that's way better than my suggestion
Attachment 336282 [details] pushed as 9c03e78 - MetaPlugin: add a size_changed vfunc Attachment 336283 [details] pushed as 1d280d8 - window: Inform the compositor when a window effectively changes size Attachment 336284 [details] pushed as 5df5b00 - Revert "wayland: Mark pending moved as moved" Attachment 336285 [details] pushed as 1956a6a - window: Use the target rect for the grab anchor position on unmaximize
Attachment 336286 [details] pushed as 68b671a - shell-wm: Add a size-changed signal to handle the new plugin vfunc Attachment 340617 [details] pushed as 7bc1d57 - windowManager: Fix fullscreen animations of wayland clients Attachment 340619 [details] pushed as 3d6bf43 - windowManager: Handle fullscreening from zero sized windows