After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 630934 - Chats should jump to the top of the notification queue
Chats should jump to the top of the notification queue
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: High normal
: ---
Assigned To: Marina Zhurakhinskaya
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-09-29 19:01 UTC by Marina Zhurakhinskaya
Modified: 2011-01-05 23:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
I hope everything is complete in this patch. Tell me if there's still anything wrong :). (5.00 KB, patch)
2010-12-23 00:39 UTC, Hellyna Ng
reviewed Details | Review
I used the sort method against doing it manually. I assume that the sort method will be more efficient and cleaner looking, but there shouldn't be much of a difference. (5.28 KB, patch)
2011-01-04 09:37 UTC, Hellyna Ng
committed Details | Review

Description Marina Zhurakhinskaya 2010-09-29 19:01:27 UTC
Chats should jump to the top as they are received. Not above urgent
messages, but above Gwibber or Rhythmbox messages.
Comment 1 Hellyna Ng 2010-12-23 00:39:58 UTC
Created attachment 176908 [details] [review]
I hope everything is complete in this patch. Tell me if there's still anything wrong :).
Comment 2 Marina Zhurakhinskaya 2011-01-03 23:41:00 UTC
Review of attachment 176908 [details] [review]:

How about this for a commit message

"Chats should jump to the top of the notification queue

This is to ensure users get notified as soon as chats are received. Notifications with critical urgency still have the highest priority."

::: js/ui/messageTray.js
@@ +40,3 @@
 
+// We set another set of Urgency enum here apart from the one in notificationDaemon.js, to allow us more flexibility in ranking
+// priority of messages for example, from chats.

This enum should not be described as "another" enum. How about this comment?

// Message tray has its custom Urgency enumeration. LOW, NORMAL, and CRITICAL
// urgency values map to the corresponding values for the notifications received
// through the notification daemon. HIGH urgency value is used for chats received
// through the Thelepathy client.

@@ +501,3 @@
     },
 
+    // This function maps the standard urgency levels in notificationDaemon.js to custom levels defined in here.

This comment no longer applies. You can just remove it.

@@ +1224,3 @@
                                  Lang.bind(this, this.removeNotification));
+            this._notificationQueue.unshift(notification);
+            this._notificationQueue.sort(this._compareNotificationUrgencies);

You should use push() instead of unshift() here to keep the notifications that are already in the queue and have the same urgency value as the newly added notification before it.

I think it would be more clear to define the sorting function inline. Otherwise you would need to define it as a static function outside of the MessageTray class. So how about
this._notificationQueue.sort(function(notification1, notification2) {
    return notification2.urgency - notification1.urgency;
});

The sort should be stable, that is it should not change the order of notifications already in the queue that have the same urgency value, so using push() and sort() is a nice solution that should work.

Another solution would be to use a while loop similar to the one you had in your original patch. The while loop here starts out from the end of the array to ensure that we append the new notification at the end of the subset of notifications with the same urgency value. I'm including it in the comment as an example of an alternative solution, however you can keep the first solution with push() and sort() in your patch.

let i = this._notificationQueue.length - 1;
while (i >= 0 && notification.urgency > this._notificationQueue[i].urgency)
    i--;
this._notificationQueue.splice(i + 1, 0, notification);

@@ +1569,3 @@
         // are expanded in case they were in the process of hiding and need
         // to re-expand.
+        if (this._notification.urgency == Urgency.CRITICAL || this._notification.expanded)

The nearby comment that says "We auto-expand urgent notifications." should say "We auto-expand notifications with critical urgency."

There is another comment that says "We use this function to grab focus when the user moves the pointer to an urgent notification that was already auto-expanded." That one should also say "a notification with critical urgency" instead of "an urgent notification".

::: js/ui/telepathyClient.js
@@ +490,2 @@
         this._setSummaryIcon(this.createNotificationIcon());
+        this._notification.setUrgency(MessageTray.Urgency.HIGH);

You should move this line to be right under
this._notification = new Notification(this);
Comment 3 Hellyna Ng 2011-01-04 09:37:52 UTC
Created attachment 177466 [details] [review]
I used the sort method against doing it manually. I assume that the sort method will be more efficient and cleaner looking, but there shouldn't be much of a difference.
Comment 4 Marina Zhurakhinskaya 2011-01-05 22:05:33 UTC
Review of attachment 177466 [details] [review]:

Looks great! See one minor comment correction below.

::: js/ui/messageTray.js
@@ +1615,2 @@
     // We use this function to grab focus when the user moves the pointer
+    // to a notification with CRITICAL urgency and was already auto-expanded.

"and was" => "that was"
Comment 5 Hellyna Ng 2011-01-05 22:49:24 UTC
Review of attachment 177466 [details] [review]:

commited by marinaz