GNOME Bugzilla – Bug 630936
Have an indication that there are more queued notifications
Last modified: 2012-08-28 16:53:04 UTC
We need some indication that there are more queued notifications when a notification is being shown.
Created attachment 185355 [details] [review] Needs work! Right now,the patch indicates queued notifications.
Review of attachment 185355 [details] [review]: Thank you for taking a stab at this feature! The two issues I see with that patch are: 1) The style doesn't get updated if a second notification is added to or removed from the queue while a notification is showing. It's better to associate adding and removing the style with places where the set of notifications contained in this._notificationQueue is changed. We have different functions used for that: splice(), push(), shift(), and filter(). You can learn more about what each of these functions does here: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array Perhaps you can have a single function called _updateNotificationStyle(), which adds or removes the style class depending on the number of notifications in the queue. Then you can call this function after each of the above changes of the set of notifications in the queue. 2) Adding the border makes the notification in the banner mode sink down. If we really wanted the all-around border as a final effect, we would have to update the y value for the notification actor when the border is added/removed. However, as a quick fix, you can just use the right border alone, instead of the all-around border, to indicate that there are more queued notifications. The other things mentioned below are small issues. There are many trailing whitespaces in this patch. Please see http://live.gnome.org/GnomeShell/Development/WorkingWithPatches#How_to_rid_your_patch_of_trailing_whitespaces for info on how to ensure your patches don't have trailing whitespaces. ::: js/ui/messageTray.js @@ +1860,2 @@ this._notificationBin.show(); + // this._notification.actor.remove_style_class_name('notification-bordering'); Please don't leave commented out lines of code in the patches you attach. @@ +1910,3 @@ }; + if (!this._notification.expanded) + tweenParams.y = 0; This only adds trailing whitespaces. @@ +1969,3 @@ this._notificationBin.hide(); + if (this._notificationStyleAdd) + this._notification.actor.remove_style_class_name('notification-bordering'); You actually don't need to have this._notificationStyleAdd because it is just fine to call remove_style_class_name() even if the actor doesn't have the style being removed. It just won't do anything in that case. Also you should use true and false values, rather than 1 and 0 for booleans.
Created attachment 187591 [details] [review] Patch indicates queued notifications Update the style of right border when notifications are queued.
A few points before marinaz or even me review your patch: - There are still many trailing whitespaces, please fix that. "There are many trailing whitespaces in this patch. Please see http://live.gnome.org/GnomeShell/Development/WorkingWithPatches#How_to_rid_your_patch_of_trailing_whitespaces" or if you use vim "http://blog.hellyna.com/2010/12/auto-stripping-of-trailing-whitespace.html" - Seems like this patch is based on your previous patch, correct me if I am wrong. It should be based on origin/HEAD instead. Check out http://blog.hellyna.com/2011/04/quick-and-easy-development-with-git.html
Created attachment 188944 [details] [review] patch indicates queued notifications ready for designers' review
Created attachment 188969 [details] [review] patch indicates queued notifications ready for designers' review
Review of attachment 188969 [details] [review]: The patch looks very good and works as expected! Let's wait for the designers to come up with the effect for what we want to do if there are notifications queued after the one showing. Eventually, the patch will need a good commit message. See http://lists.cairographics.org/archives/cairo/2008-September/015092.html and the git log for what we'd like to have in the commit messages. ::: js/ui/messageTray.js @@ +2037,3 @@ + } + else { + this._notification.actor.remove_style_class_name('notification-queue'); You don't need curly braces if both parts of the if-else clause have only one line. I think 'more-notifications-queued' would be a clearer css class name.
Review of attachment 188969 [details] [review]: ::: js/ui/messageTray.js @@ +2037,3 @@ + } + else { + this._notification.actor.remove_style_class_name('notification-queue'); Actually, realized that we also need to check if this._notification is not null before doing anything with it. Putting if (!this._notification) return; in the beginning of this function would work. You can wait though for the design before making any further changes.
What does it look like?
Right now it is showing a right border as a place holder indicator that there are more notifications queued, but we need a design for what it should look like.
Created attachment 189708 [details] [review] Indicates that there are more queued notifications It would be helpful if the notifications have an indication that there are more messages waiting to show up, so that the user can pay heed without getting distracted more often.
Do we still want something like this?
Bug 677217 has the new design for how we should indicate that there are more queued notifications. *** This bug has been marked as a duplicate of bug 677217 ***