Just an additional note, dropping the icon in the final (new) position should make the (alpha dimmed) icon from the old position zoom out disappearing while the a new one zoom in. So the dash will not change its size.

Created attachment 179934 [details] [review]
base-icon: Add an option to not show the label

Currently there is a serious problem with ellipsization in various
parts of the overview. While wrapping the label or giving it more
space may be appropriate approaches for the application view, neither
works very well for the dash - possibly the best option there is to
not show the label at all.
So add a constructor parameter to BaseIcon to allow hiding the
label.

Created attachment 179935 [details] [review]
dash: Hide item labels

With the current dash layout of a single column, nearly every icon
label ends up ellipsized, even at the largest allowed icon size.
Not showing any labels appears to be the cleanest approach in this
case, so disable them in the dash.

Created attachment 179936 [details] [review]
dash: Calculate icon size changes without reallocation

The current approach to adjust the icon size of dash items is rather
expensive: the size of each item is changed from largest to smallest,
until the height of the dash fits the maximum available height, so
quite some ClutterTextures are created and disposed.
A better approach is to calculate the required size beforehand and
only change the icon size when necessary.

Created attachment 179937 [details] [review]
dash: Take remove target into account for icon size

Previously the icon size was only adjusted due to changes in the list
of application icons displayed, not when showing or hiding the remove
target. As a result, the remove target could end up cut off, so take
this case into account and adjust the icon size when showing or hiding
the remove target.

Created attachment 179938 [details] [review]
dash: Don't empty dash on changes

When the list of applications in the dash changes, all items are
removed and new ones added. While this approach is nice and simple,
it does not work if we want to animate changes. So rather than
replacing the old list of applications with the new one, figure
out the changes and only apply those.

Created attachment 179939 [details] [review]
dash: Wrap items in a scale-aware container

Clutter container only take their children's size into account, but
not their scale. As we want the dash to change its size smoothly
when zooming items in/out, we wrap each item in a custom container
which does consider the child's scale.

Created attachment 179940 [details] [review]
dash: Animate item and size changes

In general, all changes in the shell interface should be backed
by animations to give the interface a more natural feel and provide
feedback of what's happening. Currently the dash violates that
principle, as items simply appear/disappear or change size abruptly,
so add animations for application list and icon size changes.

Created attachment 179941 [details] [review]
dash: Avoid "zoom" effect when first shown

The dash is created empty and the initial set of items is added
before it's shown for the first time. As the additions of items
is now animated, this results in a slightly odd effect when all
items zoom in at once. So special-case the first time _redisplay()
is called and skip animations in that case.

Created attachment 179942 [details] [review]
dash: Minor style fixes

- 1px border rather than 2
 - less padding around launchers
 - icon prelight was too bright, bring it down a notch

Based on an original patch by Jakub Steiner.

*** Bug 637206 has been marked as a duplicate of this bug. ***

Created attachment 179946 [details] [review]
dash: Animate item and size changes

Fix a stupid exception which sneaked in during cleanup.

Created attachment 179947 [details] [review]
dash: Avoid "zoom" effect when first shown

Reattaching to maintain patch order.

Created attachment 179948 [details] [review]
dash: Minor style fixes

Reattaching to maintain patch order.

Review of attachment 179934 [details] [review]:

I'm unclear why we'd create a hidden label, and get its size, etc. If there was API to show the label the inefficiency might be worth simplicity of not having to change the hierarchy, but it looks like it never changes after creation.

Review of attachment 179935 [details] [review]:

Looks good

Created attachment 180018 [details] [review]
base-icon: Add an option to not show the label

