Created attachment 283300 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 283301 [details] [review]
viewSelector: Allow different animation types

Make viewSelector able to manage different kind of animations for pages.
This is necessary for the upcoming patch to animate AllView and
FrequentView.

Created attachment 283302 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

However animating the icons in iconGrid involves a few challenges due to
the current need of scaling icons on iconGrid on a call which uses
BEFORE_REDRAW.  We need to call animate() after the icons are already
scaled, that means however, calling after a BEFORE_REDRAW, so that
means, we have to call animate() in a call using BEFORE_REDRAW which
call a function using BEFORE_REDRAW as well, so we wait two frames to
make sure icons are scaled correctly.

Created attachment 283303 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.

Created attachment 283304 [details] [review]
Search: cleanup unused code

Created attachment 283305 [details] [review]
Search: Use AppIcon for gridResults

Currently GridResult and ListResult inherit from a base class named
SearchResult which creates a StButton as a base item.
Then GridResult put as a child of the StButton an AppIcon created on
provider.createResultObject. But, StButton lacks some features of
AppIcon like dnd, style, etc. So GridResult add those in the class and
also adds the style of the AppIcon on the theme.

Obviously that doesn't look very appealing, so instead of that, delete
the base class and provide AppIcon some API needed by
Search and then use directly AppIcon on GridResult.

Created attachment 283306 [details] [review]
Search: Animate new window of apps in AllView and FrequentView

Following design mockups, animate the icons on AllView and FrequentView
to zoom out when opening a new window of the app or when the app is not
running and the user execute it.

Created attachment 283307 [details] [review]
Search: animate icons on new window

Following design decision, animate icons on search page on new window.

Created attachment 283308 [details] [review]
Dash: Animate on new window

Following design decision animate dash icons on new window.

For the record, Florian is reviewing on private with me.

Created attachment 284868 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 284869 [details] [review]
viewSelector: Allow different animation types

Make viewSelector able to manage different kind of animations for pages.
This is necessary for the upcoming patch to animate AllView and
FrequentView.

Created attachment 284870 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

However animating the icons in iconGrid involves a few challenges due to
the current need of scaling icons on iconGrid on a call which uses
BEFORE_REDRAW.  We need to call animate() after the icons are already
scaled, that means however, calling after a BEFORE_REDRAW, so that
means, we have to call animate() in a call using BEFORE_REDRAW which
call a function using BEFORE_REDRAW as well, so we wait two frames to
make sure icons are scaled correctly.

Thanks Florian Müllner for the debugging work

Created attachment 284871 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.
Also make possible animating indicators between view changes as well.

Created attachment 284872 [details] [review]
appDisplay: Animate appIcon for new window of apps

Following design mockups, animate the icons on AllView, FrequentView,
Dash and Search to zoom out when opening a new window of the app or when
the app is not running and the user execute it.

Created attachment 284873 [details] [review]
search: Allow providers to return the complete result object

 This makes the existing createResultObject() match its name better
 and avoids oddities like nested StButtons in app search results.

Created attachment 284874 [details] [review]
search: Remove DND code from GridSearchResult

Providers that need drag-and-drop behavior can implement this via the
createResultObject() hook (as the app search provider already does), no
need to duplicate that code in the generic result objects
(ListSearchResult already does not implement DND).

Review of attachment 284873 [details] [review]:

::: js/ui/appDisplay.js
@@ +1684,1 @@
     },

ups

Created attachment 284885 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 284886 [details] [review]
viewSelector: Allow different animation types

Make viewSelector able to manage different kind of animations for pages.
This is necessary for the upcoming patch to animate AllView and
FrequentView.

Created attachment 284887 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

However animating the icons in iconGrid involves a few challenges due to
the current need of scaling icons on iconGrid on a call which uses
BEFORE_REDRAW.  We need to call animate() after the icons are already
scaled, that means however, calling after a BEFORE_REDRAW, so that
means, we have to call animate() in a call using BEFORE_REDRAW which
call a function using BEFORE_REDRAW as well, so we wait two frames to
make sure icons are scaled correctly.

Thanks Florian Müllner for the debugging work

Created attachment 284888 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.
Also make possible animating indicators between view changes as well.

Created attachment 284889 [details] [review]
appDisplay: Animate appIcon for new window of apps

Following design mockups, animate the icons on AllView, FrequentView,
Dash and Search to zoom out when opening a new window of the app or when
the app is not running and the user execute it.

Created attachment 284890 [details] [review]
search: Allow providers to return the complete result object

 This makes the existing createResultObject() match its name better
 and avoids oddities like nested StButtons in app search results.

Created attachment 284891 [details] [review]
search: Remove DND code from GridSearchResult

Providers that need drag-and-drop behavior can implement this via the
createResultObject() hook (as the app search provider already does), no
need to duplicate that code in the generic result objects
(ListSearchResult already does not implement DND).

Created attachment 284892 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

