GNOME Bugzilla – Bug 630934
Chats should jump to the top of the notification queue
Last modified: 2011-01-05 23:17:48 UTC
Chats should jump to the top as they are received. Not above urgent messages, but above Gwibber or Rhythmbox messages.
Created attachment 176908 [details] [review] I hope everything is complete in this patch. Tell me if there's still anything wrong :).
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);
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.
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"
Review of attachment 177466 [details] [review]: commited by marinaz