(In reply to comment #15)
> I'm unclear why we'd create a hidden label, and get its size, etc. If there was
> API to show the label the inefficiency might be worth simplicity of not having
> to change the hierarchy, but it looks like it never changes after creation.

OK.

Created attachment 180022 [details] [review]
dash: Animate item and size changes

Improve zoomIn()/zoomOut() methods in DashItemContainer by using getters/setters instead of monkey-patching.

Review of attachment 179936 [details] [review]:

Looks good, obviously some tricky issues about ordering and asynchronous resizing, in exactly how this is used.

::: js/ui/dash.js
@@ +226,3 @@
+
+        let [minHeight, natHeight] = this.actor.get_preferred_height(-1);
+        let diff = (this._maxHeight - natHeight) / iconChildren.length;

Wouldn't hurt to have a comment explaining this - something like

 // compute the amount of extra space (or missing space) we have per icon
 // with the current icon size

Created attachment 180025 [details] [review]
base-icon: Add an option to not show the label

Sorry for the noise, last update got the icon-with-label case wrong.

Review of attachment 179937 [details] [review]:

Codewise, looks fine. UI-wise weird to me, but I guess better than lapping off the screen.

I have been running this for a few days now and I noticed that sometimes the shell would just hang (but I can still move the cursor) and eat a HUGE amount of memory (6-9GB+). I don't have a reproducer so I was running it inside gdb and noticed the follwing:

(gdb) call gjs_dumpstack()
== Stack trace for context 0x233f2d0 ==
0 anonymous() ["/home/linux/gnome-shell/source/gnome-shell/js/ui/dash.js":503]
1 anonymous() ["/home/linux/gnome-shell/install/share/gjs-1.0/lang.js":110]
2 _runDeferredWork(workId = "2") ["/home/linux/gnome-shell/source/gnome-shell/js/ui/main.js":559]
3 _runAllDeferredWork() ["/home/linux/gnome-shell/source/gnome-shell/js/ui/main.js":568]
4 anonymous() ["/home/linux/gnome-shell/source/gnome-shell/js/ui/main.js":649]


The C trace isn't that interesting:

(gdb) bt

+ Trace 225865

The problem seems to be that the (while) loop in Dash._redisplay ends up being an infinite loop.

The odd thing is why is this even called even though I did not touch the dash?

That aside, it does not seem obvious to me why this would happen but there might be a logic error in this loop somewhere.

Probably it should just exit when oldApps equals newApps ?

(In reply to comment #22)
> Probably it should just exit when oldApps equals newApps ?

Well this won't help as we wouldn't recompute the icon size.

(In reply to comment #23)
> (In reply to comment #22)
> > Probably it should just exit when oldApps equals newApps ?
> 
> Well this won't help as we wouldn't recompute the icon size.

It already does that so must be something else ...

Happened again, sightly different trace but still the same function:

(gdb) bt

+ Trace 225866

(gdb) call gjs_dumpstack()
== Stack trace for context 0x1f97540 ==
0 anonymous() ["/home/linux/gnome-shell/source/gnome-shell/js/ui/dash.js":491]
1 anonymous() ["/home/linux/gnome-shell/install/share/gjs-1.0/lang.js":110]
2 _runDeferredWork(workId = "2") ["/home/linux/gnome-shell/source/gnome-shell/js/ui/main.js":559]
3 _runBeforeRedrawQueue() ["/home/linux/gnome-shell/source/gnome-shell/js/ui/main.js":574]
4 anonymous() ["/home/linux/gnome-shell/source/gnome-shell/js/ui/main.js":583]

So it seems that it isn't the loop but that we keep running _redisplay itself forever (reading the loop's code seems to agree with that as there is no case I can think of where it would keep looping), probably due to the "notifiy::height" signal.

Yet another bug:

1) Open enough apps to fill the dash to not shrink
2) Drag a window that is a favorite, the "remove item" shows up and the dash shrinks
3) After stopping the drag the dash does not resize to it previous size

Created attachment 180318 [details] [review]
dash: Skip animations while the overview is hidden

If a window is closed, the list of running applications may change
while the overview is hidden. Animating dash changes is pointless
in this case, so update the dash without animations in that case.

Review of attachment 179938 [details] [review]:

::: js/ui/dash.js
@@ +307,3 @@
+            // This part is a bit tricky - we express a move as adding and
+            // removing the item, the order of the operations depends on
+            // whether the item has been moved up or down

I think probably you need to explain the overall approach before the start of the loop. (And maybe a few examples worked out)

If the code is left like this, it's worth explaining that it works only for a single app being moved. In fact, as far a I can tell, something like just hangs

 ABCDE
 ABEDC