However animating the icons in iconGrid involves a few challenges due to
the current need of scaling icons on iconGrid on a call which uses
BEFORE_REDRAW.  We need to call animate() after the icons are already
scaled, that means however, calling after a BEFORE_REDRAW, so that
means, we have to call animate() in a call using BEFORE_REDRAW which
call a function using BEFORE_REDRAW as well, so we wait two frames to
make sure icons are scaled correctly.

Thanks Florian Müllner for the debugging work

Review of attachment 284885 [details] [review]:

::: js/ui/appDisplay.js
@@ +1370,3 @@
         this._boxPointer.hide(BoxPointer.PopupAnimation.FADE |
+                              BoxPointer.PopupAnimation.SLIDE,
+                              this._view.animateOut);

Left-over, there's no such function here or in a later patch (technically it works out OK as it resolves to 'undefined', so the "if (onComplete)" test in BoxPointer catches it)

::: js/ui/iconGrid.js
@@ +368,3 @@
+
+        for (let index = 0; index < actors.length; index++)
+            actors[index].opacity = 0;

You are making two assumptions here that are a bit unexpected:
 (1) any animation will work on clones rather than the actors themselves
 (2) any animation will restore the original actors' opacities when finished

Also treating the no-actors case the same as an ongoing animation looks a bit dangerous - if a consumer does something essential in the 'animation-done' handler, stuff breaks when the signal is never emitted.

So how about moving things around a bit like this:

_animationDone: function() {
    this._animating = false;
    this.emit('animation-done');
},

animate: function(animationType, animationDirection) {
    if (this._animating)
        return;

    this._animating = true;

    let actors = this._getChildrenToAnimate();
    if (actors.length == 0) {
        this._animationDone();
        return;
    }

    // switch statement goes here
    this._animatePulse(actors, animationDirection);
},

_animatePulse(actors, animationDirection) {
    for (let i = 0; i < actors.length; i++) {
        let actor = actors[i];

        actor.opacity = 0;

        // [...]

            onComplete: Lang.bind(this, function() {
                if (isLastActor)
                    this._animationDone();
                actor.opacity = 255;
            })
    }
},

@@ +375,3 @@
+                break;
+            default:
+                log("animation doesn't exist. Icongrid won\'t work");

A simple log message when breaking animations (because this._animating will never be unset once we hit this) feels a bit weak; either throw an exception, or handle the case more gracefully (e.g. call _animationDone() in addition to the message)

