Created attachment 164514 [details] [review]
[MessageTray] Use status from gnome-session

This patch modifies MessageTray behaviour so that normal (not urgent)
notifications are not shown when the user is Busy (they're sent
immediately to the Summary area). When status is then changed,
notifications still pending are shown again.
Additionally, when status is modified from Idle to anything else, the
message tray is forced open for 10 seconds, so that summary icons are
visible. As usual, moving to the message tray and then away closes it
and stops the timeout.

No comments have been made for two weeks after the patch, but the message tray design was updated. Should this be marked as duplicate of 617224?

Created attachment 181075 [details] [review]
Use status from gnome-session

Update to apply to the latest in master.

Review of attachment 181075 [details] [review]:

The subject line should reflect that it is the message tray behavior that is being updated, e.g.:
"MessageTray: act based on the user status from gnome-session"

We should not show the summary briefly in the busy state when a new notification is queued; this can be achieved by checking if we are not busy in this line of _updateState() :
if (notificationsDone && this._newSummaryItems.length > 0)

This patch will be used as a basis for the work Hellyna is doing for bug 636838. For example, we will further modify the behavior to only show the message tray when the user comes back from being idle if there are new notifications in it.

::: js/ui/messageTray.js
@@ +943,3 @@
+        this._presence = new GnomeSession.Presence();
+        this._userStatus = GnomeSession.PresenceStatus.AVAILABLE;
+        this._busy = false;

this._busy doesn't seem necessary for the current patch, you can just check if this._userStatus == GnomeSession.PresenceStatus.BUSY when it's used once in _updateState() .

On the other hand, it is likely wrong that when we transition from busy to idle, we start showing queued notifications. So maybe keep this._busy as true when the presence is set to idle after being busy, so that we still don't show any non-critical notifications. This is important if, for example, you were making a presentation and paused to talk and your session turned idle. You don't want the queued notifications to start popping up as that happens.

@@ +1438,3 @@
+            this._backFromAwayTimeoutId = Mainloop.timeout_add_seconds(10, Lang.bind(this,function() {
+                this._backFromAway = false;
+                this._backFromAwayTimeoutId = 0;

This doesn't actually remove the summary after the timeout because nothing calls _updateState() again.

What should work is to get rid of this._backFromAwayTimeoutId and instead have _showSummary() take the timeout value instead of a boolean 'withTimeout'. The longer summary timeout value can be defined with a variable LONGER_SUMMARY_TIMEOUT and perhaps set to 10 or 5 seconds.

Then, in _updateState() , if this._backFromAway is true call _showSummary with LONGER_SUMMARY_TIMEOUT .

@@ +1483,3 @@
     _updateState: function() {
         // Notifications
+        let notificationsPending = this._notificationQueue.length > 0 && (!this._busy || this._notificationQueue[0].urgent);

With the more recent changes on how we define urgency for notifications, this should be
this._notificationQueue[0].urgency >= Urgency.CRITICAL

(In reply to comment #4)
> Review of attachment 181075 [details] [review]:
>
> ::: js/ui/messageTray.js
> @@ +943,3 @@
> +        this._presence = new GnomeSession.Presence();
> +        this._userStatus = GnomeSession.PresenceStatus.AVAILABLE;
> +        this._busy = false;
> 
> this._busy doesn't seem necessary for the current patch, you can just check if
> this._userStatus == GnomeSession.PresenceStatus.BUSY when it's used once in
> _updateState() .
> 
> On the other hand, it is likely wrong that when we transition from busy to
> idle, we start showing queued notifications. So maybe keep this._busy as true
> when the presence is set to idle after being busy, so that we still don't show
> any non-critical notifications. This is important if, for example, you were
> making a presentation and paused to talk and your session turned idle. You
> don't want the queued notifications to start popping up as that happens.
>

I removed this._busy anyway because:
1) Idle means showing the screensaver. Whatever we do while idle, it won't be visible to the user.
2) Closing the screensaver means going back to busy, and that should be more important than backFromAway.
3) Full screen applications that set status to busy are likely to also inhibit the screensaver, so status won't change to idle.

Created attachment 181327 [details] [review]
MessageTray: Use status from gnome-session

This patch modifies MessageTray behaviour so that normal (not urgent)
notifications are not shown when the user is Busy (they're sent
immediately to the Summary area). When status is then changed,
notifications still pending are shown again.
Additionally, when status is modified from Idle to anything else, the
message tray is forced open for 10 seconds, so that summary icons are
visible.

Created attachment 181329 [details] [review]
MessageTray: Use status from gnome-session

This patch modifies MessageTray behaviour so that normal (not urgent)
notifications are not shown when the user is Busy (they're sent
immediately to the Summary area). When status is then changed,
notifications still pending are shown again.
Additionally, when status is modified from Idle to anything else, the
message tray is forced open for 10 seconds, so that summary icons are
visible.

Review of attachment 181329 [details] [review]:

> > On the other hand, it is likely wrong that when we transition from busy to
> > idle, we start showing queued notifications. So maybe keep this._busy as true
> > when the presence is set to idle after being busy, so that we still don't show
> > any non-critical notifications. This is important if, for example, you were
> > making a presentation and paused to talk and your session turned idle. You
> > don't want the queued notifications to start popping up as that happens.
> >
> 
> I removed this._busy anyway because:
> 1) Idle means showing the screensaver. Whatever we do while idle, it won't be
> visible to the user.
> 2) Closing the screensaver means going back to busy, and that should be more
> important than backFromAway.
> 3) Full screen applications that set status to busy are likely to also inhibit
> the screensaver, so status won't change to idle.