Which doesn't seem OK. I think there are basically two choices here:

 A) Try to modify this algorithm so it does something meaningful in that case; I think it might work to just make the if (...) { continue; } if (...) { continue } into an if (...) else { ... } ... if adding doesn't make obvious sense, and deleting doesn't make obvious sense, then might as well delete anyways.

 B) Try to do the "best possible thing" - that would probably be the "longest common subsequence". (http://en.wikipedia.org/wiki/Longest_common_subsequence_problem, or http://wordaligned.org/articles/longest-common-subsequence, which is more readable.)

If A) makes sense to you it probably best unless we end up spending forever on this code because we don't have a clear idea of what it is trying to achieve - that would be the advantage of an LCS algorithm - it has a clear goal.

@@ +313,3 @@
+            let addedApps = addedItems.map(function(item) {
+                return item.app;
+            });

Doing these from scratch in the loop is ugly (and apparently causes huge memory usage when stuck in the loop) - maybe just write a loop over removedActors when you need to compute alreadyRemoved.

@@ +316,3 @@
+
+            let movedUp = newApps[newIndex + 1] &&
+                          newApps[newIndex + 1] == oldApps[oldIndex];

This needs to be commented with something like:

 // A single app was inserted at this position newApps[newIndex]. Since
 // we know that it was already somewhere in the array, if we encountered
 // the insertion first then this app was moved to an earlier (higher)
 // position.

Except that that doesn't make sense ... because if it was deleted first
then we'd *still* hit this? -we'd hit alreadyRemoved as well,
but it movedUp would be true, but it would be moved down not up. So maybe this
should be something like "insertedHere" ?

