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 702460 - don't replay banners after unlocking.
don't replay banners after unlocking.
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
3.14.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-06-17 13:24 UTC by Jakub Steiner
Modified: 2015-02-23 22:57 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
The summary notification feature after the user resumes from the lock screen. (1.62 KB, patch)
2014-10-20 20:49 UTC, Devyani [devyani7]
needs-work Details | Review
When notifications are queued up, a summary notification is displayed. (2.07 KB, patch)
2014-10-21 11:56 UTC, Devyani [devyani7]
needs-work Details | Review
Summary-notification feature that allows the user to view the notifications on acknowledgment. (2.51 KB, patch)
2014-10-22 11:14 UTC, Devyani [devyani7]
needs-work Details | Review
Summarized-notification when messages queue up. (2.46 KB, patch)
2014-10-22 17:36 UTC, Devyani [devyani7]
needs-work Details | Review
Sir, I have rectified my error and added the source to the tray (1.60 KB, patch)
2014-10-26 12:35 UTC, Devyani [devyani7]
none Details | Review
The previous patch doesn't seem right, I have rectified my error, and (2.51 KB, patch)
2014-10-26 18:28 UTC, Devyani [devyani7]
reviewed Details | Review
Hello, (2.54 KB, patch)
2014-10-27 11:50 UTC, Devyani [devyani7]
committed Details | Review

Description Jakub Steiner 2013-06-17 13:24:21 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
Comment 1 Devyani [devyani7] 2014-09-26 20:39:53 UTC
@Jakub Steiner
Respected Sir,
    Is this bug still relevant with the new designs. Looking forward to hear from you.
Thanking You,
Devyani Kota
Comment 2 Jakub Steiner 2014-09-29 18:06:24 UTC
This is still relevant in 3.14. We still replay everything.
Comment 3 Devyani [devyani7] 2014-10-20 20:49:42 UTC
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.
Comment 4 Florian Müllner 2014-10-20 20:59:24 UTC
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
Comment 5 Devyani [devyani7] 2014-10-21 11:56:51 UTC
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.
Comment 6 Florian Müllner 2014-10-21 13:59:19 UTC
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!
Comment 7 Devyani [devyani7] 2014-10-22 11:14:45 UTC
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.
Comment 8 Florian Müllner 2014-10-22 14:58:28 UTC
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.
Comment 9 Devyani [devyani7] 2014-10-22 17:36:57 UTC
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.
Comment 10 Florian Müllner 2014-10-22 17:44:26 UTC
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!
Comment 11 Devyani [devyani7] 2014-10-26 12:35:55 UTC
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.
Comment 12 Devyani [devyani7] 2014-10-26 18:28:20 UTC
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.
Comment 13 Florian Müllner 2014-10-26 20:19:11 UTC
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.
Comment 14 Devyani [devyani7] 2014-10-27 11:50:34 UTC
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.
Comment 15 Florian Müllner 2014-10-27 17:07:57 UTC
Review of attachment 289406 [details] [review]:

Code looks good to me now - Jakub, can you try this and give your design-ack as well?
Comment 16 Jakub Steiner 2014-10-27 17:15:28 UTC
I could only test it briefly with notify-send, but it appears to be working as intended. Looks good.
Comment 17 Florian Müllner 2014-10-27 17:17:04 UTC
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 18 Florian Müllner 2014-10-27 17:24:52 UTC
Comment on attachment 289406 [details] [review]
Hello,

Pushed to both master and gnome-3-14.