Created attachment 153644 [details] [review]
[MessageTray] Fix summary to be initially-hidden

Some recent change broke it so that the first time a notification pops
up, the summary will be visible alongside it.

Created attachment 153645 [details] [review]
[MessageTray] reimplement the state machine

Previously, every time _updateState was called, it would make some
change, and so it was necessary to very carefully set up all the calls
to it, to ensure it was always called at exactly the right time. Now,
instead, we keep a bunch of state variables like "_notificationState"
and "_pointerInSummary", and potentially multiple timeouts, and
_updateState looks at all of them and figure out what, if anything,
needs to be changed.

By making the rules about what causes changes more explicit, it will
be easier to change those rules in the future as we add new
functionality.

Created attachment 153646 [details] [review]
[MessageTray] Show the message tray in the overview

Also, make the summary area always visible when in the overview

The black-on-black UI here is obviously wrong and needs design input

(In reply to comment #2)
> Created an attachment (id=153645) [details] [review]
> [MessageTray] reimplement the state machine

it might be easier to review this by just applying it and then just reviewing MessageTray from top-to-bottom, rather than by trying to figure out the patch, since almost all of it has changed.

I think the black on black is fine for now.  The text, icon, and actions are clear.  I think we do want the tray font size to be the same size as the top bar though.

Review of attachment 153644 [details] [review]:

::: js/ui/messageTray.js
@@ +347,3 @@
         this._summary.connect('enter-event',
+                              Lang.bind(this, this._showMessageTray));
+        this._hideSummary();

I wonder if this should be

this._summaryBin.opacity = 0;

instead, so that we are not doing an animation on startup (even though we are not normally showing the tray on startup).

Review of attachment 153645 [details] [review]:

Very clever overall :). Good to commit once you decide what you want to do about the comments below. Perhaps it is good to reflect the changes in behavior in the commit message if you decide to keep these changes.

Here are the changes in behavior that I see based on this patch. They are probably easy to fix if they are not intentional.

1) The new notification will show up while the user has the summary open. Previously, we would have waited for the user to end interacting with the summary, but this might be a desirable change if the summary and notifications area will not overlap. The notification that needs to be expanded first shows up as a banner, and then quickly expands, which is a little weird.

2) The summary will show up if the user mouses over to the right corner while the notification is up. The notification would expand in that case too. The summary would not show up, but the notification will expand, if the user mouses over to any other part of the tray. The summary would show up if the user moves the mouse over the tray to the right corner after that. This seems like a reasonable behavior, though it might seem a little arbitrary if all you are expecting is just an expanded notification.

::: js/ui/messageTray.js
@@ +451,1 @@
             this._updateState();

It seems that _updateState() should take care of deciding if we are ready to show another notification, so we don't need the if (!this._notification) check here.

@@ +488,3 @@
+    // _updateState() figures out what (if anything) needs to be done
+    // at the present time. Every tray animation should add 1 to
+    // this._animations when it starts, and then subtract 1 and call

this._animations is not used

(In reply to comment #7)
> 1) The new notification will show up while the user has the summary open.

Right, that part was intentional. I fixed one or two small things in the rewrite, and then forgot to mention it. (This one came from the big long list of questions to Jon; #16.) It also fixes it so that the summary is only shown after a notification if it has changed, and so that the notification-out and summary-in animations overlap, like in the original animated tray mockups.

> The notification that needs to be expanded first shows
> up as a banner, and then quickly expands, which is a little weird.

Oops. Probably should fix it so that if the summary is visible, then the notification only expands if the pointer is in the non-summary part of the message tray

> 2) The summary will show up if the user mouses over to the right corner while
> the notification is up.

And should probably just make that not happen. (The summary can only pop up when there is no notification.) (Though the reverse is still not true; a notification can pop up while the summary is visible.)

Review of attachment 153646 [details] [review]:

Looks good. We should make sure clicking on the icons leaves the overview and switches to the corresponding window, instead of just switching to that window when the user leaves the overview later. This can be achieved by always closing the overview if it is opened in activateWindow() in main.js (as suggested by Dan on IRC).

Created attachment 154073 [details] [review]
Main.activateWindow: always exit the overview if it's currently open

This way, clicking a message tray icon while the overview is open will
close the overview when activating its window.

Remove some other overview-related activation code which is now
redundant.

Also, remove calls to "global.get_current_time()" when calling
Main.activateWindow, since it's unnecessary (activateWindow will call
it itself if you don't pass in that arg).

Created attachment 154074 [details] [review]
fixes

per comment #7. This will be squashed before committing. (I also made the
change suggested in comment #6, and this patch won't apply cleanly unless
you fix that first.)

Review of attachment 154073 [details] [review]:

Looks good.

Review of attachment 154074 [details] [review]:

Looks good, just be sure to include the changes in the behavior in the commit message when you squash the patches.

Attachment 153644 [details] pushed as 02f67af - [MessageTray] Fix summary to be initially-hidden
Attachment 153645 [details] pushed as e86c821 - [MessageTray] reimplement the state machine
Attachment 153646 [details] pushed as 77fe0db - [MessageTray] Show the message tray in the overview
Attachment 154073 [details] pushed as cec62a7 - Main.activateWindow: always exit the overview if it's currently open