It's important to keep track of whether the user got to the idle state from the busy state and keep suppressing the notifications in that case because
1) Right now, we start showing the missed notifications as the screensaver fades in, which is wrong. You can see that by changing desktop/gnome/session/idle_delay to 1 in gconf-editor, setting your status to busy, sending yourself a notification with notify-send or one of the tests from libnotify/tests, and being idle for 1 minute.
2) The user will not know that they missed some messages during idle when they got to it from the busy state and returned right to the busy state, because we will not show them the summary when they are back from away because they are busy and we will no longer be queueing any suppressed notifications to show them when they switch to available state.
3) We can't rely on all full screen applications to do the right thing by inhibiting the screensaver, and of course the user can sometimes choose the busy state themselves.

::: js/ui/messageTray.js
@@ +28,1 @@
 const SUMMARY_TIMEOUT = 1;

Can you please make the order and naming consistent with HIDE_TIMEOUT and LONGER_HIDE_TIMEOUT defined just below. Also 10 seconds is really long; let's make it 4, same as NOTIFICATION_TIMEOUT.

@@ +1422,3 @@
 
+    _onStatusChanged: function(presence, status) {
+        if(this._userStatus == GnomeSession.PresenceStatus.IDLE

Need a space after if.

@@ +1471,2 @@
         // Notifications
+        let notificationsPending = this._notificationQueue.length > 0 && (!busy || this._notificationQueue[0].urgency >= Urgency.CRITICAL);

actually, let's make this urgency == Urgency.CRITICAL (same thing, just that's how we check for it in other places)

@@ +1490,3 @@
         let summarySummoned = this._pointerInSummary || this._overviewVisible;
+        // Ignore summary timeouts if busy, but keep the summary open in other cases
+        let summaryPinned = (this._summaryTimeoutId != 0 && !busy) || this._pointerInTray || summarySummoned || this._locked;

So this change would remove the summary if it was shown automatically and you changed your state to Busy, which makes sense. It would also make sense then to remove a notification that is showing as a banner when you change your state to Busy.

It's better to actually unset this._summaryTimeoutId and this._notificationTimeoutId explicitly in _onStatusChanged() when the status changes to busy, rather than just ignoring them when busy. So I think it's best to add the following to _onStatusChanged() and not have !busy check for summaryPinned.

if (status == GnomeSession.PresenceStatus.BUSY) {
    this._updateNotificationTimeout(0);
    if (this._summaryTimeoutId) {
        Mainloop.source_remove(this._summaryTimeoutId);
        this._summaryTimeoutId = 0;
    }
}

@@ +1508,1 @@
             else if (summarySummoned)

If one part of the if-else clause requires curly braces, we should use them for all of them.

@@ +1762,3 @@
 
+        if (timeout != 0) {
+            /* XXX: timeout is always a multiple of 1000, can we use timeout_add_seconds here? */

timeout_add_seconds() is not precise; see the docs here
http://library.gnome.org/devel/glib/stable/glib-The-Main-Event-Loop.html#g-timeout-add-seconds-full

Created attachment 181637 [details] [review]
MessageTray: Use status from gnome-session

This patch modifies MessageTray behaviour so that normal (not urgent)
notifications are not shown when the user is Busy (they're sent
immediately to the Summary area). When status is then changed,
notifications still pending are shown again.
Additionally, when status is modified from Idle to anything else, the
message tray is forced open for 10 seconds, so that summary icons are
visible.

Review of attachment 181637 [details] [review]:

The commit message body should say
"Additionally, when status is modified from Idle to anything other than Busy,
the message tray is forced open for 4 seconds, so that summary icons are
visible."

Good to commit with these minor fix-ups.

::: js/ui/messageTray.js
@@ +1425,3 @@
+        if (this._userStatus == GnomeSession.PresenceStatus.IDLE
+            && this._userStatus != status)
+            this._backFromAway = true;

Actually, can we replace this with
this._backFromAway = (this._userStatus == GnomeSession.PresenceStatus.IDLE && this._userStatus != status);

so that we update this value to false on any other status change, and e.g. don't show the summary when the user changes their status from BUSY to AVAILABLE after having been IDLE during the BUSY state.

@@ +1430,3 @@
+
+        if (status == GnomeSession.PresenceStatus.BUSY) {
+            // remove notifications and allow the summary to be closed now

"remove notification..." would be more accurate

@@ +1441,3 @@
+        }
+        // else, don't set this._busy, so activating the screensaver
+        // when busy doesn't show queued notifications

I think it would be clearer to move in a comment like this just above this._busy = false; saying:
// We preserve the previous value of this._busy if the status turns to IDLE
// so that we don't start showing notifications queued during the BUSY state
// as the screensaver gets activated.

@@ +1485,3 @@
         // Notifications
+        let notificationsPending = this._notificationQueue.length > 0 &&
+            (!this._busy || this._notificationQueue[0].urgency == Urgency.CRITICAL);

In the neighboring code, we align the second line with the beginning of the assigned value.

@@ +1517,3 @@
+                // Immediately set this to false, so that we don't schedule a timeout later
+                this._backFromAway = false;
+                this._showSummary(LONGER_SUMMARY_TIMEOUT);

It might make sense to do

if (this._backFromAway) {
    //Immediately set this to false, so that we don't schedule a timeout later
    this._backFromAway = false;
    if (!this._busy)
        this._showSummary(LONGER_SUMMARY_TIMEOUT);
}

Attachment 181637 [details] pushed as 1496c85 - MessageTray: Use status from gnome-session