Created attachment 179267 [details] [review]
Only update the expanded message tray item after a time-out

Does this patch fix/make better the jiggle you are describing? This patch
should make the message tray accordion out animation feel more stable in
general.

It causes a very strange effect where the farthest icon and label "bounce" in and out of its expanded mode. Sometimes this causes all of the tray icons to "escape" from you (they will suddenly collapse). If I very very carefully mouse over icons I can still see the jiggle as well.

Created attachment 179975 [details] [review]
Only animate the message tray items on a hover of a new item or when the tray is left

This makes the animation feel more stable and keeps the left-most item expanded
when you move the mouse to the left in the tray.

The following behavior details are implemented by this patch:
 - we wait to collapse an expanded item till after the tray is hidden if
   we are hiding the tray
 - we don't collapse an expanded item if the summary notification associated
   with it is being shown
 - we don't consider the mouse hovering over a summary notification as
   hovering over the tray, which allows us to collapse an expanded summary
   item if it is not associated with the summary notification being shown

We still need to use this._imaginarySummaryItemTitleWidth because we
start out with all the items collapsed and in some cases we still collapse
all the items.

(In reply to comment #3)
>  - we don't collapse an expanded item if the summary notification associated
>    with it is being shown
>  - we don't consider the mouse hovering over a summary notification as
>    hovering over the tray, which allows us to collapse an expanded summary
>    item if it is not associated with the summary notification being shown

Don't those contradict each other, unless you're allowing for two items to be expanded at once?

It seems like we should just make it so that having a notification visible locks the summary in an expanded state. So we'd have fully-collapsed mode (initially), "accordian mode" (when you're browsing sources, but haven't clicked on anything yet), and locked-expanded mode (when you click on an item to show its notification). Dismissing a notification from locked-expanded mode would bring you back to accordian mode, and clicking on a different summary item would hide the current notification, re-expand the summary to the new item, and show that item's notification.

Created attachment 180024 [details] [review]
Only animate the message tray items on a hover of a new item or when the tray is left

Updated the commit message to say:

 - we don't collapse an expanded item if the summary notification associated
   with it is being shown, unless a different summary item is hovered over
 - we don't consider the mouse hovering over a summary notification as
   hovering over the tray, which allows us to collapse an expanded summary
   item if it is not associated with the summary notification being shown

So we do collapse the summary item for which a summary notification is shown
if a different summary item is hovered over. Showed the new behavior to Jon
and it makes sense to him. We do want to expand other summary items on hover
when a summary notification is visible because that lets the user decide
which other summary item to switch to or learn what is that new summary item
that appeared.

Comment on attachment 180024 [details] [review]
Only animate the message tray items on a hover of a new item or when the tray is left

>+        // We only update the expanded summary item when a new item is being hovered over
>+        // or when the mouse leaves the tray. This ensures that the transition to collapse
>+        // a previously expanded item and expand a new one is fluid. This handling of hover
>+        // events for the summary items means that we won't collapse the left-most item if
>+        // the mouse moves to the left of it within the tray. This works well for handling
>+        // overshoot when the user moves the mouse to the left-most item.
>+        if (summaryItem.actor.hover)
>+            this._setExpandedSummaryItem(summaryItem);

OK, I feel like the patch is over-commented... I think it has more lines of comment than code. This comment, for instance, seems pretty much unnecessary. (It only makes sense if you're considering the code in the context of how it *used to* behave. But comments ought to be written for the benefit of people from the future, not people from the past.)

People can always "git blame" to find the commit message associated with a line of code if they need additional information about what it's doing, and then they can follow the link from there to bugzilla for even more information...

>+            // Don't do anything if the mouse is over the summary notification as this should be considered as
>+            // leaving the tray. The tray is locked when the summary notification is visible anyway, but we
>+            // should treat the mouse being over the summary notification as the tray being left for collapsing
>+            // any expanded summary item other than the one related to the notification.
>+            if (this._summaryNotificationBoxPointer.bin.hover)
>+                return;

This should not be possible... if we're at this point in the code, we know that the tray itself is being hovered over, so it's not possible that the summarynotification could also be hovered-over, right?

>+            // Collapse the expanded summary item as soon as the mouse leaves the tray regardless of whether we'll actually
>+            // hide the tray. The tray can be showing with all the items collapsed if it is locked because a summary
>+            // notification is showing or we are in the overview mode.
>+            // this._setExpandedSummaryItem(null);

I'm assuming that last line shouldn't be commented out? Were you testing something?

>+            // If we are hiding the tray, we'll collapse the expanded summary item when we are done hiding the tray.
>+            // However, we should collapse the expanded summary item at this point if the tray is locked or we are
>+            // in the overview mode and we are not displaying a summary notification related to the currently
>+            // expanded summary item.
>+            if ((this._locked || this._overviewVisible) && this._clickedSummaryItem != this._expandedSummaryItem)
>+                this._setExpandedSummaryItem(null);

I think this belongs in _updateState(), not here.

(Also, while we do want to get this patch in, it doesn't actually address the original bug report. AFAICT, that's caused by rounding errors in the _expandedSummaryItemTitleWidth handling, such that at one moment the total width of the summary area is, say, 235, and then in the next step of the animation it's 234, and then in the next step it's 235 again. If we wanted to fix it, we'd have to have the code notice when it was going from one fully-expanded state to another fully-expanded state, and if so, sprinkle in some extra +1s at the end if needed to ensure that the total width of the summary area stays the same throughout.)

Created attachment 180145 [details] [review]
Only animate the message tray items on a hover of a new item or when the tray is left

We need to check if this._summaryNotificationBoxPointer bin is being hovered
over explicitly, because otherwise this is considered as the tray being
hovered because it is a part of the tray St.Group .

The commented out code should not have been there, so I removed that. Also
removed the comment you found unnecessary and moved this._setExpandedSummaryItem()
call to _updateState() .

Comment on attachment 180145 [details] [review]
Only animate the message tray items on a hover of a new item or when the tray is left

> Also removed the comment you found unnecessary

Well, that was just an example. There were lots of comments I found either unnecessary or too verbose. Feel free to trim more.

Comment on attachment 180145 [details] [review]
Only animate the message tray items on a hover of a new item or when the tray is left

Looked over the comments in the final patch again, and there were just two new comments added. I think it is useful to explain the expected behavior if it is implemented across different sections of the code (e.g. sometimes we animate collapse of an expanded item and sometimes we just collapse it after it's hidden) or if there is a non-obvious reason for why the code is there (e.g. hovering over the summary notification should not be treated as hovering over the tray).

Created attachment 182747 [details] [review]
messageTray: fix summary area wobbling

Due to accumulation of rounding errors, the left edge of the summary area
could wobble by a few pixels during animations. Fix that.

Review of attachment 182747 [details] [review]:

Hmm, have concerns that that algorithm is unstable and doesn't really work as intended, though it could be misinterpretation on my part.

::: js/ui/messageTray.js
@@ +1371,3 @@
 
+            let oldWidth = this._summaryItems[i].getTitleWidth();
+            let newWidth = Math.floor(oldWidth * shrinkage);

Why do you floor not round? If you round, then the deviation will be something like O(sqrt(n_items)) while if you floor it will be O(n_items).

@@ +1391,3 @@
+                let oldWidth = this._summaryItems[i].getTitleWidth();
+                if (oldWidth != 0) {
+                    this._summaryItems[i].setTitleWidth (oldWidth + excess);

Isn't the end result that every step of the animation O(n_items) width migrates to the left-most item, and then is fodder for the next step of the animation which will assign more to the left-most item. Seems inherently unstable.

If keeping integer widths isn't important, then it seems the only part of this that matters is the fix to adjust _imaginarySummaryItemTitleWidth, but assuming you want to keep things integral, one technique is:

 total = 0;
 for (i = 0; i < items; i++)
   {
     new_total = total + (computation of unrounded quantity);
     this_amount = round(new_total) - round(total);
     total = new_total;
   }

(This is what we've been using for allocations, where it has a natural geometric interpretation)

(In reply to comment #12)
> Why do you floor not round? If you round, then the deviation will be something
> like O(sqrt(n_items)) while if you floor it will be O(n_items).

If we floor we're guaranteed a result that is too small (or correct). If we round, we could also end up with a result that was too big, which is more complicated, because you can't necessarily subtract all the excess from a single item.

> Isn't the end result that every step of the animation O(n_items) width migrates
> to the left-most item

No. The excess is added to the left-most item *that is currently shrinking* (width != 0 and not the expandedItem). And the excess is added after it is shrunk. If there is always rounding error then it will always end up being a bit wider than it should be at that tick, but it will still be shrinking, because being wider means it ends up getting proportionately more of the shrinkage on the next tick.

> If keeping integer widths isn't important

It's not, per se, but just assigning everything fractional widths did not fix the bug. (It reduced the wobble to O(1), but it was still there.)

> one technique is:

Given that this patch does actually work, do you still want me to rewrite it like that?

(In reply to comment #13)
> (In reply to comment #12)
> > Why do you floor not round? If you round, then the deviation will be something
> > like O(sqrt(n_items)) while if you floor it will be O(n_items).
> 
> If we floor we're guaranteed a result that is too small (or correct). If we
> round, we could also end up with a result that was too big, which is more
> complicated, because you can't necessarily subtract all the excess from a
> single item.
> 
> > Isn't the end result that every step of the animation O(n_items) width migrates
> > to the left-most item
> 
> No. The excess is added to the left-most item *that is currently shrinking*
> (width != 0 and not the expandedItem). And the excess is added after it is
> shrunk. If there is always rounding error then it will always end up being a
> bit wider than it should be at that tick, but it will still be shrinking,
> because being wider means it ends up getting proportionately more of the
> shrinkage on the next tick.

It seems to me that what would happen is that say you had 4 items, and you wanted to shrink them the total from 20 to 0 in 1 pixel increments, so you start with:

 5 5 5 5

and then they'd shrink to 4.75 4.75 4.75 4.75  and you'd have an excess of 3,
and you'd make it:

 7 4 4 4

And then at the next step, you shrink to 6.4 3.8 3.8 3.8, and you'd have an excess of 3 again, and you'd end up with

 9 3 3 3

so the left-item is getting more shrunk but not nearly enough to make up for the excess pixels it's getting.

> > If keeping integer widths isn't important
> 
> It's not, per se, but just assigning everything fractional widths did not fix
> the bug. (It reduced the wobble to O(1), but it was still there.)
> 
> > one technique is:
> 
> Given that this patch does actually work, do you still want me to rewrite it
> like that?

If you feel confident that the code is working, go ahead with it. It's not making sense to me still.

+        // If the tray as a whole is fully-expanded, make sure the
+        // left edge doesn't wobble during animation due to rounding.
+        if (this._imaginarySummaryItemTitleWidth == 0 && excess != 0) {

ie, it only runs when going from one fully-expanded state to another fully-expanded state (because if the summary as a whole is expanding or shrinking, you're not going to be able to tell if it's a pixel off for one tick).

Likewise, while it's possible that one item might grow by a pixel or two when it's supposed to be shrinking, this will only happen during the course of an animation, when the actors on either side of it are moving as well, and you're not going to notice the discrepancy. (And unless you've slowed down animations, tweener isn't going to be moving things in single-pixel increments anyway.)

(In reply to comment #15)
> +        // If the tray as a whole is fully-expanded, make sure the
> +        // left edge doesn't wobble during animation due to rounding.
> +        if (this._imaginarySummaryItemTitleWidth == 0 && excess != 0) {
> 
> ie, it only runs when going from one fully-expanded state to another
> fully-expanded state (because if the summary as a whole is expanding or
> shrinking, you're not going to be able to tell if it's a pixel off for one
> tick).
> 
> Likewise, while it's possible that one item might grow by a pixel or two when
> it's supposed to be shrinking, this will only happen during the course of an
> animation, when the actors on either side of it are moving as well, and you're
> not going to notice the discrepancy. (And unless you've slowed down animations,
> tweener isn't going to be moving things in single-pixel increments anyway.)

I think the algorithm is essentially wrong, and in other circumstances could easily lead to things being off by 10-20 pixels not 1-2 pixels, but if you slow things down and it doesn't do anything too weird, and you are comfortable with how it works, feel free to go ahead and commit. After all, the whole point of animations is to look good, so if it looks good, it is good.

with St.set_slow_down_factor(20), the animation is definitely not ideal,
but it does still make progress at each step, and never grows.

I tried a few rewrites in the style of comment 12, but it makes the code
bigger and more complicated, because you need to recompute excess,
oldExcess, and shrinkage at each step, and then you still are potentially
off by 1 pixel at the end, so you still need that fix-up step too. And it
doesn't make the animation look better even at really high slowdowns,
because at that point you notice *other* problems with the animation.

(If someone wants to rewrite this again, a better way might be to go back
to the old system of animating each item separately, but instead of
having them in a boxlayout, have them overlapping each other in an StGroup,
and set/tween their padding-right to get them in the right sequence.
Then you don't need to worry about accumulated rounding error at all,
since the non-moving icons will really be non-moving.)

Attachment 182747 [details] pushed as e27293e - messageTray: fix summary area wobbling

Woohoo, you rock!  Thanks Dan!