@@ +379,3 @@
+    },
+
+    _animatePulse: function(actors, animationDirection) {

animationDirection is not used at all, while the animation is clearly only an IN animation - it would be pointless to implement an OUT animation that we are not going to use, but you could do something like

if (animationDirection != AnimationDirection.IN) {
    this._animationDone();
    return;
}

to make this clear.

@@ +399,3 @@
+            Tweener.addTween(actorClone,
+                            { time: ANIMATION_TIME_IN / 4,
+                              transition: 'easeInOutQuad',

Have you tried 'easeOutBack'? It's not quite the same of course, but

  Tweener.addTween(actorClone,
                   { time: ANIMATION_TIME_IN,
                     transition: 'easeOutBack',
                     delay: delay,
                     scale_x: 1,
                     scale_y: 1 });

looks fairly close to the nested tweeners.

(If that's not an option, I'd like to have some variable like
  bounceUpTime = ANIMATION_TIME / 4;
to avoid repeating the magic /4 later)

Review of attachment 284886 [details] [review]:

I don't really get the point of the patch - code moves around, but it doesn't "make viewSelector able to manage different kinds of animations", and neither does it look "necessary for the upcoming patch". Some of the splits are just awkward (_hidePageAndSyncEmpty?) and unnecessary; what makes sense to me is:

  (1) rename _fadePageIn to _animateIn (because there will be non-fade animations in the future)
  (2) add _animateOut (analogous to _animateIn)
  (3) factor out the tweens into _fadePageIn/_fadePageOut

If you keep the patch separated (rather than squashing it with the swarm one), then the commit message should reflect better what the patch does as well.

Review of attachment 284888 [details] [review]:

::: js/ui/appDisplay.js
@@ +192,3 @@
+    },
+
+    animateSwitch: function(active) {

Now that we have IconGrid.AnimationDirection, using that instead of a boolean would make sense to me.

@@ +275,3 @@
+
+        // Show them if necessary
+        this.animateIndicatorsIn();

Don't. The current code is perfectly clear and straight-forward - indicators should be shown when there are at least two of them, so update their visibility when their number changes. Your replacement is far from that - best proof of that is that you are getting it wrong:

  (1) when the page number changes from >1 to >1 while the all-view is visible,
      the indicators disappear and then animate in again with the new number

  (2) when the page number changes from 2 to 1 while the all-view is visible,
      the second indicator disappear while the first one sticks around until
      the next time the indicators are hidden (then it will just disappear without
      animation)

@@ +328,3 @@
+            offset = children[0].width;
+
+        let delay = INDICATORS_ANIMATION_DELAY;

Shouldn't this be INDICATORS_ANIMATION_DELAY_OUT?

In general, the two functions look almost identical, so can we have animateIndicators(animationDirection) instead?

@@ +536,3 @@
         if (animationDirection == IconGrid.AnimationDirection.IN) {
+            if (this._grid.actor.mapped)
+                this._pageIndicators.animateIndicatorsIn();

This is a bit mysterious ...

Review of attachment 284889 [details] [review]:

::: js/ui/appDisplay.js
@@ +1751,3 @@
     },
 
+    activate: function (button) {

The name change here looks accidental - this patch should come after the createResultObject() patch, and the relevant bits from this patch should be moved there.

@@ +1769,3 @@
     },
 
+    animateOut: function() {

Not convinced by the name - everywhere else you use "animateOut" for hide animations, but this isn't really one of those. animateLaunch() maybe?

::: js/ui/iconGrid.js
@@ +201,3 @@
+        // Animate only the container box instead of the entire actor,
+        // so the styles like hover and running are not applied while
+        // animating.

Hmprf, this is a bit accidental (e.g. having the style on .overview-icon rather than .app-well-icon or ".overview-icon > ShellGenericContainer"). You could use actorZoomOut(this.actor.child) though, which is a bit more robust to hierarchy changes (not that I'd expect much there) and allow you to not keep a reference to the container

@@ +210,3 @@
+};
+
+function actorZoomOut(actor) {

Odd name again - zoomOutActor() sounds more natural, but is still a bit misleading: it operates on a clone rather than the actor, and it fades in addition to the zoom ...

@@ +226,3 @@
+    let scaledWidth = width * APPICON_ANIMATION_OUT_SCALE;
+    let scaledHeight = height * APPICON_ANIMATION_OUT_SCALE;
+    // Assume pivot point at 0.5, 0.5

Confusing comment - I don't see why the pivot point of actor should matter, and for actorClone you don't have to assume anything (you set it just a couple of lines above).

Review of attachment 284890 [details] [review]:

When I said I wasn't entirely happy with my patch, I didn't mean you should break it completely ;-)

::: js/ui/appDisplay.js
@@ +23,3 @@
 const OverviewControls = imports.ui.overviewControls;
 const PopupMenu = imports.ui.popupMenu;
+const Search = imports.ui.search;

This was needed to inherit from Search.SearchResult; you no longer do that, so the import is now unused

@@ +1111,3 @@
+        let appIcon = new AppIcon(app);
+        appIcon.metaInfo = resultMeta;
+        appIcon.provider = this;

Doesn't really seem useful (in particular with AppIcon not being a SearchResult) - either remove it altogether, or move those bits to _createResultDisplay().

@@ +1761,3 @@
     },
 
+    setSelected: function(selected) {

I know it's not a lot of code, but "objects returned by createResultObject() should implement a setSelected() method that looks exactly like this, so please copy it" is just meh :-(

So if you don't like inheritance here, how about something like:

_selectResult: function(result, selected) {
    if (!result)
        return;

    if (selected) {
        result.actor.add_style_pseudo_class('selected');
        Util.ensureActorVisibleInScrollView(this._scrollView, result.actor);
    } else {
        result.actor.remove_style_pseudo_class('selected');
    }
}

in SearchResults?

::: js/ui/search.js
@@ +302,3 @@
+    _createResultDisplay: function(meta) {
+        if (this.provider.createResultObject)
+            return result;

After removing half the function, this code is completely pointless - if the provider implements createResultObject, return undefined, else null ...

@@ +320,3 @@
     _activateResult: function(result, id) {
+        if (this.provider.createResultObject) {
+            result.activate();

This doesn't make sense - _activateResult() is called when result.activate() emits ::activate; so this only works when the custom result object does *not* emit the signal (in which case _activateResult() is not called at all), otherwise you end up trapped in a loop.

Review of attachment 284891 [details] [review]:

This should be OK after cleaning up the mess in the other patch.

Review of attachment 284892 [details] [review]:

::: js/ui/appDisplay.js
@@ +458,3 @@
     },
 
+    animate: function(animationDirection, onCompleteOut) {

It's a bit icky to have three classes derived from BaseAppView using two different function signatures for animate()

@@ +463,3 @@
+        let [centerDashPositionX, centerDashPositionY] = [dashX + dashWidth / 2, dashY + dashHeight / 2];
+        // Design decision, 1/2 of the dash icon size.
+        let [dashScaledWidth, dashScaledHeight] = [dashWidth / 2, dashHeight / 2];

There must be some way to improve code sharing here

@@ +492,3 @@
+                    this._grid.disconnect(animationDoneId);
+                    if (onCompleteOut)
+                        onCompleteOut();

There isn't really any reason for ignoring the "onComplete" callback for AnimationDirection.IN other than "we don't need this currently"; it's better to not have parameters interact in obscure ways like this, in particular where it's that easy to avoid: just move those lines outside the if-else block.

@@ +930,3 @@
+                               // show the view
+                               if (animationDirection == IconGrid.AnimationDirection.OUT)
+                                   this._controls.opacity = 255;

Yikes - so we don't end up flashing the controls at the end of the animation because the out animation based on ANIMATION_TIME_OUT should always be faster than ANIMATION_TIME_IN, so the parent is already hidden when we hit this? Is there anything less fragile we can do here?

::: js/ui/iconGrid.js
@@ +366,3 @@
     },
 
+    animate: function(animationType, animationDirection, params) {

I do wonder whether animatePulse(direction) and animateString(direction, params) would be better - the shared code is just a bit of boilerplace, and you would avoid the awkward type-params interaction. Or pass the params parameter on to the implementations, like this._animatePulse(direction, params) / this._animateSpring(direction, params)

@@ +373,3 @@
+                                        sourceHeight: null });
+
+        let actors = this._getChildrenToAnimate(params);

I don't like this - so if the non-paginated icon grid also started requiring a special parameter, consumers would need to pass in two separate parameters to make both implementations of getChildrenToAnimate() happy? Maybe we should just "teach" PaginatedIconGrid about the current page? (Could be as quick-n-dirty as replacing this._currentPage with this._grid.currentPage in AllView - not too nice either, but I'd still prefer this to the layer violation here)

@@ +440,3 @@
+        let distances = actors.map(Lang.bind(this, function(actor) {
+            let [actorX, actorY] = actor.get_transformed_position();
+            return this._distance(actorX, actorY, sourceX, sourceY);

I'd just write this as
  let [x, y] = [actorX - sourceX, actorY - sourceY];
  return Math.sqrt(x * x, y * y);

Doesn't seem worth splitting out ...

@@ +457,3 @@
+            let scaleY = sourceHeight / height;
+            // Center the actor on the source position, expecting
+            // sourcePosition to be the center where the actor should

Maybe it's better to pass in the sourceActor itself rather than the individual parameters? Then you wouldn't need to make any assumptions at all, just take the position/size you want ...

@@ +814,3 @@
 
+    _getChildrenToAnimate: function(params) {
+        return this._getChildrenInPage(params.page);

Why the split?

@@ +877,3 @@
+        let lastIndex = firstIndex + this._childrenPerPage;
+
+        return children.slice(firstIndex, Math.min(lastIndex, children.length));

From the docs[0]:
"If end is greater than the last index of the array, slice extracts to the end of the sequence."

So children.slice(firstIndex, lastIndex) will always work correctly.

[0] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/slice

::: js/ui/viewSelector.js
@@ +396,3 @@
+            // Restore opacity of the page, since we could took the
+            // opacity on _fadePageIn if we didn't use appDisplay custom
+            // animation to animate out.

// Restore opacity, in case we animated out via _fadePageOut

How close are we to landing this ? From a release team perspective, I would say that the window probably closes with .91

Review of attachment 284892 [details] [review]:

::: js/ui/appDisplay.js
@@ +458,3 @@
     },
 
+    animate: function(animationDirection, onCompleteOut) {

AllView and FrequentView sadly has completely different needs... Not sure I can merge both of them

::: js/ui/iconGrid.js
@@ +814,3 @@
 
+    _getChildrenToAnimate: function(params) {
+        return this._getChildrenInPage(params.page);

because it's a override from IconGrid. Not sure I understand your question.

(In reply to comment #35)
> ::: js/ui/iconGrid.js
> @@ +814,3 @@
> 
> +    _getChildrenToAnimate: function(params) {
> +        return this._getChildrenInPage(params.page);
> 
> because it's a override from IconGrid. Not sure I understand your question.

The question was about _getChildrenInPage, which isn't used anywhere else.

(In reply to comment #36)
> (In reply to comment #35)
> > ::: js/ui/iconGrid.js
> > @@ +814,3 @@
> > 
> > +    _getChildrenToAnimate: function(params) {
> > +        return this._getChildrenInPage(params.page);
> > 
> > because it's a override from IconGrid. Not sure I understand your question.
> 
> The question was about _getChildrenInPage, which isn't used anywhere else.

oh ok

Created attachment 284958 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 284959 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

Thanks Florian Müllner for the debugging work

Created attachment 284960 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.
Also make possible animating indicators between view changes as well.

Review of attachment 284958 [details] [review]:

::: js/ui/appDisplay.js
@@ +965,3 @@
     },
 
+    animate: function(animationDirection) {

I'm undecided whether it makes sense to add an unused onComplete parameter for consistency with BaseAppView.animate in the follow-up patch.

@@ +1345,3 @@
         this._boxPointer.setArrowActor(this._source.actor);
+        // We need to hide the icons of the view until the boxpointer animation
+        // is completed so we can animate the icons after as we like withouth

*without

The comment could be more compact, sth like:
 // Hide icons while the folder is opening, animate them after completion

@@ +1353,3 @@
+            function() {
+                // Restore the view opacity, so now we show the icons and animate
+                // them

Gratuitous comment - it's just a wordy variant of "opacity = 255; view.animate();" :-)

::: js/ui/iconGrid.js
@@ +396,3 @@
+            let bounceUpTime = ANIMATION_TIME_IN / 4;
+            // Defeat onComplete anonymous function closure
+            let isLastItem= index == actors.length - 1;

Missing space before =

Review of attachment 284960 [details] [review]:

::: js/ui/appDisplay.js
@@ +361,3 @@
+            delay -= (totalAnimationTime - INDICATORS_ANIMATION_MAX_TIME_OUT) / this._nPages;
+
+        let baseTime = isAnimationIn ? INDICATORS_BASE_TIME : INDICATORS_BASE_TIME_OUT;

I would move this up and have maxTime as well, and then use those instead of special-casing all the computations:

let totalAnimationTime = baseTime + delay * this._nPages;
if (totalAnimationTime > maxTime)
  delay -= (totalAnimationTime - maxTime) / this._nPages;

@@ -323,2 @@
         for (let i = 0; i < this._nPages; i++) {
-            children[i].translation_x = offset;

This breaks the first IN animation, no?

@@ +573,3 @@
+
+        if (animationDirection == IconGrid.AnimationDirection.IN)
+            this._pageIndicators.animateIndicators(IconGrid.AnimationDirection.IN);

This is silly :-)

Also: we end up calling animateIndicators twice now (here and in notify::mapped) - do we need to guard against that?

Review of attachment 284959 [details] [review]:

::: js/ui/appDisplay.js
@@ +213,3 @@
+            // Some subclasses may want to wait for some event to
+            // animate out
+            this._mayDelayedAnimateOut(gridAnimationFunction);

Not a big fan - how about:

_doSpringAnimation: function(direction) {
    this._grid.actor.opacity = 255;
    this._grid.animateSpring(direction,
                             Main.overview.getShowAppsButton());
},

_animate: function(direction, onComplete) {
    if (direction == IN) {
        // [...]
    } else {
        this._doSpringAnimation(direction);
    }

    if (onComplete) {
        let id = this._grid.connect('animation-done', Lang.bind(this,
            function() {
                this._grid.disconnect(id);
                onComplete();
            }));
    }
},

AllView:
_animate: function(direction, onComplete) {
    if (direction == OUT &&
        this._displayingPopup && this._currentPopup) {
        this._currentPopup.popdown();
        let spaceClosedId = this._grid.connect('space-closed', Lang.bind(this,
            function() {
                this._grid.disconnect(spaceClosedId);
                this.parent(direction, onComplete);
            }));
    } else {
        this.parent(direction, onComplete);
    }
}

@@ +900,3 @@
+                        this._restoreControlsOpacity = true;
+                });
+        currentView.connect('animation-done', restoreControlsOpacityFunction);

You are adding a new signal connection with each animation (and never disconnecting); in any case this looks cumbersome, how about:

this._controls.connect('notify::mapped', Lang.bind(this,
    function() {
        // controls are faded either with their parent or explicitly
        // in animate(); we can't now how they'll be shown next, so
        // make sure to restore their opacity when they are hidden
        if (!this._controls.mapped)
            this._controls.opacity = 255;
    });

::: js/ui/iconGrid.js
@@ +446,3 @@
+
+        let distances = actors.map(Lang.bind(this, function(actor) {
+            let [actorX, actorY] = actor.get_transformed_position();

get_transformed_position() is a bit more expensive than a simple property read, so might be worth calling it only once per actor:

actors.forEach(function(a) {
    a._transformedPosition = a.get_transformed_position();
}

While at it, you could also use monkey-patching to attach the distance to the corresponding actor to avoid the slightly awkward two-arrays-connected-by-index pattern:

actors.forEach(function(a) {
    let [actorX, actorY] = a._transformedPosition = a.get_transformed_position();
    let [x, y] = [actorX - sourceX, actorY - sourceY]
    a._distance = Math.sqrt(x * x, y * y);
});
let maxDist = actors.reduce(function(prev, cur) { return Math.max(prev, cur._distance); }, 0);
let minDist = actors.reduce(function(prev, cur) { return Math.min(prev, cur._distance); }, Infinity);

@@ +459,3 @@
+
+            let actorClone = new Clutter.Clone({ source: actors[index],
+                                                 reactive: false });

ClutterActors are non-reactive by default

@@ +462,3 @@
+            Main.uiGroup.add_actor(actorClone);
+
+            actorClone.set_pivot_point(0.0, 0.0);

(0,0) is already the default

@@ +467,3 @@
+            let scaleX = sourceWidth / width;
+            let scaleY = sourceHeight / height;
+            let [adjustedSourcePositionX, adjustedSourcePositionY] = [sourceX - sourceWidth / 2, sourceY - sourceHeight / 2];

All the source{X,Y,Width,Height} is a bit confusing, as they change meaning: (sourceX, sourceY) starts as the source's position in stage coordinates, then becomes the source's center in stage coordinates, sourceWidth is now actually sourceWidth / 2 ...
So how about: (sourceX, sourceY) : source position in stage coordinates
              (sourceCenterX, sourceCenterY) : source center in stage coordinates
              (sourceWidth, sourceHeight) : source size
              (scaledWidth, scaledHeight) : adjusted size
              (cloneSourceX, cloneSourceY) : clone start/target position (i.e. over the source)

::: js/ui/viewSelector.js
@@ +388,3 @@
+                           onComplete: Lang.bind(this, function() {
+                               page.hide();
+                               this.emit('page-empty');

I'd leave this in _animateIn() (like _fadePageIn() does now) - it's a bit unexpected (considering the name), but overviewControls strongly depends on this signal being emitted before animating the new page, and relying on people remembering to emit it before calling _animateIn() is rather fragile (the same code is now replicated three times)

@@ +437,3 @@
 
+        if (oldPage) {
+            this._animateOut(oldPage, page)

I wasn't a big fan of the old "fadePageIn(notThePageFadedIn)" code, but the new page, newPage, oldPage isn't really that much clearer (in particular as "page" changes its meaning, e.g. "_animateOut(oldPage, page) => _animateOut(page, newPage)"); maybe keep using this._activePage like the current code isn't that bad after all ...

Review of attachment 284959 [details] [review]:

::: js/ui/appDisplay.js
@@ +201,3 @@
+            let toAnimate = this._grid.actor.connect('notify::allocation', Lang.bind(this,
+                function() {
+                    if (this._grid.actor.mapped) {

I forgot to ask - when would this ever happen (an allocation change notification for an unmapped actor)?

Review of attachment 284959 [details] [review]:

::: js/ui/appDisplay.js
@@ +201,3 @@
+            let toAnimate = this._grid.actor.connect('notify::allocation', Lang.bind(this,
+                function() {
+        let sourceActor = Main.overview.getShowAppsButton();

Yes, there's allocations although the actor is not mapped. For what I know, that's normal.
You can see it for example going to overview (not to apps directly) and there's multiple allocations of AllView and FrequentView.

Review of attachment 284960 [details] [review]:

::: js/ui/appDisplay.js
@@ +573,3 @@
+
+        if (animationDirection == IconGrid.AnimationDirection.IN)
+                             { time: VIEWS_SWITCH_TIME,

oh my god...

Created attachment 284997 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 284998 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

Thanks Florian Müllner for the debugging work

Created attachment 284999 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.
Also make possible animating indicators between view changes as well.

Review of attachment 284889 [details] [review]:

::: js/ui/appDisplay.js
@@ +1751,3 @@
     },
 
+    activate: function (button) {

it's "accidental". You could rename that to something else. This patch works on its own.
Not sure I understood your review.

::: js/ui/iconGrid.js
@@ +210,3 @@
+};
+
+function actorZoomOut(actor) {

right. Do you have a better proposal here? zoomOutFadeWithClone()....?

Review of attachment 284890 [details] [review]:

::: js/ui/search.js
@@ +302,3 @@
+    _createResultDisplay: function(meta) {
+        if (this.provider.createResultObject)
+            return result;

ups....sorry. I don't even know why it was working :\
Probably I messed with vim after testing.

Review of attachment 284997 [details] [review]:

Only very minor nits at this point - good to push with those fixed

::: js/ui/appDisplay.js
@@ +965,3 @@
     },
 
+    animate: function(animationDirection, onComplete) {

Sorry, by "unused" I meant "not used by the consumer" - if there is an onComplete parameter, it should work when provided, e.g. either remove it or add:

  if (onComplete) {
    let id = this._grid.connect('animation-done', Lang.bind(this,
       function() {
           this._grid.disconnect(id);
           onComplete();
        }));
  }

::: js/ui/iconGrid.js
@@ +379,2 @@
+        for (let index = 0; index < actors.length; index++) {
+            let actor= actors[index];

Another missing space I missed earlier

@@ +386,3 @@
+
+            let actorClone = new Clutter.Clone({ source: actors[index],
+                                                 reactive: false });

Unneeded property (reactive is false by default)

Review of attachment 284998 [details] [review]:

Some more nits, but nothing really major either ...

::: js/ui/appDisplay.js
@@ +854,3 @@
+                // shown next, so make sure to restore their opacity
+                // when they are hidden
+                Tweener.removeTweens(this._controls);

It shouldn't matter in practice, but this breaks code like:

  controls.opacity = 0;
  Tweener.addTween(controls, { time: 1, opacity: 255 });
  controls.show();

We usually call show() before setting up the animation, but you should still only removeTweens() when you actually change the opacity:

  if (this._controls.mapped)
      return;
  Tweener.removeTweens(this._controls);
  this._controls.opacity = 255;

::: js/ui/iconGrid.js
@@ +462,3 @@
+            actor.opacity = 0;
+
+            let actorClone = new Clutter.Clone({ source: actors[index] });

Nit: you already have "actor = actors[index]" above, so you can actually use that variable throughout the block

@@ +469,3 @@
+            let scaleX = sourceScaledWidth / width;
+            let scaleY = sourceScaledHeight / height;
+            let [adjustedSourcePositionX, adjustedSourcePositionY] = [sourceCenterX - sourceScaledWidth / 2, sourceY - sourceScaledHeight / 2];

"adjusted source position" doesn't really suggest "clone's start/target position", but I can't really think of anything good either ...

@@ +482,3 @@
+
+                let delay = (1 - (actors[index]._distance - minDist) / normalization) * ANIMATION_MAX_DELAY_FOR_ITEM;
+                let [finalX, finalY]  = actors[index].get_transformed_position();

The point of caching the transformed position above is to not call the function again:

let [finalX, finalY] = actor._transformedPosition;

@@ +502,3 @@
+                               opacity: 255 };
+            } else {
+                let [startX, startY]  = actors[index].get_transformed_position();

Dto.

::: js/ui/viewSelector.js
@@ +387,3 @@
+                           transition: 'easeOutQuad',
+                           onComplete: Lang.bind(this, function() {
+                               page.hide();

Same as with emit('page-empty') really - if there is a previous page, it *needs* to be hidden; so I prefer the old

 if (oldPage)
    oldPage.hide();

  this.emit('page-empty');

in _fadePageIn() ...

@@ +388,3 @@
+                           onComplete: Lang.bind(this, function() {
+                               page.hide();
+                               this._animateIn(page);

Mmmh, "animateIn(page)" reads more as "animate @page" to me than the "animateIn(oldPage)" used elsewhere - does it make sense to rename the parameter to "oldPage", or maybe add "let oldPage = page;" at the top and use "page" in the animation and "oldPage" as animateIn parameter?

@@ +416,3 @@
+
+            this.appDisplay.animate(IconGrid.AnimationDirection.OUT,
+                                    onComplete);

This is purely stylistic, but I'd prefer this as

  this.appDisplay.animate(IconGrid.AnimationDirection.OUT, Lang.bind(this,
      function() {
          this._animateIn(page);
      }));

That way animateOut appears before animateIn in the code, i.e. the order in which the animations are actually done

Review of attachment 284999 [details] [review]:

OK

::: js/ui/appDisplay.js
@@ +353,3 @@
         for (let i = 0; i < this._nPages; i++) {
+            if (isAnimationIn)
+                children[i].translation_x = offset;

Or "children[i].translation_x = isAnimationIn ? offset : 0;"?

@@ +551,3 @@
             this.parent(animationDirection, onComplete);
+            if (animationDirection == IconGrid.AnimationDirection.OUT)
+                this._pageIndicators.animateIndicators(animationDirection);

This is because IN is handled by notify::mapped? If so, the same should be done in animateSwitch(), no?

Review of attachment 284998 [details] [review]:

::: js/ui/iconGrid.js
@@ +482,3 @@
+
+                let delay = (1 - (actors[index]._distance - minDist) / normalization) * ANIMATION_MAX_DELAY_FOR_ITEM;
+        let minDist = actors.reduce(function(prev, cur) {

I messed with vim again I guess. I remember to do a search and replace :\

::: js/ui/viewSelector.js
@@ +387,3 @@
+                           transition: 'easeOutQuad',
+                           onComplete: Lang.bind(this, function() {
+                               page.hide();

agree, leftover of previous code.

@@ +388,3 @@
+                           onComplete: Lang.bind(this, function() {
+                               page.hide();
+                               this._animateIn(page);

yeah..as silly it sound, I will do that because it's confusing this way.

@@ +416,3 @@
+
+            this.appDisplay.animate(IconGrid.AnimationDirection.OUT,
+                                    onComplete);

I prefer that too. I probably overkill this as a leftover from my previous code.

Created attachment 285074 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 285075 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

Thanks Florian Müllner for the debugging work

Review of attachment 284999 [details] [review]:

::: js/ui/appDisplay.js
@@ +353,3 @@
         for (let i = 0; i < this._nPages; i++) {
+            if (isAnimationIn)
+                children[i].translation_x = offset;

I was trying to animate always from the current point if this is called in middle of an animation. But since no longer do that for "in" animation, you are right.

@@ +551,3 @@
             this.parent(animationDirection, onComplete);
+            if (animationDirection == IconGrid.AnimationDirection.OUT)
+                this._pageIndicators.animateIndicators(animationDirection);

yes

Created attachment 285076 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.
Also make possible animating indicators between view changes as well.

Review of attachment 285076 [details] [review]:

ok from before

Created attachment 285077 [details] [review]
appDisplay: Animate folder view items

Add a new animation  to folder view based on designers mockups that
emulates pulsating icons.
The code on iconGrid is though to work well for the upcoming patches to
animate AllView and FrequentView.

Created attachment 285078 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

Thanks Florian Müllner for the debugging work

Review of attachment 285074 [details] [review]:

LGTM

::: js/ui/iconGrid.js
@@ +382,3 @@
+
+            let delay = index / actors.length * ANIMATION_MAX_DELAY_FOR_ITEM;
+            let [originalX, originalY] = actors[index].get_transformed_position();

As in the other patch, you could use "actor" instead of "actors[index]" throughout the function

Review of attachment 285075 [details] [review]:

::: js/ui/appDisplay.js
@@ +856,3 @@
+
+                Tweener.removeTweens(this._controls);
+                if (!this._controls.mapped)

This will never be false due to the early return above

::: js/ui/iconGrid.js
@@ +464,3 @@
+            Main.uiGroup.add_actor(actorClone);
+
+            let [width, height,,] = this._getAllocatedChildSizeAndSpacing(actors[index]);

actor (couple more of those left below!)

::: js/ui/viewSelector.js
@@ +410,3 @@
+    },
+
+    _animateOut: function(page) {

As you added
  let oldPage = page;
in _fadePageOut, you should do the same here

@@ +418,1 @@
             });

onComplete is now unused

Created attachment 285079 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

Thanks Florian Müllner for the debugging work

Review of attachment 285077 [details] [review]:

ok with latest review applied

Created attachment 285081 [details] [review]
appDisplay: Animate AllView and FrequentView

Following design decision, we want to animate AllView and FrequentView
when opening and closing with a swarm spring form.

This involves a few changes needed to allow that, since from some time
now, we are animating page changes in viewSelector, using only a fade
transition. However now we want to let appDisplay and iconGrid apply its
own animation.

For that we special case the change to and from apps page on
viewSelector to let appDisplay to animate its own items, using and API
on appDisplay which at the same time uses an API on iconGrid.

Thanks Florian Müllner for the debugging work

Review of attachment 285076 [details] [review]:

::: js/ui/appDisplay.js
@@ +351,3 @@
         for (let i = 0; i < this._nPages; i++) {
+            if (isAnimationIn)
+                children[i].translation_x = isAnimationIn ? offset : 0;

The left-over "if" means you never hit the "translation_x = 0" case

Review of attachment 285081 [details] [review]:

LGTM

Created attachment 285083 [details] [review]
appDisplay: Animate indicators out

Given that we animate indicator in, it makes sense to animate them out
as well.
Also make possible animating indicators between view changes as well.

Attachment 285077 [details] pushed as 9c474a6 - appDisplay: Animate folder view items
Attachment 285081 [details] pushed as f160dda - appDisplay: Animate AllView and FrequentView
Attachment 285083 [details] pushed as 75d5e84 - appDisplay: Animate indicators out

(In reply to comment #50)
> +    activate: function (button) {
> 
> it's "accidental". You could rename that to something else. This patch works on
> its own.

No, it is not accidental. You will start using AppIcon as result object, which is expected to have a method called "activate"[0] (and not "frobnicate" or whatever). This is why I think this chunk makes more sense in the search patch (and should come before the launch animation one).


> +function actorZoomOut(actor) {
> 
> right. Do you have a better proposal here? zoomOutFadeWithClone()....?

doLaunchAnimationForActor()? Dunno, zoomActor() is probably good enough ...


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

Created attachment 285086 [details] [review]
search: Allow providers to return the complete result object

 This makes the existing createResultObject() match its name better
 and avoids oddities like nested StButtons in app search results.

Created attachment 285087 [details] [review]
appDisplay: Animate appIcon for new window of apps

Following design mockups, animate the icons on AllView, FrequentView,
Dash and Search to zoom out when opening a new window of the app or when
the app is not running and the user execute it.

Review of attachment 285086 [details] [review]:

::: js/ui/search.js
@@ +612,2 @@
         if (newDefaultResult != this._defaultResult) {
             if (this._defaultResult)

This test is now in _setSelected()

@@ +613,3 @@
             if (this._defaultResult)
+                this._setSelected(this._defaultResult, false);
+            if (newDefaultResult)

Dto.

@@ +651,3 @@
     highlightDefault: function(highlight) {
         this._highlightDefault = highlight;
         if (this._defaultResult) {

Dto.

@@ +655,2 @@
             if (highlight)
                 Util.ensureActorVisibleInScrollView(this._scrollView, this._defaultResult.actor);

Already done in _setSelected()

Review of attachment 285087 [details] [review]:

::: js/ui/search.js
@@ +719,3 @@
+    },
+
+    animateOut: function() {

I don't like the animateOut() name here either. I think animateLaunch() as in AppIcon is appropriate here as well, possibly not so much in BaseIcon - we only use it to implement launch animations, but it could theoretically be used elsewhere; so maybe use animateZoomOut() there to match the helper method?

Created attachment 285101 [details] [review]
search: Allow providers to return the complete result object

 This makes the existing createResultObject() match its name better
 and avoids oddities like nested StButtons in app search results.

Created attachment 285102 [details] [review]
appDisplay: Animate appIcon for new window of apps

Following design mockups, animate the icons on AllView, FrequentView,
Dash and Search to zoom out when opening a new window of the app or when
the app is not running and the user execute it.

Created attachment 285104 [details] [review]
appDisplay: Animate appIcon for new window of apps

Following design mockups, animate the icons on AllView, FrequentView,
Dash and Search to zoom out when opening a new window of the app or when
the app is not running and the user execute it.

Review of attachment 285101 [details] [review]:

OK

Review of attachment 285104 [details] [review]:

LGTM

Attachment 284891 [details] pushed as 3981b27 - search: Remove DND code from GridSearchResult
Attachment 285101 [details] pushed as 67c216a - search: Allow providers to return the complete result object
Attachment 285104 [details] pushed as 62786c0 - appDisplay: Animate appIcon for new window of apps