Created attachment 171448 [details] [review]
MessageTray: fix wobbling during summaryitem animation

Redo the way that the summary item expand/collapse animation works so
that the items all resize in unison so that when moving from one to
another, the summary area as a whole stays a constant width rather
than wobbling slightly.

(Also rename all references to the "minimum" summary item title width,
since it's not a minimum, it's just the width.)

Review of attachment 171448 [details] [review]:

This seems to work really well! Good to commit once you address the comments below.

::: js/ui/messageTray.js
@@ +714,3 @@
+        // smoothly collapse the spacing between icon and title along
+        // with it.
+        this._sourceBox = new  Shell.GenericContainer({ style_class: 'summary-source' });

Could you explain in this comment why GenericContainer allows to smoothly collapse the spacing and St.BoxLayout doesn't?

@@ +790,3 @@
+        if (!ok)
+            spacing = 0;
+        return Math.min(naturalWidth + spacing, MAX_SOURCE_TITLE_WIDTH);

Can we represent the spacing as padding_left of this._sourceTitle?

@@ +941,3 @@
+        if (titleWidth > this._summaryItemTitleWidth) {
+            if (this._imaginarySummaryItemWidth == this._summaryItemTitleWidth)
+                this._imaginarySummaryItemWidth = titleWidth;

Should this be called this._imaginarySummaryItemTitleWidth ?

It'd be good to explain in a comment why we are resetting this._imaginarySummaryItemWidth only if it's equal to this._summaryItemTitleWidth .

@@ +1001,3 @@
+
+            this._summaryItemTitleWidth = newTitleWidth;
+            this._imaginarySummaryItemWidth = newTitleWidth;

Does this also need
if (this._imaginarySummaryItemWidth == this._summaryItemTitleWidth)
check? (Before this._summaryItemTitleWidth is changed.)

@@ +1103,3 @@
+        // invisible item if we're collapsing everything.
+        Tweener.addTween(this,
+                         { _expandedSummaryItemWidth: this._summaryItemTitleWidth,

Should this be called _expandedSummaryItemTitleWidth ?

Review of attachment 171448 [details] [review]:

In the commit subject there needs to be a space in "summaryitem".

Created attachment 171984 [details] [review]
MessageTray: fix wobbling during summaryitem animation

conflicting change which removed this._summaryNeedsToBeShown and added
this._newSummaryItems .

MessageTray: fix wobbling during summaryitem animation

Redo the way that the summary item expand/collapse animation works so
that the items all resize in unison so that when moving from one to
another, the summary area as a whole stays a constant width rather
than wobbling slightly.

(Also rename all references to the "minimum" summary item title width,
since it's not a minimum, it's just the width.)

The attached patch is your patch rebased to master. There was a conflicting change which removed this._summaryNeedsToBeShown and added this._newSummaryItems .

(In reply to comment #2)
> Could you explain in this comment why GenericContainer allows to smoothly
> collapse the spacing and St.BoxLayout doesn't?

Because a BoxLayout either has the padding or it doesn't; when you shrink the label down to 0 width, there's still the full padding, and if you remove the label completely, the padding immediately disappears rather than tweening. However...

> Can we represent the spacing as padding_left of this._sourceTitle?

Yes, and that solves the problem above and lets us use a boxlayout instead. Yay!

> Should this be called this._imaginarySummaryItemTitleWidth ?

I was trying to keep the variable name length down a little... but sure.

> It'd be good to explain in a comment why we are resetting
> this._imaginarySummaryItemWidth only if it's equal to
> this._summaryItemTitleWidth .

I changed it to

    if (!this._expandedSummaryItem)
        this._imaginarySummaryItemTitleWidth = titleWidth;

which means the same thing, but I think explains the reason better.

> Does this also need
> if (this._imaginarySummaryItemWidth == this._summaryItemTitleWidth)
> check?

yes, fixed

committed updated patch

Created attachment 173573 [details] [review]
Increase the timeout or hiding the tray if the mouse didn't move far from it.

We shouldn't hide the tray as quickly if the user might have left it
unintentionally, such as when moving the mouse over to a different tray item or
using the scroll bar in the chat notification.

https://bugzilla.gnome.org/show_bug.cgi?id=630767