GNOME Bugzilla – Bug 690241
Animate window destruction
Last modified: 2012-12-16 21:41:06 UTC
Two late night WindowManager patches, and something that needs designer review but I like: when a window is unmapped, we animate the opacity to 0 before destroying the actor. Or that was the intention. Testing at slower speed, I found out the window texture goes away immediately, so what's really animated is the background shadow. Still, at normal speed it looks better to me than just killing the window altogether. (While bugzilla forced me to choose a component, I noticed that, despite being a shell and a window manager, we have no component for window management or application search/launch. Funny! )
Created attachment 231608 [details] [review] WindowManager: clean up effects code Use consistently shouldAnimateActor(), and try to have a uniform code flow between the modal dialog and the normal case.
Created attachment 231609 [details] [review] WindowManager: scale windows to primary monitors corner on minimize We previously scaled windows to (primary.x, 0), which is wrong if the primary monitor is not at y == 0.
Created attachment 231610 [details] [review] WindowManager: tween out the opacity when a window is destroyed It looks nicer, and is symmetric with the mapping effect.
This has been discussed before. Indeed, window contents may get destroyed before we have any say in the matter. The TFP spec says that if we don't re-bind the texture and there's a DAMAGE event, that the current state of the pixmap is "undefined", so we'd have to copy the window pixmap ourselves, which is expensive.
Review of attachment 231608 [details] [review]: Nice cleanup in general. ::: js/ui/windowManager.js @@ +210,2 @@ _shouldAnimateActor: function(actor) { if (!this._shouldAnimate()) You should be able to move shouldAnimate() inline here. @@ +410,3 @@ Tweener.removeTweens(actor); actor.opacity = 255; + actor.scale_y = 1; Seems like an unrelated bugfix? Would like it in a separate patch.
Review of attachment 231609 [details] [review]: OK
Review of attachment 231610 [details] [review]: Not going to happen.
(In reply to comment #4) > This has been discussed before. Indeed, window contents may get destroyed > before we have any say in the matter. The TFP spec says that if we don't > re-bind the texture and there's a DAMAGE event, that the current state of the > pixmap is "undefined", so we'd have to copy the window pixmap ourselves, which > is expensive. Uhm... Docs for XCompositeNameWindowPixmap say that the pixmap remains allocated until freed, even if the window is unmapped or destroyed, so can't we just keep processing damage for the old pixmap, update the TFP texture and paint? Currently, mutter stops processing damage when an effect is in progress, with a big comment in meta_window_actor_process_damage() saying it's done to freeze the window, but as I understand this just corrupts the texture. OTOH, the TFP spec says that if we don't re-bind the texture and any client renders on it (which is perfectly possible that happens between when we bind the texture and when we paint with it), the result will be undefined. Now, I'm not saying that continuing damage processing is easy to do, given that we unregister the window when unmanaged...
Uh oh. I think I found the problem: window contents are not undefined, they're correctly updated as the window is updated. And in particular, as the client window is destroyed, the frame window and pixmap are correctly repainted with the background. So yes, this another bug that won't be solved until we have client side decorations. Leaving open for the cleanups, once committed this becomes WONTFIX.
Created attachment 231663 [details] [review] WindowManager: clean up scale_y when overwrite the map animation If we overwrite a map animation (for example because the actor is now destroyed), we need to complete it first, otherwise it starts off from a random middle point. This is the same treatment opacity gets for normal windows.
Created attachment 231664 [details] [review] WindowManager: clean up effects code Use consistently shouldAnimateActor(), and try to have a uniform code flow between the modal dialog and the normal case. I can't inline shouldAnimate because that's used by switchWorkspace too.
(In reply to comment #9) > Uh oh. I think I found the problem: window contents are not undefined, they're > correctly updated as the window is updated. And in particular, as the client > window is destroyed, the frame window and pixmap are correctly repainted with > the background. It's undefined by the spec. That's the implementation some graphics drivers will give it, because it's efficient. """ Rendering to the drawable while it is bound to a texture will leave the contents of the texture in an undefined state. However, no synchronization between rendering and texturing is done by GLX. It is the application's responsibility to implement any synchronization required. """ It's undefined because it might be slower in some implementations that would be require a copy.
Review of attachment 231609 [details] [review]: Actually, not yet. This should probably go to the position of the monitor the window is on, because there's a hot corner there, and that's closest to where it will appear in the overview.
Review of attachment 231663 [details] [review]: OK.
Review of attachment 231664 [details] [review]: OK.
Attachment 231663 [details] pushed as 15063ef - WindowManager: clean up scale_y when overwrite the map animation Attachment 231664 [details] pushed as 3b8a125 - WindowManager: clean up effects code
Created attachment 231666 [details] [review] LayoutManager: fix findMonitorForWindow to take a metaWindow This makes the method usable in places where the associated window actor might not have the right size (such as from window manager animations). Also, make the method public from LayoutManager.
Created attachment 231667 [details] [review] WindowManager: scale windows to their monitor's corner on minimize We previously scaled windows to (primary.x, 0), which is wrong if the window is not on the primary monitor, or if the primary monitor is not at y == 0.
Review of attachment 231666 [details] [review]: OK.
Review of attachment 231667 [details] [review]: Commit message is wrong, otherwise fine.
(In reply to comment #20) > Review of attachment 231667 [details] [review]: > > Commit message is wrong, otherwise fine. Wrong? In what way?
I'd like some expansion about *why* we made this change. Feel free to copy the rationale I made above.
(In reply to comment #9) > So yes, this another bug that won't be solved until we have client side > decorations. > Leaving open for the cleanups, once committed this becomes WONTFIX. Why does CSD make a difference?
(In reply to comment #23) > (In reply to comment #9) > > So yes, this another bug that won't be solved until we have client side > > decorations. > > Leaving open for the cleanups, once committed this becomes WONTFIX. > > Why does CSD make a difference? Because the client stops rendering on the window, so the pixmap contents are effectively a frozen image of the window. Without CSD, the client drops the window, so the frame window is repainted with just the background by X, the pixmap is updated and the actual window contents are lost.
Attachment 231666 [details] pushed as b38ecaf - LayoutManager: fix findMonitorForWindow to take a metaWindow Attachment 231667 [details] pushed as 1256af7 - WindowManager: scale windows to their monitor's corner on minimize