GNOME Bugzilla – Bug 702460
don't replay banners after unlocking.
Last modified: 2015-02-23 22:57:40 UTC
While the implemented behavior is right accorging to the original design, the replay of banners after unlocking poses numerous problems. The design was to replay up to 10 banners after unlocking. The combination of a linear queue, and a timed window to react to individual banner makes this more of an annoyance. This threshold should be lowered all the way down to one so that only a summary banner is shown, similarly to how it's done in the overview. Activating this banner should open up the message tray. https://live.gnome.org/GnomeShell/Design/Guidelines/MessageTray/#Message_queuing
@Jakub Steiner Respected Sir, Is this bug still relevant with the new designs. Looking forward to hear from you. Thanking You, Devyani Kota
This is still relevant in 3.14. We still replay everything.
Created attachment 288983 [details] [review] The summary notification feature after the user resumes from the lock screen. Respected Sir, I have attached the patch for the summary notification feature, that will help the user to avoid multiple notifications when he resumes from the lock screen. He will be informed about the number of notifications that need acknowledgement. Please kindly look into it. Thanking You. Devyani.
Review of attachment 288983 [details] [review]: Please respect the coding style, see https://git.gnome.org/browse/gnome-shell/tree/HACKING. The commit message will also need some work - first, the subject is wrong: you are adding a summary notification whenever notifications queue up, not just when returning from the lock screen. Then you should add a small body explaining the reasoning behind the change (see http://lists.cairographics.org/archives/cairo/2008-September/015092.html). ::: js/ui/messageTray.js @@ +2435,3 @@ + if (showNextNotification){ + let len = this.notifications.length; + if(len>1){ As discussed on #gnome-design, I think the limit is too strict for the case when not coming back from the lock screen
Created attachment 289022 [details] [review] When notifications are queued up, a summary notification is displayed. Respected Sir, I have rectified my error according to the coding-style and added the reasoning in the commit message explaining the reason behind the change. And i have also increased the limit to three, as suggested. Please kindly look into it. Thanking You. Devyani.
Review of attachment 289022 [details] [review]: Please try to test a patch before attaching. ::: js/ui/messageTray.js @@ +2434,3 @@ let showNextNotification = (!limited || nextNotification.forFeedback || nextNotification.urgency == Urgency.CRITICAL); + if (showNextNotification){ + let len = this.notifications.length; this.notifications does not exist, so this throws an error @@ +2435,3 @@ + if (showNextNotification){ + let len = this.notifications.length; + if (len > 3) { No, that's not what I meant in the last review - the limit of 1 was fine when coming back from the lock screen, there should be a different limit in normal mode @@ +2436,3 @@ + let len = this.notifications.length; + if (len > 3) { + let source = new MessageTray.SystemNotificationSource(); MessageTray is undefined (for a good reason - you don't need to import a module from within itself) @@ +2439,3 @@ + let notification = new MessageTray.Notification(source, _("%d new messages".format(len))); + notification.setTransient(true); + messageTray.openTray(notification); Dito messageTray - but it any case, this is wrong. Opening the message tray should only ever be done as response to user action (in this case: clicking the notification or an action button in the notification, either option sounds fine to me). The message tray is modal (e.g. it grabs the mouse and keyboard), so if it just pops up at a random time (when a program sends a notification), it is very likely to interrupt (and rightfully annoy) the user. Also you still leave the notifications the summary notification is supposed to replace in the queue, so rather than reducing the load of messages, you are adding another one!
Created attachment 289116 [details] [review] Summary-notification feature that allows the user to view the notifications on acknowledgment. Respected Sir, I have rectified the errors, and also added the feature where the user gets to view the notifications that were summarized in the message tray, when the summarized-notification is clicked. And also i have removed the notifications that were being replaced by the summary-notification, from the queue. Please kindly look into it. Thanking You. Devyani.
Review of attachment 289116 [details] [review]: This still does not work, see comments below. You should also use a single patch, the current split does not make much sense. The commit message needs some work, too: - the summary is a bit confusing (what is an "on-click feature"?) How about: "messageTray: Summarize notifications when messages queue up" - the body still talks about the lock screen; while this is the issue reported in the bug, it's not the behavior in the patch, which summarizes notifications unconditionally - style nit: try not to assume that the user is male; you can simply use users/they to make it gender-neutral ::: js/ui/messageTray.js @@ +2437,3 @@ + if (len > 1) { + this._notificationQueue.length = 0; + let source = SystemNotificationSource(); This does not work - instances must be created with new @@ +2438,3 @@ + this._notificationQueue.length = 0; + let source = SystemNotificationSource(); + let notification = Notification(source, _("%d new messages".format(len))); Dto.
Created attachment 289152 [details] [review] Summarized-notification when messages queue up. Sir, I have attached the patch where the errors are rectified. Please kindly look into it. Thanking You. Devyani. messageTray: Summarize notifications when messages queue up It is really annoying for the user to acknowledge multiple notifications when they queue up.So, to prevent a notification flood that has to be handled by the user one-by-one, a summarized-notification feature is added which leaves a single summarized-notification for the user,replacing multiple notifications if the number exceeds 1, which they may or may not acknowledge. When this summarized-notification is acknowledged, the message-tray is opened where they can view the notifications that were summarized. This helps the user concentrate on his primary task simultaneously informing them about the new notifications.
Review of attachment 289152 [details] [review]: ::: js/ui/messageTray.js @@ +2437,3 @@ + if (len > 1) { + this._notificationQueue.length = 0; + let source = new SystemNotificationSource(); You don't add the source to the tray, so the notification is never shown!
Created attachment 289340 [details] [review] Sir, I have rectified my error and added the source to the tray so that the notification is displayed. Please kindly look into it. Thanking You. Devyani. MessageTray: Summarize notifications when messages queue up It is really annoying for the user to acknowledge multiple notifications when they queue up. So, to prevent a notification flood that has to be handled by the user one-by-one, a summarized-notification feature is added which leaves a single summarized-notification for the user, replacing multiple notifications if the number exceeds 1, which they may or may not acknowledge. When this summarized-notification is acknowledged, the message-tray is opened where they can view the notifications that were summarized. This helps the user concentrate on his primary task simultaneously informing them about the new notifications.
Created attachment 289375 [details] [review] The previous patch doesn't seem right, I have rectified my error, and added the source to the tray so that the notifications are shown. Please kindly review it. Thanking You. Devyani. messageTray: Summarize notifications when messages queue up It is really annoying for the user to acknowledge multiple notifications when they queue up. So, to prevent a notification flood that has to be handled by the user one-by-one, a summarized-notification feature is added which leaves a single summarized-notification for the user, replacing multiple notifications if the number exceeds 1, which they may or may not acknowledge. When this summarized-notification is acknowledged, the message-tray is opened where they can view the notifications that were summarized. This helps the user concentrate on his primary task simultaneously informing them about the new notifications.
Review of attachment 289375 [details] [review]: ::: js/ui/messageTray.js @@ +2439,3 @@ + let source = new SystemNotificationSource(); + this.add(source); + let notification = new Notification(source, _("%d new messages".format(len))); Even without the singular case, you should still use ngettext to handle language with different plural forms.
Created attachment 289406 [details] [review] Hello, I have used ngettext to handle language with different plural forms.Please kindly look into it. Thanking You. Devyani. messageTray: Summarize notifications when messages queue up It is really annoying for the user to acknowledge multiple notifications when they queue up. So, to prevent a notification flood that has to be handled by the user one-by-one, a summarized-notification feature is added which leaves a single summarized-notification for the user, replacing multiple notifications if the number exceeds 1, which they may or may not acknowledge. When this summarized-notification is acknowledged, the message-tray is opened where they can view the notifications that were summarized. This helps the user concentrate on his primary task simultaneously informing them about the new notifications.
Review of attachment 289406 [details] [review]: Code looks good to me now - Jakub, can you try this and give your design-ack as well?
I could only test it briefly with notify-send, but it appears to be working as intended. Looks good.
Great! That means we get to re-use strings from the overview and don't need to ask for a string freeze exception for the gnome-3-14 branch.
Comment on attachment 289406 [details] [review] Hello, Pushed to both master and gnome-3-14.