Created attachment 286683 [details] [review]
iconGrid: Use dynamic max delay for pulse animations

Currently animations delay for pulse animation are calculated based on
the number of actors. That bring a problem though, when there are only a
few actors the animations feels very slow.
To avoid that make the delay of the animation dynamic for the first
items and then use a maximum value on.

Review of attachment 286683 [details] [review]:

"To avoid that make the delay of the animation dynamic for the first items"

This is misleading:
 - the behavior is not different for the first items, but for small (total) number of items
 - it's not really dynamic: if the number of items is <= 4, you use a constant delay of 1/6 ANIMATION_TIME_IN

I do wonder whether it makes sense to just always pick a fixed delay - it means that the total length of the folder animation depends on the number of items in the folder, but that doesn't sound too crazy ...

::: js/ui/iconGrid.js
@@ +431,3 @@
         }
 
+        let totalDelay = clamp(ANIMATION_BASE_DELAY_FOR_ITEM * actors.length, 0, ANIMATION_MAX_DELAY_FOR_ITEM);

Why clamp? Math.min(ANIMATION_BASE-DELAY_FOR_ITEM * actors.length, ANIMATION_MAX_DELAY_FOR_ITEM) can never be negative ...

(In reply to comment #2)
> Review of attachment 286683 [details] [review]:
> 
> "To avoid that make the delay of the animation dynamic for the first items"
> 
> This is misleading:
>  - the behavior is not different for the first items, but for small (total)
> number of items
>  - it's not really dynamic: if the number of items is <= 4, you use a constant
> delay of 1/6 ANIMATION_TIME_IN
You are right.
I made some different codes, and this was my last one, and it's not dynamic.
> 
> I do wonder whether it makes sense to just always pick a fixed delay - it means
> that the total length of the folder animation depends on the number of items in
> the folder, but that doesn't sound too crazy ...
we can't control how much items are in the folder, so it can be really slow for lot of items. So it's not really scalable.
> 
> ::: js/ui/iconGrid.js
> @@ +431,3 @@
>          }
> 
> +        let totalDelay = clamp(ANIMATION_BASE_DELAY_FOR_ITEM * actors.length,
> 0, ANIMATION_MAX_DELAY_FOR_ITEM);
> 
> Why clamp? Math.min(ANIMATION_BASE-DELAY_FOR_ITEM * actors.length,
> ANIMATION_MAX_DELAY_FOR_ITEM) can never be negative ...
Right.

Created attachment 286704 [details] [review]
iconGrid: Fix slowness on pulse animation for few items

Currently we use the same amount of total delay divided by all items,
but that makes the items animate slow if the amount of items is small.
To avoid that, use a smaller total amount delay for an amount of items
smaller than four.

Review of attachment 286704 [details] [review]:

(In reply to comment #3)
> we can't control how much items are in the folder, so it can be really slow for
> lot of items. So it's not really scalable.

But does it matter? With a delay of roughly 60ms (e.g. what you are using in this patch for up to 4 items), 18 items will take about a second (which admittedly is 200-300ms longer than the current animation) - assuming a 6x4 grid, those would be all items actually visible without scrolling, so not sure anything above that matters too much ...

That said, I'm perfectly fine with this approach, just some nits that escaped me earlier:

::: js/ui/iconGrid.js
@@ +431,3 @@
         }
 
+        let totalDelay = Math.min(ANIMATION_BASE_DELAY_FOR_ITEM * actors.length, ANIMATION_MAX_DELAY_FOR_ITEM);

"totalDelay" sounds like

   ANIMATION_MAX_DELAY_FOR_ITEM * (actors.length - 1) / 2

to me, e.g. the sum of all delays added together. Should use something like maxDelay (though that's slightly incorrect as well, as the largest delay used will be slightly smaller than that, but meh).

It's also not immediately obvious without the context of this bug, so a small comment would be good.

Created attachment 288102 [details] [review]
iconGrid: Fix slowness on pulse animation for few items

Currently we use the same amount of total delay divided by all items,
but that makes the items animate slow if the amount of items is small.
To avoid that, use a smaller total amount delay for an amount of items
smaller than four.

(In reply to comment #6)
> Created an attachment (id=288102) [details] [review]
> iconGrid: Fix slowness on pulse animation for few items
> 
> Currently we use the same amount of total delay divided by all items,
> but that makes the items animate slow if the amount of items is small.
> To avoid that, use a smaller total amount delay for an amount of items
> smaller than four.

I want review on the comment, I'm not sure how good it is =)

Review of attachment 288102 [details] [review]:

Comment looks fine to me

Comment on attachment 288102 [details] [review]
iconGrid: Fix slowness on pulse animation for few items

Thanks for review!

Attachment 288102 [details] pushed as d0de411 - iconGrid: Fix slowness on pulse animation for few items