GNOME Bugzilla – Bug 637353
Overview: Remove invisible animations
Last modified: 2010-12-16 19:25:11 UTC
Basically if the user does not see it anyway, whats the point in doing an animation? Some numbers: Before: ------------------------------------------------------------ # Additional malloc'ed bytes the second time the overview is shown leakedAfterOverview 569280, 808000, 280112 # Frame rate when going to the overview, first time overviewFpsFirst 39.9578626176, 43.330998274, 41.7064773951 # Frames rate when going to the overview, second time overviewFpsSubsequent 54.5818229687, 49.5616888145, 50.5868069607 # Time to first frame after triggering overview, first time overviewLatencyFirst 86244, 82787, 97712 # Time to first frame after triggering overview, second time overviewLatencySubsequent 53553, 55822, 59915 # Malloc'ed bytes after the overview is shown once usedAfterOverview 141892000, 142197504, 143442592 ------------------------------------------------------------ After: ------------------------------------------------------------ # Additional malloc'ed bytes the second time the overview is shown leakedAfterOverview 160784, 205552, 215200 # Frame rate when going to the overview, first time overviewFpsFirst 53.2793435985, 54.0423692175, 50.4956994496 # Frames rate when going to the overview, second time overviewFpsSubsequent 60.9248390569, 61.6305386167, 60.7625702567 # Time to first frame after triggering overview, first time overviewLatencyFirst 126963, 106567, 115861 # Time to first frame after triggering overview, second time overviewLatencySubsequent 53335, 57098, 52639 # Malloc'ed bytes after the overview is shown once usedAfterOverview 131088192, 131084240, 131086752 ------------------------------------------------------------ Test run with: Workspace 1: 4 windows Workspace 2: 8 windows Workspace 3: 1 window Workspace 4: 2 windows
Created attachment 176506 [details] [review] Overview: Remove invisible animations Given that the grid view is gone there is no point in animating the window previews on all workspaces anymore so just do it for the current one avoid taking a slow down caused by animating windows on other workspaces.
Created attachment 176508 [details] [review] Overview: Remove invisible animations Given that the grid view is gone there is no point in animating the window previews on all workspaces anymore so just do it for the current one avoid taking a slow down caused by animating windows on other workspaces. --- *) Cleanups
Review of attachment 176508 [details] [review]: This looks like a good idea. I don't really like functions where doSomething(false) means dontDoSomething(), so it would be neat to get rid of the 'animate' parameter. If the suggestions below don't work out, renaming the functions to e.g. _showOverlay(clone, overlay, false) would be an OK alternative. ::: js/ui/workspace.js @@ +370,3 @@ + fadeIn: function(animate) { + this.title.opacity = animate ? 0 : 255; Hmmm, this.fadeIn(false) looks a bit weird, given that there is no fade ... I wonder if overlay.show() works correctly in the case where we don't want an animation. @@ +1006,3 @@ clone.actor.set_position(x, y); clone.actor.set_scale(scale, scale); + this._fadeInWindowOverlay(clone, overlay, false); It's perfectly fine to have animate == false && isOnCurrentWorkspace == true, which is a case where we _do_ want an animation. So the last parameter should be isOnCurrentWorkspace. But then again, it's a bit weird to have a function called "fade" which does not fade anything, so maybe it's cleaner to add a separate function for the non-animated case. Thinking about it, there's not much of a point in showing the overlay at all in this case, so I suspect that if (isOnCurrentWorkspace) this._fadeInWindowOverlay(clone, overlay); works just fine. @@ +1060,3 @@ if (this._showOnlyWindows != null && !(clone.metaWindow in this._showOnlyWindows)) continue; + this._fadeInWindowOverlay(clone, overlay, clone.metaWindow.get_workspace() == currentWorkspace); Hmmm - maybe we should leave the function as-is (e.g. _fadeInAllOverlays fades in all overlays) and modify _updateVisibility (in workspacesView) to only call showWindowOverlays for the active workspace? @@ +1235,3 @@ for (let i = 0; i < this._windows.length; i++) { let clone = this._windows[i]; + let isOnCurrentWorkspace = clone.metaWindow.get_workspace() == currentWorkspace; Any reason to have this inside the loop? this._windows is already filtered for windows on the corresponding workspace, so the above should be the same as: let isCurrentWorkspace = this.metaWorkspace == currentWorkspace; outside the loop. @@ +1257,3 @@ + clone.actor.scale_x = 1.0; + clone.actor.scale_y = 1.0; + clone.actor.opacity = 255; The workspace is destroyed after the animation, so the else part is not necessary. In fact, you can probably bail out early (even before entering the loop) with if (!isCurrentWorkspace) return; You probably should move the line this._visible = false; up when you do that, although the property is probably obsolete by now.
(In reply to comment #3) > Review of attachment 176508 [details] [review]: > > This looks like a good idea. > > I don't really like functions where doSomething(false) means dontDoSomething(), > so it would be neat to get rid of the 'animate' parameter. If the suggestions > below don't work out, renaming the functions to e.g. _showOverlay(clone, > overlay, false) would be an OK alternative. OK > ::: js/ui/workspace.js > @@ +370,3 @@ > > + fadeIn: function(animate) { > + this.title.opacity = animate ? 0 : 255; > > Hmmm, this.fadeIn(false) looks a bit weird, given that there is no fade ... > > I wonder if overlay.show() works correctly in the case where we don't want an > animation. It should. > @@ +1006,3 @@ > clone.actor.set_position(x, y); > clone.actor.set_scale(scale, scale); > + this._fadeInWindowOverlay(clone, overlay, false); > > It's perfectly fine to have animate == false && isOnCurrentWorkspace == true, > which is a case where we _do_ want an animation. So the last parameter should > be isOnCurrentWorkspace. > > But then again, it's a bit weird to have a function called "fade" which does > not fade anything, so maybe it's cleaner to add a separate function for the > non-animated case. Thinking about it, there's not much of a point in showing > the overlay at all in this case, so I suspect that > > if (isOnCurrentWorkspace) > this._fadeInWindowOverlay(clone, overlay); > > works just fine. No it doesn't, this way we end up with overlays just on the active workspace. > @@ +1060,3 @@ > if (this._showOnlyWindows != null && !(clone.metaWindow in > this._showOnlyWindows)) > continue; > + this._fadeInWindowOverlay(clone, overlay, > clone.metaWindow.get_workspace() == currentWorkspace); > > Hmmm - maybe we should leave the function as-is (e.g. _fadeInAllOverlays fades > in all overlays) and modify _updateVisibility (in workspacesView) to only call > showWindowOverlays for the active workspace? No, see above. > @@ +1235,3 @@ > for (let i = 0; i < this._windows.length; i++) { > let clone = this._windows[i]; > + let isOnCurrentWorkspace = clone.metaWindow.get_workspace() == > currentWorkspace; > > Any reason to have this inside the loop? this._windows is already filtered for > windows on the corresponding workspace, so the above should be the same as: > > let isCurrentWorkspace = this.metaWorkspace == currentWorkspace; > > outside the loop. Makes sense. > @@ +1257,3 @@ > + clone.actor.scale_x = 1.0; > + clone.actor.scale_y = 1.0; > + clone.actor.opacity = 255; > > The workspace is destroyed after the animation, so the else part is not > necessary. In fact, you can probably bail out early (even before entering the > loop) with > > if (!isCurrentWorkspace) > return; > > You probably should move the line > > this._visible = false; > > up when you do that, although the property is probably obsolete by now. OK.
(In reply to comment #3) > It's perfectly fine to have animate == false && isOnCurrentWorkspace == true, > which is a case where we _do_ want an animation. No I don't ;) animate == false means "do NOT want an animation".
Created attachment 176541 [details] [review] Overview: Remove invisible animations Given that the grid view is gone there is no point in animating the window previews on all workspaces anymore so just do it for the current one avoid taking a slow down caused by animating windows on other workspaces.
Created attachment 176542 [details] [review] Overview: Remove invisible animations Given that the grid view is gone there is no point in animating the window previews on all workspaces anymore so just do it for the current one avoid taking a slow down caused by animating windows on other workspaces. --- Seems I was wrong "animate" means "animate windows" not labels.
Review of attachment 176542 [details] [review]: Looks good. ::: js/ui/workspace.js @@ +1025,3 @@ }, + _showWindowOverlay: function(clone, overlay, animate) { I prefer 'fade' over 'animate', but up to you. @@ +1234,3 @@ + if (this._metaWorkspace == currentWorkspace) { + this._visible = false; Maybe worth doing a quick patch to remove all occurrences of this._visible before committing this patch - it's set in several places, but never queried (apparently since linear view landed)
Attachment 176542 [details] pushed as 7279cc1 - Overview: Remove invisible animations