@@ +340,3 @@
+        for (let i = 0; i < addedItems.length; i++) {
+            let removedBefore = removedActors.filter(function(actor) {
+                return children.indexOf(actor) <= addedItems[i].pos;

This isn't right? We are comparing a current index - children.indexOf(actor) with a final index? I think the right thing to do is in the proceeding to save something that isn't pos, but rather is newIndex + (number of items we've deleted so far).

Review of attachment 179939 [details] [review]:

In the commit message 'Clutter container only take' was probably meant to be 'Clutter containers only take'

Other than that, one comment.

::: js/ui/dash.js
@@ +91,3 @@
+        this.actor.add_actor(this._child);
+
+        this.actor._delegate = this._child._delegate;

Really, if you set actor._delegate at all, you should have 'this.actor._delegate = this' - the pairing of actor and delegate shouldn't be an ad-hoc thing different in different cases. Can we do without this?

Review of attachment 180025 [details] [review]:

this looks better, btut:

::: js/ui/iconGrid.js
@@ +61,3 @@
         let availHeight = box.y2 - box.y1;
 
+        let iconSize = availHeight - this._spacing;

spacing should be dependent on whether the label is showing, right?

@@ +65,2 @@
         let [iconMinHeight, iconNatHeight] = this._iconBin.get_preferred_height(-1);
+        let preferredHeight = this._spacing + iconNatHeight;

and here?

Review of attachment 180025 [details] [review]:

this looks better, but

::: js/ui/iconGrid.js
@@ +61,3 @@
         let availHeight = box.y2 - box.y1;
 
+        let iconSize = availHeight - this._spacing;

spacing should be dependent on whether the label is showing, right?

@@ +65,2 @@
         let [iconMinHeight, iconNatHeight] = this._iconBin.get_preferred_height(-1);
+        let preferredHeight = this._spacing + iconNatHeight;

and here?

Review of attachment 180025 [details] [review]:

this looks better, but

::: js/ui/iconGrid.js
@@ +61,3 @@
         let availHeight = box.y2 - box.y1;
 
+        let iconSize = availHeight - this._spacing;

spacing should be dependent on whether the label is showing, right?

@@ +65,2 @@
         let [iconMinHeight, iconNatHeight] = this._iconBin.get_preferred_height(-1);
+        let preferredHeight = this._spacing + iconNatHeight;

and here?

Review of attachment 180022 [details] [review]:

Mostly looks reasonable to me - comments below about missing comments. One issue that seems like a potential bug.

::: js/ui/dash.js
@@ +99,3 @@
+    },
+
+    zoomIn: function() {

zoomIn/zoomOut here are a bit odd to me since there's nothing like a telephoto effect going on. Something like animateIn/animateOutAndDestroy() might be a bit better.

@@ +306,2 @@
             this._adjustIconSize();
+            this._favRemoveTarget.actor.show();

Needs a brief comment explaining why you do this

@@ +426,3 @@
+
+            icon.icon.set_size(icon.icon.width * scale,
+                               icon.icon.height * scale);

Needs some explanatory comment

 // We immediately update the image based on the final size, then animate
 // it scaling to the final size. The intermediate state will involve
 // rescaled pixels but this won't be noticeable

or something like that

@@ +617,3 @@
+                    this._dragPlaceholder.actor.connect('destroy',
+                        Lang.bind(this, function() {
+                            this._oldDragPlaceholderAnimating = false;

what keeps you from having two old drag place holders animating at once and the first one to finish clears this inappropriately? Do you need to keep an array or count of old animating placeholders instead?

@@ +627,3 @@
+            let fadeIn = false;
+            if (this._dragPlaceholder)
+                this._dragPlaceholder.actor.destroy();

Move fadeIn = false into the if block, the logic here doesn't flow - the fadeIn = false initialization seems like just an attempt to save a line of code. Also some comment along the lines of "if the drag place holder already exists, we just move it, but if we are adding it and it will expand the total size, do that as an animation

Review of attachment 179947 [details] [review]:

::: js/ui/dash.js
@@ +536,3 @@
         this._adjustIconSize();
 
+        if (!this._shownInitially) {

a) Could use a // skip animations when first showing comment
b) it's hard to tell from the patch in isolation, but isn't the placement between hiding and showing favRemoteTarget weird? I guess it favRemoveTarget doesn't exist on initial show, but still doesn't read well.

Review of attachment 179948 [details] [review]:

Sure

Review of attachment 180318 [details] [review]:

OK

Created attachment 180513 [details] [review]
dash: Don't empty dash on changes

(In reply to comment #28)
> Review of attachment 179938 [details] [review]:
> 
> ::: js/ui/dash.js
> If the code is left like this, it's worth explaining that it works only for a
> single app being moved. In fact, as far a I can tell, something like just hangs
> 
>  ABCDE
>  ABEDC
> 
> Which doesn't seem OK.

Fixed. I checked both the above case and moving several items at once using gsettings; the assumption of only moving one app at a time is still there, but the consequences when it turns out wrong are much more acceptable (e.g. the animation might end up slightly wrong).


> If A) makes sense to you it probably best unless we end up spending forever on
> this code because we don't have a clear idea of what it is trying to achieve -
> that would be the advantage of an LCS algorithm - it has a clear goal.


> Doing these from scratch in the loop is ugly (and apparently causes huge memory
> usage when stuck in the loop) - maybe just write a loop over removedActors when
> you need to compute alreadyRemoved.

Now uses removedActors.reduce(), which should be fine as it does not return new objects?


> Except that that doesn't make sense ... because if it was deleted first
> then we'd *still* hit this? -we'd hit alreadyRemoved as well,
> but it movedUp would be true, but it would be moved down not up. So maybe this
> should be something like "insertedHere" ?

Hmm, yeah ... I've gone with "insertHere" now.


> This isn't right? We are comparing a current index - children.indexOf(actor)
> with a final index? I think the right thing to do is in the proceeding to save
> something that isn't pos, but rather is newIndex + (number of items we've
> deleted so far).

Done (though I still call it "pos").

(In reply to comment #19)
> Wouldn't hurt to have a comment explaining this - something like
> 
>  // compute the amount of extra space (or missing space) we have per icon
>  // with the current icon size

Added locally.

(In reply to comment #29)
> In the commit message 'Clutter container only take' was probably meant to be
> 'Clutter containers only take'

Gah! Fixed.

 
> Really, if you set actor._delegate at all, you should have
> 'this.actor._delegate = this' - the pairing of actor and delegate shouldn't be
> an ad-hoc thing different in different cases. Can we do without this?

We can, though it's getting a bit verbose. Basically I've added

this.actor._delegate = this;

to the container class and made the child property public; I then use fugly constructs like

actor._delegate.child._delegate.stuff

...

(In reply to comment #30)
> this looks better, but:
> spacing should be dependent on whether the label is showing, right?

Right. Fixed locally.

Review of attachment 180513 [details] [review]:

Looks good, one suggestion about a comment

::: js/ui/dash.js
@@ +287,3 @@
+        // time; when moving several items at once, everything will still
+        // end up at the right position, but there might be additional
+        // additions/removals (e.g. items "bubbling up/down").

bubbling up/down implies to me that you'd get multiple add and remove animations for the same launcher, which sounds problematical ... but that isn't actually possible, I'm pretty sure - if you want to explain 'additional additions/removals' more I might say something like "it might remove all the launchers and add them back in the new order even when a smaller set of additions and removals is possible" or something like that.

Created attachment 180516 [details] [review]
dash: Animate item and size changes

(In reply to comment #33)
> zoomIn/zoomOut here are a bit odd to me since there's nothing like a telephoto
> effect going on. Something like animateIn/animateOutAndDestroy() might be a bit
> better.

OK.


> @@ +306,2 @@
>              this._adjustIconSize();
> +            this._favRemoveTarget.actor.show();
> 
> Needs a brief comment explaining why you do this

Done. I've also slightly modified the condition for hiding the remove target to account for the cases where it should actually be taken into account for the icon size (e.g. when a running app is added during DND).


> @@ +426,3 @@
> +
> +            icon.icon.set_size(icon.icon.width * scale,
> +                               icon.icon.height * scale);
> 
> Needs some explanatory comment

Done.


> @@ +617,3 @@
> +                    this._dragPlaceholder.actor.connect('destroy',
> +                        Lang.bind(this, function() {
> +                            this._oldDragPlaceholderAnimating = false;
> 
> what keeps you from having two old drag place holders animating at once and the
> first one to finish clears this inappropriately? Do you need to keep an array
> or count of old animating placeholders instead?

Hmmm, yeah - replaced the boolean with a counter.


> @@ +627,3 @@
> +            let fadeIn = false;
> +            if (this._dragPlaceholder)
> +                this._dragPlaceholder.actor.destroy();
> 
> Move fadeIn = false into the if block, the logic here doesn't flow - the fadeIn
> = false initialization seems like just an attempt to save a line of code.

Yup, that was the intent. Fixed (with comment).

(In reply to comment #34)
> a) Could use a // skip animations when first showing comment

+        // Skip animations on first run when adding the initial set
+        // of items, to avoid all items zooming in at once


> b) it's hard to tell from the patch in isolation, but isn't the placement
> between hiding and showing favRemoteTarget weird? I guess it favRemoveTarget
> doesn't exist on initial show, but still doesn't read well.

Maybe. Moved it below the showing of the favRemoveTarget.

(In reply to comment #41)
> if you want to explain 'additional
> additions/removals' more I might say something like "it might remove all the
> launchers and add them back in the new order even when a smaller set of
> additions and removals is possible" or something like that.

Much better, thanks!

Review of attachment 180516 [details] [review]:

Looks fine

Attachment 179935 [details] pushed as 4d474e2 - dash: Hide item labels
Attachment 179936 [details] pushed as 5aab878 - dash: Calculate icon size changes without reallocation
Attachment 179937 [details] pushed as a0584b9 - dash: Take remove target into account for icon size
Attachment 179939 [details] pushed as 29e97a5 - dash: Wrap items in a scale-aware container
Attachment 179947 [details] pushed as 9bbf293 - dash: Avoid "zoom" effect when first shown
Attachment 179948 [details] pushed as d33958c - dash: Minor style fixes
Attachment 180025 [details] pushed as 8cf9b5e - base-icon: Add an option to not show the label
Attachment 180318 [details] pushed as 026f598 - dash: Skip animations while the overview is hidden
Attachment 180513 [details] pushed as 2b84554 - dash: Don't empty dash on changes
Attachment 180516 [details] pushed as d6020f1 - dash: Animate item and size changes