GNOME Bugzilla – Bug 693924
viewSelector: don't crossfade between pages
Last modified: 2013-02-16 11:58:55 UTC
When controls are sliding out, moving in the next page causes it to reflow its contents in flight, which doesn't look very good. Fix it by removing the pages crossfade, and only fading the second one after the first is gone.
Created attachment 236313 [details] [review] viewSelector: don't crossfade between pages Instead, wait until the first animation is completed before switching to the second page. By using the same timeouts as the dash, we can synchronize the two animations, and avoid seeing the page expand while the dash is still shrinking.
Review of attachment 236313 [details] [review]: Makes sense.
Created attachment 236324 [details] [review] viewSelector: don't crossfade between pages Instead, wait until the first animation is completed before switching to the second page.
Created attachment 236325 [details] [review] viewSelector: emit a signal when the view is empty This is very a useful point in time when the size of the side controls can change without affecting the allocation of the view selector. Emit a signal at that time.
Created attachment 236326 [details] [review] overview: move ctrlAltTab handling to Dash This is also what the panel does.
Created attachment 236327 [details] [review] overview: move controls visibility handling to a separate object This keeps the core code of the overview clean and will help coordinating animations.
Created attachment 236328 [details] [review] overviewControls: make dash private
Created attachment 236329 [details] [review] overviewControls: split out an util function Will be used later
Created attachment 236330 [details] [review] overviewControls: use translations for explicit slide in/out Since we now can set the slide at the right time by using page-empty, use translations to perform the slide in/out animations. The slideX resize for now is kept for DnD, and will always be needed for the thumbnails box when showing the windows page.
Here's a new patchset that completely removes any unwanted movements and resizes when switching pages - I really like the overall effect, I think it's a huge improvement.
Review of attachment 236326 [details] [review]: OK.
Review of attachment 236330 [details] [review]: Don't bother reviewing this yet, I noticed there are still some glitches.
Review of attachment 236324 [details] [review]: ::: js/ui/viewSelector.js @@ +205,3 @@ }, + _fadePageIn: function(page) { Parameter is unused? @@ +206,3 @@ + _fadePageIn: function(page) { + if (this._activePage) { Could activePage ever be null?
Review of attachment 236329 [details] [review]: Well, this is the last patch in the stack. Not sure how it's going to be used later, except in a later stack.
Review of attachment 236329 [details] [review]: Oh, sorry, my eyes skipped over the needs-work one.
Review of attachment 236328 [details] [review]: Ok.
Review of attachment 236325 [details] [review]: Uh? If you changed the size of the side controls, the view selector would still get a smaller or bigger allocation. It's not noticeable, since there's no visible actor, but the commit message is wrong, I think.
Created attachment 236334 [details] [review] overviewControls: use translations for explicit slide in/out Since we now can set the slide at the right time by using page-empty, use translations to perform the slide in/out animations. The slideX resize for now is kept for DnD, and will always be needed for the thumbnails box when showing the windows page.
Review of attachment 236327 [details] [review]: OK. ::: js/ui/overviewControls.js @@ +253,3 @@ + // actors will be made visible again when entering the overview + // next time, and animating them while doing so is just + // unnecesary noise unnecessary
Review of attachment 236327 [details] [review]: "help coordinate" or "help with coordinating" in commit message
Created attachment 236335 [details] [review] viewSelector: don't crossfade between pages -- Yeah, the check was useless, I removed it now.
Created attachment 236336 [details] [review] viewSelector: emit a signal when the view is empty -- The commit message was indeed a bit misleading; what I meant is that no visible view is affected by a size change.
Review of attachment 236334 [details] [review]: Not the biggest fan of this patch. For the app picker refresh ( https://live.gnome.org/GnomeShell/Design/AppPickerRefresh) it looks like we want a partially hidden dash, too, so delegating slideX to DND only is sort of bad. I'd want two controls that we can set: widthFactor and slideFactor, both from 0.0 to 1.0. The names aren't perfect, feel free to come up with your own. The math should work out so that if we had widthFactor=0.5 and slideFactor=0.5, it's like we had slideX=0.25, from the position and looks side of things. This way, we can have DND/thumbnails on widthFactor, and move controls out of the way with slideFactor, and in the future, we would have DND on slideFactor, too. I don't know how this makes sense for the _getSlide() scheme. ::: js/ui/overviewControls.js @@ +143,3 @@ + this.actor.translation_x = translationStart; + Tweener.addTween(this.actor, { translation_x: translationEnd, + time: SIDE_CONTROLS_ANIMATION_TIME, Since the page tweens at this / 2, doesn't this mean this will extend for the full time? We should cut the variable in half, and drop the / 2 for the pages. @@ +176,3 @@ + + pageEmpty: function() { + if (this.slideInPending) { The slideInPending stuff sounds like it's some state management that's going to wrong at some point. I'd like to see if we can orchestrate this better. From reading this patch, each transition is: * User types a letter * Windows quickly fade out. Side controls stay where they are. * As the search results fade in, the side controls slide out. This doesn't sound like it will fix the problem for us, as the allocation of the search results is still affected. Have you checked the jimmac's motion mockups for the search?
Review of attachment 236336 [details] [review]: Going to defer reviewing this one until we determine it's what we want, in the other patch.
Review of attachment 236335 [details] [review]: OK.
Created attachment 236339 [details] [review] overviewControls: use translations for explicit slide in/out -- Removes the slideInPending stuff, and fixes the translation to use the visible width when the actor is not zoomed out. Also, this reduces the delay in animations the other patch had, since we always start sliding controls immediately.
Created attachment 236342 [details] [review] overviewControls: use translations for explicit slide in/out -- Now with more comments.
Review of attachment 236342 [details] [review]: When commented, the flow here is much cleaner. Thanks.
Attachment 236326 [details] pushed as 1379c6b - overview: move ctrlAltTab handling to Dash Attachment 236327 [details] pushed as 579e53f - overview: move controls visibility handling to a separate object Attachment 236328 [details] pushed as 8ce3531 - overviewControls: make dash private Attachment 236329 [details] pushed as 7ed4bd6 - overviewControls: split out an util function Attachment 236335 [details] pushed as 278686d - viewSelector: don't crossfade between pages Attachment 236336 [details] pushed as 153d304 - viewSelector: emit a signal when the view is empty Attachment 236342 [details] pushed as 41406e3 - overviewControls: use translations for explicit slide in/out
Very nice indeed.