GNOME Bugzilla – Bug 707248
Add support for fullscreen and unfullscreen animations
Last modified: 2015-10-17 22:59:51 UTC
This is a first implementation for compositor animations when a window is made fullscreen or exits fullscreen. Comments on my approach would be great.
Created attachment 253789 [details] [review] compositor: add hooks for fullscreen and unfullscreen animations
Created attachment 253790 [details] [review] windowManager: add animations for fullscreen and unfullscreen We use the newly introduced feature from Mutter to hook up our own fullscreen and unfullscreen animations. To give the illusion of a transition as smooth as possible, we create a snapshot of the current contents of the actor before its state is changed, and crossfade between the two states while the size changes. -- This is the shell part.
Review of attachment 253790 [details] [review]: I think this would have a race condition if the app chooses to fullscreen on its own, as it could have prepared a redraw, after which we'd be too late to respond. But we may not care about this in practice. I'd also like to see if we can use the GPU to render the snapshot instead of using the CPU and cairo, but it might be sufficiently complicated, and this is a one-time cost, so I'm fine with landing this without it. ::: js/ui/windowManager.js @@ +740,3 @@ + let actorContent = Shell.util_get_content_for_window_actor(actor, oldRect); + let actorClone = new St.Widget({ content: actorContent }); + actorClone.set_offscreen_redirect(Clutter.OffscreenRedirect.ALWAYS); What's the purpose of setting it to paint to an FBO? @@ +745,3 @@ + Main.uiGroup.add_actor(actorClone); + + actor.__fullscreenClone = actorClone; Instead of doing this, can't you simply add it to the onCompleteParams? @@ +766,3 @@ + actor.translation_y = actor.y; + actor.scaleX = 1 / scaleX; + actor.scaleY = 1 / scaleY; Huh, does this work instead of using scale_x / scale_y? @@ +777,3 @@ + }); + + shellwm.completed_fullscreen(actor); This needs to be in fullscreenWindowDone.
Review of attachment 253789 [details] [review]: ::: src/core/window.c @@ +4117,3 @@ + meta_screen_get_monitor_geometry (window->screen, + window->monitor->number, + &new_rect); Hm, I thought fullscreen windows could span multiple monitors with _NET_WM_FULLSCREEN_MONITORS? I'd double-check with the code in constraints.c
> Hm, I thought fullscreen windows could span multiple monitors with > _NET_WM_FULLSCREEN_MONITORS? I'd double-check with the code in constraints.c Yes they can. In fact, most Linux VMware products do this to support virtual machines across multiple monitors. I would suggest avoiding transition in those cases.
Review of attachment 253790 [details] [review]: I don't really like the "read back and do in software" thing either but that aside ... does this work for unredirected windows? (which is very likely to happen for fullscreen windows). You have to do all this *after* the window gets redirected (which happens when it gets unfullscreened didn't check on which exact point though).
Created attachment 281950 [details] [review] compositor: add hooks for fullscreen and unfullscreen animations Rebased to git master.
Created attachment 281951 [details] [review] windowManager: add animations for fullscreen and unfullscreen Updated and rebased to git master
Review of attachment 281950 [details] [review]: Barring any big design change in the way we do window animations, there is not much to review here.
Review of attachment 281951 [details] [review]: Mh... XComposite (as implemented in Xorg for the free drivers) creates a new pixmap any time the window changes size, so we can keep using the old one until we NameWindowPixmap, which we don't do until the actor is thawed at the end of animation, so why do we need this?
*** Bug 724758 has been marked as a duplicate of this bug. ***
I pushed rebased patches here: https://git.gnome.org/browse/mutter/log/?h=wip/fullscreen https://git.gnome.org/browse/gnome-shell/log/?h=wip/fullscreen
(In reply to Cosimo Cecchi from comment #12) > I pushed rebased patches here: > https://git.gnome.org/browse/mutter/log/?h=wip/fullscreen > https://git.gnome.org/browse/gnome-shell/log/?h=wip/fullscreen Not asking for this to be done right now, but do you think it would be enhanced so that the client tells you which part of the window to zoom in to?
(In reply to Bastien Nocera from comment #13) > Not asking for this to be done right now, but do you think it would be > enhanced so that the client tells you which part of the window to zoom in to? Yeah - that would be a combination of https://bugzilla.gnome.org/show_bug.cgi?id=747578 and https://bugzilla.gnome.org/show_bug.cgi?id=641648 which could be implemented on top of this.
Created attachment 313142 [details] [review] windowManager: add animations for fullscreen and unfullscreen -- Updated to use the new size change framework in mutter.
Created attachment 313143 [details] [review] compositor: add hooks for fullscreen and unfullscreen animations -- Updated for the new size change framework.
Review of attachment 313143 [details] [review]: ::: src/meta/compositor.h @@ +59,3 @@ META_SIZE_CHANGE_UNMAXIMIZE, + META_SIZE_CHANGE_FULLSCREEN, + META_SIZE_CHANGE_UNFULLSCREEN missing comma ::: src/x11/window-x11.c @@ +2044,3 @@ meta_window_move_resize_internal (window, flags, gravity, rect); + + if (legacy_fullscreen) Should legacy fullscreen windows get animations? That doesn't seem right to me.
Review of attachment 313142 [details] [review]: ::: js/ui/windowManager.js @@ +1275,3 @@ + }); + + shellwm.completed_size_change(actor); So, part of the reason it's not working like how you want it to is because of this -- you're completing the size change immediately instead of letting it freeze.
Created attachment 313162 [details] [review] compositor: add hooks for fullscreen and unfullscreen animations
Created attachment 313163 [details] [review] windowManager: add animations for fullscreen and unfullscreen -- The part where I don't let it freeze is intentional; I added some comments to the code to explain the logic, fixed a couple of bugs and refactored the animations in a single function now.
Review of attachment 313162 [details] [review]: Looks good to me.
Review of attachment 313163 [details] [review]: Not entirely happy with the code, but fine by me.
Comment on attachment 313162 [details] [review] compositor: add hooks for fullscreen and unfullscreen animations Attachment 313162 [details] pushed as a692fd3 - compositor: add hooks for fullscreen and unfullscreen animations
Attachment 313163 [details] pushed as 207c847 - windowManager: add animations for fullscreen and unfullscreen
Hey, I love the animations, but I would like to add a note that the fullscreen animations look really weird on the 2nd screen of a dual screen setup. Whenever I fullscreen a youtube video the effect awkwardly slides in from the right, and when I un-fullscreen it the effect slides all the way across the primary monitor before settling on the 2nd monitor. However, on the primary monitor the effects look great.
(In reply to Britt Yazel from comment #25) > Hey, I love the animations, but I would like to add a note that the > fullscreen animations look really weird on the 2nd screen of a dual screen > setup. Fixed in bug 756697.
Great! You rock
Another quick comment, I see when pressing a fullscreen button from a youtube video with a web browser or by pressing f11 on a window we get the nice maximize and minimize effect, but neither double clicking a window border nor right clicking the border and saying maximize elicits an effect. Is this by design?
(In reply to Britt Yazel from comment #28) > Another quick comment, I see when pressing a fullscreen button from a > youtube video with a web browser or by pressing f11 on a window we get the > nice maximize and minimize effect, but neither double clicking a window > border nor right clicking the border and saying maximize elicits an effect. > Is this by design? Yes this is about transitions to fullscreen not about maximize. The former is when an application covers the entire screen ... the latter just resizes the window to fit the workarea and leaves the panel and window decorations (if any) visible.
Ahh I see, I guess I did not think of those as two different sets of actions, rather two varieties of the same action