Traditionally, we've had the model of "click on the source to remove it and perform the default action, click away to dismiss it and do nothing".

I've always felt this was super poor. What sort of thing would you prefer here?

Hey Jasper,

(In reply to comment #1)
> Traditionally, we've had the model of "click on the source to remove it and
> perform the default action, click away to dismiss it and do nothing".
> 
> I've always felt this was super poor. What sort of thing would you prefer here?

If the notification is persistent, I think it should stick around in the tray until you explicitly remove it using the context menu. So, persistent notifications:

 * Clicking on the close button should close the bubble, but leave the message source in the tray.
 * Clicking on the body of a bubble should just perform the default action, but not remove the source.

Transient notifications:

 * Clicking on the bubble body should do the default action and remove the item from the tray.
 * Clicking on the close button should close the bubble and remove the source from the tray.

All of these actions should also hide the tray.

Hope that makes sense...

I just wanted to clarify that we have three types of notifications - resident, persistent, and transient. Below is the current behavior. Do you still think we need to make any changes?

Resident notifications (e.g. Rhythmbox, chat)

* Clicking the close button on the new notification, closes the notification, but doesn't remove it or the source
* Clicking the close button on the summary notification, removes the notification and the source
* Clicking on the new or summary notification, closes the notification and performs the default action, but keeps the notification and source around
* If there is a new chat message or a new song is played, a new notification will show up and the source will be re-added

Persistent notifications (e.g. XChat IRC messages, updates)

* Clicking the close button on the new notification or the summary notification, removes the notification and its source
* Clicking on the new or summary notification, removes the notification and its source, as well as performs a default action
* If there is a new notification from the source, it will show up and the source will be re-added

Transient notifications (e.g. non-critical battery information)

* The bahavior is the same as for persistent notifications
* Also, the notification and the source will be removed right away as soon as the notification is done displaying, even if the user takes no action

Note that most notifications are persistent notifications - it's the behaviour you get by default.

(In reply to comment #0)
>  * It is surprising - the close button is on the bubble, not on the source.
> This means that I expect it to act on the bubble only. Also, many people won't
> be aware that message sources can be removed.

You open a bubble containing all the notifications in the source.

Created attachment 224730 [details] [review]
messageTray: make SummaryItem._closeButton public

Use this to show/hide the close button instead of closeButtonVisible.

Created attachment 224731 [details] [review]
messageTray: Only hide the notification stack when clicking on the close button

Rather than destroying the entire source, which is unintuitive, simple close the
notification. Removing the entire source is still possible by right-clicking on
the summary item and choosing "Remove".

Created attachment 224732 [details] [review]
messageTray: Don't destroy the notification when clicking on the close button

Clicking on the close button should simply hide the notification.

Created attachment 224733 [details] [review]
messageTray: Don't animate the notification hiding if it's closed

Having the notification, which includes the close button, zoom away from
you looks weird, as if the system was acting on its own. Make the
notification immediately disappear.

There has been some noise wrt misunderstanding the terminology (transient, resident and persistent), but the point remains in that closing a bubble while in the message tray should not remove/destroy the source as well as the bubble. It's very indirect and unexpected. I operate on the bubble and suddenly an icon disappears (and the whole message tray closes).

Persistent notifications that have been seen (bubble opened) would get cleared (sources removed) the next time the message tray closes, while resident would not. But even if you have the bubble open on a persistent message and close it with the (X) button, the source remains, the MT doesn't close. We could mark the icon with opacity: 0.5 at this point to indicate it will be cleared when the message tray closes, but I don't think it's critical.

I don't think it is a very good idea to introduce close buttons in 3.6.x and then change radically how they work in 3.8, so we should try to get this into 3.6.1

(In reply to comment #11)
> I don't think it is a very good idea to introduce close buttons in 3.6.x and
> then change radically how they work in 3.8, so we should try to get this into
> 3.6.1

Can we, then?

Review of attachment 224730 [details] [review]:

Sure.

Review of attachment 224731 [details] [review]:

::: js/ui/messageTray.js
@@ +2360,3 @@
             closeButton.show();
+            this._summaryBoxPointerCloseClickedId = closeButton.connect('clicked', Lang.bind(this, function() {
+                this._onSummaryBoxPointerUngrabbed();

Calling onUngrabbed() to do the ungrabbing is pretty confusing - how about

  this._grabHelper.ungrab({ actor: this._summaryBoxPointer.bin.child }); ?

Review of attachment 224732 [details] [review]:

::: js/ui/messageTray.js
@@ +2258,3 @@
         }
+
+        this._notificationClosed = false;

This is rather surprising at this point ("But I just hid the notification, how comes it is not closed?") - if you follow my suggestion for the next patch of leaving the animation (but hiding the close button), you can move this back into _onCloseClicked() ...

Review of attachment 224733 [details] [review]:

Having the notification pop down (not zooming away) looks rather natural to me - better in fact than hiding it immediately. I think we should hide the close button though.

Review of attachment 224733 [details] [review]:

What's the difference between "pop down" and "zoom away"?

In my understanding "zoom" implies a size change, so it doesn't describe the sliding animation used for notification hiding very well.

(In reply to comment #18)
> In my understanding "zoom" implies a size change, so it doesn't describe the
> sliding animation used for notification hiding very well.

Oh. I just meant "move fast".

So, uh, we didn't get this into 3.6.1 because of another conflict in design.

*sigh*

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

(In reply to comment #20)
> So, uh, we didn't get this into 3.6.1 because of another conflict in design.

I'm sure waiting ten days until coming back to the bug helped as well ;-)

But yes, I do disagree that animations suggest that "the system was acting on its own" - by that reasoning, we should remove the close animations when toggling summary sources / top bar buttons as well? Jumping from one state to another does not feel natural, so we should try hard to avoid this - sometimes we need to make an exception, and if we have a "bigger" animation at the same time, we get away with it nicely. Changing the top bar icons while the screen shield slides down is a good example for this, hiding the close button immediately while sliding out the notification would be more or less the same.

Last but not least, while the particular animation is different, the mockup[0] does animate the notification after clicking the close button.

[0] http://www.youtube.com/watch?v=14z4wdgNF9g

(In reply to comment #22)
> (In reply to comment #20)
> > So, uh, we didn't get this into 3.6.1 because of another conflict in design.
> 
> I'm sure waiting ten days until coming back to the bug helped as well ;-)
> 
> But yes, I do disagree that animations suggest that "the system was acting on
> its own" - by that reasoning, we should remove the close animations when
> toggling summary sources / top bar buttons as well?

If there was an explicit close button on those menus, then yes. I guess the reasoning I have is because the button I clicked moves away from under my pointer. It also doesn't behave like a button, with prelight/down states.

> Last but not least, while the particular animation is different, the mockup[0]
> does animate the notification after clicking the close button.
> 
> [0] http://www.youtube.com/watch?v=14z4wdgNF9g

(In reply to comment #23)
> I guess the reasoning I have is 
> because the button I clicked moves 
> away from under my pointer.

Oh, I agree with the button movement being distracting - which is why I'm suggesting to hide the button immediately. We should try to avoid abrupt state changes, but with the notification already transitioning, the additional button animation does more harm than good.

(In reply to comment #24)
> Oh, I agree with the button movement being distracting - which is why I'm
> suggesting to hide the button immediately.

That could work, yeah. Do you have time to cook up a few patches? I'm currently deep yak-shaving in the scary caves of XInput2 / pressure barriers, so it could take me a few hours to get back out.

(I really should stop playing Minecraft so much...)

Created attachment 227302 [details] [review]
messageTray: make SummaryItem._closeButton public

Unmodified, just reattaching to maintain patch order.

Created attachment 227303 [details] [review]
messageTray: Only hide the notification stack on clicking close

Changed as of the suggestion in comment #14, and fixed an exception I missed in the initial review.

Created attachment 227304 [details] [review]
messageTray: Don't destroy the notification when clicking on the close button

Updated according to comment #15.

Created attachment 227305 [details] [review]
messageTray: Hide notification close button immediately on click

Having the close button move away from under the pointer after
clicking it is confusing and distracts from the main transition,
which is hiding the notification. Just hide it immediately.

Review of attachment 227305 [details] [review]:

Sure.

Review of attachment 227304 [details] [review]:

Yep.

Review of attachment 227303 [details] [review]:

::: js/ui/messageTray.js
@@ +2412,3 @@
             closeButton.show();
+            this._summaryBoxPointerCloseClickedId = closeButton.connect('clicked', Lang.bind(this, function() {
+                this._grabHelper.ungrab({ actor: this._summaryBoxPointer.bin.child });

Lang.bind(this, _hideSummaryBoxPointer) ?

Review of attachment 227302 [details] [review]:

Restoring your ACN from the first time.

Attachment 227302 [details] pushed as 4f87699 - messageTray: make SummaryItem._closeButton public
Attachment 227303 [details] pushed as 71c2361 - messageTray: Only hide the notification stack on clicking close
Attachment 227304 [details] pushed as 92a01c6 - messageTray: Don't destroy the notification when clicking on the close button
Attachment 227305 [details] pushed as a7b5134 - messageTray: Hide notification close button immediately on click

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