Created attachment 280163 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from others pages
like apps page or search page since the windows comes from nowhere
with a initial position not known for the user.

Instead of that just fade the desktop with the windows in its
original position.

Review of attachment 280163 [details] [review]:

What about the opposite direction, e.g. when opening the overview with <super>a? Should zoomToOverview() be replaced with animateToOverview() too?

::: js/ui/viewSelector.js
@@ +175,3 @@
+        if (!zoom) {
+            // The animation on workspaces.js will take care of the
+            // opacity of the windows.

Maybe get rid of the intermediate "zoom" variable and use
  if (animationType == WorkspaceType.FADE_IN) { ... } ?
This should make it fairly obvious that animateFromOverview() is expected to handle the opacity ...

@@ +179,3 @@
+            this._workspacesPage.opacity = 255;
+        }
+        this._showPage(this._workspacesPage);

Only needed in the !zoom case (_showPage() returns early otherwise, so no big deal leaving it like this)

::: js/ui/workspace.js
@@ +1612,3 @@
+                    clone.actor.opacity = 0;
+                }
+                    Tweener.addTween(clone.actor,

Nit: wrong indentation

(In reply to comment #2)
> Review of attachment 280163 [details] [review]:
> 
> What about the opposite direction, e.g. when opening the overview with
> <super>a? Should zoomToOverview() be replaced with animateToOverview() too?
> 
> ::: js/ui/viewSelector.js
> @@ +175,3 @@
> +        if (!zoom) {
> +            // The animation on workspaces.js will take care of the
> +            // opacity of the windows.
> 
That is actually necessary on my swarm animation work, that's why I didn't modify it here. Since you agree to change the name here too, I'll do it.
> Maybe get rid of the intermediate "zoom" variable and use
>   if (animationType == WorkspaceType.FADE_IN) { ... } ?
> This should make it fairly obvious that animateFromOverview() is expected to
> handle the opacity ...
> 
> @@ +179,3 @@
> +            this._workspacesPage.opacity = 255;
> +        }
> +        this._showPage(this._workspacesPage);
> 
> Only needed in the !zoom case (_showPage() returns early otherwise, so no big
> deal leaving it like this)
> 
Not sure you read it correctly, the activePage at this moment is not workspacePage. But I show it and put full opacity to let it animate right now.
> ::: js/ui/workspace.js
> @@ +1612,3 @@
> +                    clone.actor.opacity = 0;
> +                }
> +                    Tweener.addTween(clone.actor,
> 
> Nit: wrong indentation
ups yeah

> > 
> > Only needed in the !zoom case (_showPage() returns early otherwise, so no big
> > deal leaving it like this)
> > 
> Not sure you read it correctly, the activePage at this moment is not
> workspacePage. But I show it and put full opacity to let it animate right now.
I take that back, I understand what you meant.

Created attachment 280167 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from others pages
like apps page or search page since the windows comes from nowhere
with a initial position not known for the user.

Instead of that just fade the desktop with the windows in its
original position.

(In reply to comment #2)
> What about the opposite direction, e.g. when opening the overview with
> <super>a? Should zoomToOverview() be replaced with animateToOverview() too?

What I meant here:
If we open the app picker directly, does the zoom animation really make more sense than the old zoom animation you remove when leaving the overview from the app picker, or should we fade out the windows in that case as well?

Comment on attachment 280167 [details] [review]
workspace: Fade in instead of zoom to return desktop

Carlos noticed that the patch does not handle multi-monitor at all, so marking needs-work to get off the review list

Created attachment 280668 [details] [review]
viewSelector: Remove duplicate call to showPage

We were caling twice showPage() with the correct
page, here and in show() / animateFromOverview.
Also, this call here doesn't belong at all to reseting
the button.

Created attachment 280669 [details] [review]
overviewControls: Don't slideIn always to overview

Here we were sliding in always when entering overview,
overriding the explicit calls to slideIn.
This is wrong. So just let the explicit calls behave
like it should.
This function could be entirely deleted, but for some reason the
translation at the time pageEmpty is called is not correct,
so we still need to adjust the translation in the middle of
the animation. With that pourpose, let the updateTranslation
call (which was actually called before too when calling slideIn).

Created attachment 280670 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from others pages
like apps page or search page since the windows comes from nowhere
with a initial position not known for the user.

Instead of that just fade the desktop with the windows in its
original position.

Created attachment 282503 [details] [review]
viewSelector: Remove duplicate call to showPage

We were caling twice showPage() with the correct page, here and in show() /
zoomFromOverview given that _resetShowAppsbutton was called from the signal
'showing' of overview.
Given that the call to _resetShowAppsbutton is only actually used when hiding
the overview we can actually put the checked state of the button to false when
animating from overview so it shows the workspace page, causing the same
behaviour of _resetShowAppsbutton withouth all the sheaningang of reseting when
the hiding overview signal is trigered.

Created attachment 282504 [details] [review]
workspaceThumbnails: allow requesting size at any time

The slide of thumbnailWorkspace is shown when entering overview, connecting to
the same signal that creates the thumbnails, the showing signal of overview,
but, to make the slide animation we need to know how much width the slider has.
To do that we ask the thumbnailsWorkspace about its width, but given that it
connects to the same signal it could ask the width withouth having created the
thumbnails yet, so reporting a width of 0 and confusing the slide animation.

Currently it acccidentaly works because gjs calls the callbacks following the
order of the clients connecting that signal, and accidentally the
thumbnailsWorskpace is connected before the slide ones.

To avoid that we allow to request the preferred size of the thumbnailsBox at any
time with any number of thumbnails. The only thing required is to make sure the
porthole is accesible when requesting the preferred size.

Created attachment 282505 [details] [review]
overviewControls: don't override explicit calls to slideIn

Currently we are overriding the explicit calls to slideIn
given that it's called also with the signal of showing overview.

It was necesary because of the bug that previous patch fixed,
so now we can just delete that.

Created attachment 282506 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from others pages
like apps page or search page since the windows comes from nowhere
with a initial position not known for the user.

Instead of that just fade the desktop with the windows in its
original position.

Florian and me reviewed in person and he had some comments; thought some of them didn't like much after implementation. Here's the comments from Florian and my reasons to not do it:

Patch viewSelector: Remove duplicate call to showPage:
- noFade in another patch: I thin it doesn't has much sense to let a non used param and delete it on a patch after. So I just deleted it in the first patch.

Patch workspace: Fade in instead of zoom to return desktop
- set showAppButon.checked to a value before showing overview and delete custom property: showingAppsFromOverview needed for aninmateFromOverview from search page. Then I think I prefer to have consistency and use both variables, ShowingAppsToOverview & showingAppsFromOverview and not delete ShowingAppsToOverview property and  replace it with showAppsButton.checked.
- add hide() to the hack for opacity on workspaceView: Well, for my use I still need to set the opacity explicitly, so adding a visible hack along the opacity hack won't provide me anything useful. The only advantage would be to call workspacePage.visible = false on fadePageIn instead of workspacePage.opacity = 0.
But given that it is already a hack and the amount of code necessary, I don't think it worth it.

All other reviews applied.

Review of attachment 282503 [details] [review]:

The commit message is a bit misleading ("causing the same behavior" => not quite, there's now an additional transition), but ok given that this'll be addressed in future commits. A couple of misspellings though: caling => calling, withouth => without, sheaningang => shenanigans, reseting => resetting (also: behaviour is British spelling, we generally use American spelling, thus behavior)

::: js/ui/viewSelector.js
@@ -407,3 @@
 
-    _resetShowAppsButton: function() {
-        this._showAppsBlocked = true;

This is the only place where _showAppsBlocked was set to true, so you can kill that property too

Review of attachment 282504 [details] [review]:

It's not quite as accidental as the commit message makes it sound, but making it work more obviously doesn't look like a bad idea. As always, a couple of typos: withouth => without, accidentaly => accidentally, accesible => accessible

::: js/ui/workspaceThumbnail.js
@@ +1153,3 @@
 
+    _ensurePorthole: function() {
+        this._porthole = Main.layoutManager.getWorkAreaForMonitor(Main.layoutManager.primaryIndex);

You are introducing a tiny risk of inconsistency when the porthole changes between getWidth() and getHeight(); this is not the case with the current code, which updates the porthole once in _createThumbnails() (which btw is now obsolete). The method name would suggest something like:
  if (!this._porthole)
      this._porthole = ...
which should work as well, provided you invalidate it when destroying the thumbnails.

Review of attachment 282505 [details] [review]:

OK

Review of attachment 282506 [details] [review]:

Obligatory commit message nit: "a initial position" => "an initial position" ;-)

::: js/ui/viewSelector.js
@@ +298,2 @@
     _toggleAppsPage: function() {
+        this.showingAppsToOverview = !this._showAppsButton.checked;

Still not a fan of this, but can you at least make this private?

@@ +322,3 @@
 
+    animateFromOverview: function() {
+        let showingAppsFromOverview = this._activePage != this._workspacesPage;

... except that the condition is true for either app picker or search display

@@ +326,3 @@
+            // Let workspace.js manage the animation of the windows clones.
+            this._workspacesPage.opacity = 255;
+            this._showAppsButton.checked = false;

I think I'd prefer those unconditionally
("make workspaces visible and uncheck apps-button before starting the transition"
 reads easier to me without having to think about the omitted else block)

@@ +385,3 @@
+                        this.showingAppsToOverview = false;
+                        this._workspacesPage.opacity = 0;
+                    }

Could we have something like:

Main.overview.connect('shown', Lang.bind(this,
    function() {
        this._showingAppsToOverview = false;
        if (this._activePage != this._workspacesPage)
            this._workspacesPage.opacity = 0;
    }));

instead? Tying the alternative overview transition into normal page switches feels a bit accidental ...

::: js/ui/workspace.js
@@ +1573,3 @@
+        // We'll use the OverviewControls.SIDE_CONTROLS_ANIMATION_TIME as max
+        // time instead of Overview.ANIMATION_TIME because we are going to
+        // show the apps page, and that's the time it lasts to show it and

That's basically because you're abusing _fadePageIn() to hide the workspace page? I'd prefer not to do that, see comment in viewSelector.js

@@ +1577,3 @@
+        // we avoid cutting the animation at the middle.
+        let windowBaseTime;
+        if (this._windows.length > WINDOW_ANIMATION_MAX_NUMBER_BLENDING) {

I'd prefer using Math.min() over if-else here, e.g.

let nTimeSlots = Math.min(WINDOW_ANIMATION_MAX_NUMBER_BLENDING + 1, this._windows.length);
let windowBaseTime = OverviewControl.SIDE_CONTROLS_ANIMATION_TIME / nTimeSlots;

@@ +1582,3 @@
+            windowBaseTime = OverviewControls.SIDE_CONTROLS_ANIMATION_TIME / (WINDOW_ANIMATION_MAX_NUMBER_BLENDING + 1);
+        } else {
+            windowBaseTime = OverviewControls.SIDE_CONTROLS_ANIMATION_TIME / this._windows.length;

Possible division by zero! There's also no tween to attach the onComplete handler to in that case, but I guess that's OK ...

@@ +1587,3 @@
+            this.animatingWindowsFade = false;
+            // Position and scale the windows in case the user use workspacePage
+            // after using appsPage

Not sure the comment is really useful - calling recalculateWindowPositions() at this point seems fairly obvious

@@ +1593,3 @@
+            for (let i = 0; i < this._windows.length; i++) {
+                let clone = this._windows[i];
+                clone.actor.opacity = 255;

Why is this necessary? We should already be doing this[0], no?

[0] https://git.gnome.org/browse/gnome-shell/tree/js/ui/workspace.js#n1335

@@ +1597,3 @@
+        });
+        // Animate gradually the most WINDOW_ANIMATION_MAX_NUMBER_BLENDING accesed windows hoping that
+        // those covers most of the screen so we can achieve a smooth transition.

This comment looks wrong - this._windows[0] is the *least* recently used window, so you start animating from the bottom of the stack (which is probably the right thing to do)

Also have you considered special-casing maximized windows? Fading any windows stacked below a maximized window doesn't really make much sense, we could just show/hide them directly at the end of the animation.

@@ +1622,3 @@
+            overlay.hide();
+
+        let [origX, origY] = clone.getOriginalPosition();

Unused variables. Missing handling of minimized windows.

@@ +1659,3 @@
+            windowBaseTime = Overview.ANIMATION_TIME / (WINDOW_ANIMATION_MAX_NUMBER_BLENDING + 1);
+        } else {
+            windowBaseTime = Overview.ANIMATION_TIME / this._windows.length;

See above.

@@ +1675,3 @@
+        // Reverse order of windows, so the most upper in the stack is animated first
+        let reverseStackWindow = this._windows.slice();
+        reverseStackWindow.reverse();

I don't like having three _frobnicateWindow(i)-style functions where "i" means either "the index into this._windows" or "the index into a temporary array which is the reverse of this._windows". Can you move this into the caller, e.g.

for (let i = 0; i < Math.min(); i++)
    this._fadeWindowFromOverview(this._windows.length - i - 1, ...);

@@ +1700,3 @@
+            clone.actor.scale_x = 0;
+            clone.actor.scale_y = 0;
+            clone.actor.opacity = 0;

Using scale_x/scale_y/opacity is a silly way to hide a window in the non-animated case - just use clone.actor.hide(), or something like:

clone.actor.visible = clone.metaWindow.showing_on_its_workspace();
if (clone.actor.visible) {
    [clone.actor.x, clone.actor.y] = clone.getOriginalPosition();
    clone.actor.scale_x = clone.actor.scale_y = 1;
    ...
}

::: js/ui/workspacesView.js
@@ +483,3 @@
     },
 
+    show: function(showingApps) {

Maybe "fadeOnPrimary"? In particular in animateFromOverview() the parameter is misnamed considering the search->desktop transition ...

(In reply to comment #16)
> Review of attachment 282503 [details] [review]:
> 
> The commit message is a bit misleading ("causing the same behavior" => not
> quite, there's now an additional transition), but ok given that this'll be
> addressed in future commits. A couple of misspellings though: caling =>
> calling, withouth => without, sheaningang => shenanigans, reseting => resetting
> (also: behaviour is British spelling, we generally use American spelling, thus
> behavior)
=(
> 
> ::: js/ui/viewSelector.js
> @@ -407,3 @@
> 
> -    _resetShowAppsButton: function() {
> -        this._showAppsBlocked = true;
> 
> This is the only place where _showAppsBlocked was set to true, so you can kill
> that property too
right, I forgot

(In reply to comment #17)
> Review of attachment 282504 [details] [review]:
> 
> It's not quite as accidental as the commit message makes it sound, but making
> it work more obviously doesn't look like a bad idea. As always, a couple of
> typos: withouth => without, accidentaly => accidentally, accesible =>
> accessible
> 
=( hope vim spell checker will help. If not I guess I can write it on libreoffice or on some spell checker before.
> ::: js/ui/workspaceThumbnail.js
> @@ +1153,3 @@
> 
> +    _ensurePorthole: function() {
> +        this._porthole =
> Main.layoutManager.getWorkAreaForMonitor(Main.layoutManager.primaryIndex);
> 
> You are introducing a tiny risk of inconsistency when the porthole changes
> between getWidth() and getHeight(); this is not the case with the current code,
> which updates the porthole once in _createThumbnails() (which btw is now
> obsolete). The method name would suggest something like:
>   if (!this._porthole)
>       this._porthole = ...
> which should work as well, provided you invalidate it when destroying the
> thumbnails.
Right

Created attachment 282760 [details] [review]
viewSelector: Remove duplicate call to showPage

We were calling twice showPage() with the correct page, here and in show() /
zoomFromOverview given that _resetShowAppsbutton was called from the signal
'showing' of overview.
Given that the call to _resetShowAppsbutton is only actually used when hiding
the overview we can actually put the checked state of the button to false when
animating from overview so it shows the workspace page, causing the same
behavior of _resetShowAppsbutton without all the shenanigans of reseting when
the hiding overview signal is triggered.

Created attachment 282761 [details] [review]
workspaceThumbnails: allow requesting size at any time

The slide of thumbnailWorkspace is shown when entering overview,
connecting to the same signal that creates the thumbnails, the showing
signal of overview, but, to make the slide animation we need to know how
much width the slider has.  To do that we ask the thumbnailsWorkspace
about its width, but given that it connects to the same signal it could
ask the width without having created the thumbnails yet, so reporting a
width of 0 and confusing the slide animation.

Currently it works because gjs calls the callbacks following the order
of the clients connecting that signal, and the thumbnailsWorskpace is
connected before the slide ones.

To avoid that we allow to request the preferred size of the
thumbnailsBox at any time with any number of thumbnails. The only thing
required is to make sure the porthole is accessible when requesting the
preferred size.

Created attachment 282762 [details] [review]
overviewControls: don't override explicit calls to slideIn

Currently we are overriding the explicit calls to slideIn
given that it's called also with the signal of showing overview.

It was necesary because of the bug that previous patch fixed,
so now we can just delete that.

Created attachment 282763 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from others pages
like apps page or search page since the windows comes from nowhere
with a initial position not known for the user.

Instead of that just fade the desktop with the windows in its
original position.

Review of attachment 282760 [details] [review]:

LGTM (the reseting => resetting typo is still there)

Review of attachment 282761 [details] [review]:

Caching the porthole for the time we show the workspace switcher looks reasonable to me (and is consistent with the current code). Caching it indefinitely not so much - _destroyThumbnails() looks like a good place to invalidate the porthole.

::: js/ui/workspaceThumbnail.js
@@ +935,3 @@
             let metaWorkspace = global.screen.get_workspace_by_index(k);
             let thumbnail = new WorkspaceThumbnail(metaWorkspace);
+            this._ensurePorthole();

It doesn't really make sense to call this once for each workspace we create, given that we know that we'll return early after the first time - so please move this outside the loop.

Review of attachment 282762 [details] [review]:

Assuming no code changes - missed in the last review: necesary => necessary in the commit message

Review of attachment 282763 [details] [review]:

The handling of minimized windows is wrong. Obligatory commit message nits: "others pages" => "other pages", "a initial position" => "an initial position" and "the windows comes" => "the window come", "not known for" => "not known to"

::: js/ui/viewSelector.js
@@ +264,3 @@
+            function() {
+                // If we were animating from the desktop view to the
+                // apps page the workspace page were visible, allowing

was visible

@@ +273,3 @@
+        Main.overview.connect('hidden', Lang.bind(this,
+            function() {
+                this._activePage = null;

Why do we need this?

@@ +339,3 @@
+        let showingWindowsFromOverview = this._activePage == this._workspacesPage;
+        if (!showingWindowsFromOverview)
+            this._showAppsButton.checked = false;

Fixing StButton:checked notify handling to make it possible to remove this conditional would make for a nice cleanup, but ok.

::: js/ui/workspace.js
@@ +1567,3 @@
 
+    fadeToOverview: function() {
+        // Unlikely with current code, but make sure

Uhm - <super>End, <super>A?

@@ +1576,3 @@
+        if (this._repositionWindowsId > 0) {
+            Mainloop.source_remove(this._repositionWindowsId);
+            this._repositionWindowsId = 0;

Mmh, can this really happen?

@@ +1582,3 @@
+        // to animate windows below in the stack
+        let firstMaximizedWindow = this._windows.length - 1;
+        let maximizedWindow = false;

maximizedWindow looks mostly unused, and I'd prefer the initialization of firstMaximizedWindow in the loop:

let firstMaximizedWindow;
for (firstMaximizedWindow = this._windows.length - 1; firstMaximizedWindow >= 0; firstMaximizedWindow--)
    if (maximized_horizontally && maximized_vertically)
        break;

This would also avoid having to increment firstMaximizedWindow after the loop, which confused me for a bit.

(Also "first" is a bit ambiguous - "top" is slightly clearer IMHO)

@@ +1589,3 @@
+        firstMaximizedWindow++;
+
+        let nTimeSlots = Math.min(WINDOW_ANIMATION_MAX_NUMBER_BLENDING, this._windows.length - firstMaximizedWindow);

No more MAX_NUMBER_BLENDING + 1?

@@ +1593,3 @@
+
+        // Animate the less recent accessed windows at the same time
+        // with one time slot

"least recent" - also that's technically not correct: the windows are in stacking order, which does not necessarily match the order in which windows where accessed (think of always-on-top windows)

@@ +1595,3 @@
+        // with one time slot
+        for (let i = this._windows.length - 1 - nTimeSlots; i >= 0; i--) {
+            if (i < firstMaximizedWindow) {

This is a bit odd: there are three cases (windows below the top-most maximized windows, windows animated in parallel and the top most windows animated with an offset) handled in two loops. I think just using a single loop would actually be more obvious.

How about:

let topIndex = this._windows.length - 1;
for (let i = 0; i < this._windows.length; i++) {
    if (i < firstMaximizedWindow) {
        // below top-most maximized window, don't animate
    } else {
        let onComplete = null;
        if (i == topIndex)
            onComplete = Lang.bind(this, ...);

        let time;
        let fromTop = topIndex - i;
        if (fromTop < nTimeSlots) // animate top-most windows gradually
            time = windowBaseTime * (nTimeSlots - fromTop);
         else
             time = windowBaseTime;

        this._windows[i].actor.opacity = 255;
        this._fadeWindow(i, time, 0, onComplete);

    }

@@ +1601,3 @@
+                this._windows[i].actor.opacity = 0;
+            } else {
+                this._windows[i].actor.opacity = 255;

Unobvious why you set the opacity here, but not for the top-most windows

@@ +1645,3 @@
+
+        // Special case maximized windows, since it doesn't make sense
+        // to animate windows below in the stack

There's a fair share of code here that could be shared with fadeToOverview

@@ +1665,3 @@
+                    this._windowOverlays[i].hide();
+            } else {
+                this._fadeWindow(i, windowBaseTime * nTimeSlots, 255, null);

Grrr, times used are different - that means less code-sharing :-(

Still, a cleanup similar to the one suggested above would be helpful.

(I'm undecided whether (windowBaseTime * nTimeSlots) or Overview.ANIMATION_TIME is more obvious ...)

@@ +1672,3 @@
+        // those covers most of the screen so we can achieve a smooth transition.
+        for (let i = this._windows.length - 1; i > this._windows.length - 1 - nTimeSlots; i--) {
+            let reverseIndex = this._windows.length - i;

I'd prefer something more meaningful than "reverseIndex", like "fromTop" as suggested above; and if you use the same variable name in almost identical code, please don't make it subtly different - use (windowBaseTime * (reverseIndex + 1)) instead.

@@ +1695,3 @@
+                               opacity: opacity,
+                               transition: 'easeOutQuad',
+                               onComplete: Lang.bind(this, function() {

Tweener can actually deal with

  onComplete: onComplete

@@ +1705,3 @@
+            Mainloop.timeout_add_seconds(time, Lang.bind(this, function() {
+                if (onComplete)
+                    onComplete();

No. timeout_add_seconds() doesn't magically work with a finer granularity than 1s when you pass a floating point number. Also it's ugly to set up a timeout source that most likely doesn't do anything.
Something like:
  if (onComplete)
      Mainloop.timeout_add(time * 1000, onComplete);

(this still relies on an implicit return value, the nitpicky variant would be along the lines of:
  Mainloop.timeout_add(time * 1000,
      function() {
          onComplete();
          return GLib.SOURCE_REMOVE;
      });

Created attachment 282883 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from other pages
like apps page or search page since the windows come from nowhere
with an initial position not known to the user.

Instead of that just fade the desktop with the windows in its
original position.

Created attachment 282885 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from other pages
like apps page or search page since the windows come from nowhere
with an initial position not known to the user.

Instead of that just fade the desktop with the windows in its
original position.

Created attachment 282886 [details] [review]
workspaceThumbnails: allow requesting size at any time

The slide of thumbnailWorkspace is shown when entering overview,
connecting to the same signal that creates the thumbnails, the showing
signal of overview, but, to make the slide animation we need to know how
much width the slider has.  To do that we ask the thumbnailsWorkspace
about its width, but given that it connects to the same signal it could
ask the width without having created the thumbnails yet, so reporting a
width of 0 and confusing the slide animation.

Currently it works because gjs calls the callbacks following the order
of the clients connecting that signal, and the thumbnailsWorskpace is
connected before the slide ones.

To avoid that we allow to request the preferred size of the
thumbnailsBox at any time with any number of thumbnails. The only thing
required is to make sure the porthole is accessible when requesting the
preferred size.

Review of attachment 282886 [details] [review]:

OK

Review of attachment 282885 [details] [review]:

This looks *much* nicer than the previous version! Still needs-work due to the window titles flashing during the transition (see below).

::: js/ui/viewSelector.js
@@ +415,3 @@
     _onShowAppsButtonToggled: function() {
+        if (!Main.overview.visible)
+            return;

Maybe move this to _showPage()?
(This is the only place where _showPage() may be called while the overview is hidden, so either one is fine; just wondering whether "don't switch pages while hidden" is more obvious than "ignore show-apps-button toggles while hidden" ...)

::: js/ui/workspace.js
@@ +1568,3 @@
+    fadeToOverview: function() {
+        if (this._windows.length == 0)
+            return;

Not sure if anything can break if we don't set _animatingWindowFade in this case - incidentally, the special-casing could be removed entirely now:

windows.length == 0 => topMaximizedWindow == -1 (no maximized window obviously)
nTimeSlots == 1 (windows.length - topMaximizedWindow = 0 + 1)
windowBaseTime = Overview.ANIMATION_TIME / 1 (no more division by zero \o/)

@@ +1584,3 @@
+        }
+
+        let nTimeSlots = Math.min(WINDOW_ANIMATION_MAX_NUMBER_BLENDING + 1, this._windows.length - topMaximizedWindow);

Not a big deal, but we might end up with more time slots than windows (e.g. 2 non-maximized windows => 3 slots). Could be a good thing even, see comment above.

@@ +1644,3 @@
+            if (i < topMaximizedWindow) {
+                // below top-most maximized window, don't animate
+                this._windows[i].actor.opacity = 0;

You hide the corresponding overlay in _fadeWindow() which is not called in this case, so we end up with "ghost" labels flashing up during the transition.

@@ +1651,3 @@
+                    time = windowBaseTime * (fromTop + 1);
+                else
+                    time = windowBaseTime;

This was windowBaseTime * nTimeSlots in the previous version, which made more sense to me - if this change was intentional, I'm confused now :-)

Created attachment 282919 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from other pages
like apps page or search page since the windows come from nowhere
with an initial position not known to the user.

Instead of that just fade the desktop with the windows in its
original position.

Review of attachment 282919 [details] [review]:

::: js/ui/viewSelector.js
@@ +418,3 @@
     _onShowAppsButtonToggled: function() {
+        if (!Main.overview.visible)
+            return;

You made _showPage() a no-op while hidden, so having the same check here is a bit pointless

::: js/ui/workspace.js
@@ +1613,3 @@
+    fadeFromOverview: function() {
+        if (this._windows.length == 0)
+            return;

Same comment as previously for fadeToOverview - we miss setting leavingOverview here, so we can hit repositioning during the transition

Created attachment 282922 [details] [review]
workspace: Fade in instead of zoom to return desktop

The zooming animation of the windows looks nice when animating
from the workspace display page, but looks weird from other pages
like apps page or search page since the windows come from nowhere
with an initial position not known to the user.

Instead of that just fade the desktop with the windows in its
original position.

Review of attachment 282922 [details] [review]:

Minor comment left, just copy-paste that before pushing.

::: js/ui/workspace.js
@@ +1629,3 @@
+
+        if (this.metaWorkspace != null && this.metaWorkspace != global.screen.get_active_workspace())
+            return;

Sorry, missed that earlier - this condition ("don't waste time animating stuff that is hidden anyway") also makes sense in fadeToOverview.

Attachment 282760 [details] pushed as 5d12ab4 - viewSelector: Remove duplicate call to showPage
Attachment 282762 [details] pushed as 805b686 - overviewControls: don't override explicit calls to slideIn
Attachment 282886 [details] pushed as 101daf6 - workspaceThumbnails: allow requesting size at any time
Attachment 282922 [details] pushed as 687e1eb - workspace: Fade in instead of zoom to return desktop