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 633412 - add support for notification spec version 1.2 hints
add support for notification spec version 1.2 hints
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: message-tray
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
Depends on:
Blocks: 630847
 
 
Reported: 2010-10-28 23:27 UTC by William Jon McCann
Modified: 2010-12-16 21:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
add support for resident notifications (2.46 KB, patch)
2010-10-30 06:26 UTC, Jonathan Matthew
reviewed Details | Review
NotificationDaemon: add support for resident notifications (6.61 KB, patch)
2010-11-29 21:13 UTC, Marina Zhurakhinskaya
none Details | Review
NotificationDaemon: add support for resident notifications (6.61 KB, patch)
2010-11-29 21:23 UTC, Marina Zhurakhinskaya
reviewed Details | Review
NotificationDaemon: add support for resident notifications (6.94 KB, patch)
2010-11-29 22:41 UTC, Marina Zhurakhinskaya
accepted-commit_now Details | Review
NotificationDaemon: add support for resident notifications (6.18 KB, patch)
2010-12-02 00:21 UTC, Marina Zhurakhinskaya
reviewed Details | Review
Move ICON_SIZE from Source to be a constant defined in messageTray.js (4.14 KB, patch)
2010-12-06 21:57 UTC, Marina Zhurakhinskaya
accepted-commit_now Details | Review
Support transient notifications (5.22 KB, patch)
2010-12-06 22:02 UTC, Marina Zhurakhinskaya
none Details | Review
NotificationDaemon: add support for transient notifications (5.25 KB, patch)
2010-12-06 22:07 UTC, Marina Zhurakhinskaya
reviewed Details | Review
NotificationDaemon: add support for resident notifications (9.07 KB, patch)
2010-12-10 20:27 UTC, Marina Zhurakhinskaya
committed Details | Review
NotificationDaemon: add support for transient notifications (8.92 KB, patch)
2010-12-15 21:32 UTC, Marina Zhurakhinskaya
committed Details | Review

Description William Jon McCann 2010-10-28 23:27:45 UTC
In master I've added two new hints:

 * "transient" - means that the notification should never be stored after it is displayed once.  In GNOME Shell terms this should be displayed when possible in a banner but not in the summary subsequent to that.
 * "resident" - means that the server should not automatically close the notification or remove the source when an action is invoked.  This is useful for Rhythmbox and other resident or ongoing sources.
Comment 1 Jonathan Matthew 2010-10-30 06:26:19 UTC
Created attachment 173546 [details] [review]
add support for resident notifications
Comment 2 Marina Zhurakhinskaya 2010-11-25 04:51:06 UTC
Review of attachment 173546 [details] [review]:

This is on the right track, but there are a few cases I could think of that we should address.

We should add

if (!notification.resident)
    notification.destroy();

to NotificationDaemon::_actionInvoked() because source.activated() no longer always destroys the source and the notification. The fact that notification.destroy() was missing from here caused the notifications associated with tray icons to stick around even after an action was invoked on them before your patch too. (Hence Rhythmbox notifications were not going away anyway.)

::: js/ui/notificationDaemon.js
@@ +301,2 @@
         notification.setUrgent(hints.urgency == Urgency.CRITICAL);
+        source.setResident(hints.resident == true);

I think 'resident' property should be set on a Notification because Source could have both resident and non-resident notifications associated with it over time and the value of the resident property depends on the notification currently shown. You can check (!this._isTrayIcon && !(this.notification && this.notification.resident) in Source::activated() .

@@ +422,3 @@
             return;
 
+        this.app.connect('notify::state', Lang.bind(this, this._appStateChanged));

Do we need to disconnect from this signal in destroy()?

@@ +444,2 @@
     _notificationClicked: function(notification) {
         notification.destroy();

This should not destroy the notification if it is resident, but should result in the notification popping down. You can achieve that for both new and summary notifications by calling Main.messageTray.escapeTray() .

Actually, perhaps _notificationClicked() in Source in messageTray.js should be calling Main.messageTray.escapeTray() because it is useful to do for all types of notifications and then all subclass implementations should be sure to call the base class method too. E.g. right now we leave the tray behind when you click on an "application ready" notification, which is not the right behavior.

@@ +467,3 @@
+        // destroy resident notification sources when their apps exit
+        if (this._isResident && this.app.get_state() == Shell.AppState.STOPPED) {
+            this.destroy();

No need for {}; It makes sense to destroy non-resident notifications too if the app associated with them exits, so can remove this._isResident check.

I imagine the tray icon will also be removed when the app exits and we will get 'message-icon-removed' signal, so it's ok not to check this._isTrayIcon here. However, maybe it is worth to check it and let the source be destroyed when the tray icon is removed if this._isTrayIcon is true.
Comment 3 Marina Zhurakhinskaya 2010-11-29 21:13:15 UTC
Created attachment 175497 [details] [review]
NotificationDaemon: add support for resident notifications

and updated it based on my review comments.

NotificationDaemon: add support for resident notifications

Resident notifications don't get removed when they are clicked or
one of their actions is invoked, and are only removed when the app
that created them exits.

Remove all types of notification sources when an app that created
them exits.

Make sure that we pop down the tray when a notification is clicked
or one of its actions is selected.

Based on the initial patch by Jonathan Matthew.
Comment 4 Marina Zhurakhinskaya 2010-11-29 21:15:10 UTC
The comment should have said:
"We would like to include this patch in the upcoming release so I went ahead
and updated it based on my review comments."
Comment 5 Marina Zhurakhinskaya 2010-11-29 21:23:52 UTC
Created attachment 175498 [details] [review]
NotificationDaemon: add support for resident notifications

Resident notifications don't get removed when they are clicked or
one of their actions is invoked, and are only removed when the app
that created them exits.

Remove all types of notification sources when an app that created
them exits.

Make sure that we pop down the tray when a notification is clicked
or one of its actions is selected.

Based on the initial patch by Jonathan Matthew.
Comment 6 Dan Winship 2010-11-29 21:44:25 UTC
Comment on attachment 175498 [details] [review]
NotificationDaemon: add support for resident notifications

>Resident notifications don't get removed when they are clicked or
>one of their actions is invoked, and are only removed when the app
>that created them exits.

or if it explicitly requests that they be removed.

or, AFAICT, if the app sends a non-resident notification after having sent a resident one

>Remove all types of notification sources when an app that created
>them exits.

won't that make notify-send useless?

>+    // Default implementation is to pop down the tray, but subclasses can add other actions.
>     _notificationClicked: function(notification) {
>+        Main.messageTray.escapeTray();
>     }

This seems like the wrong place for this code; the message tray should connect to the clicked signal and close itself.

>+    setResident: function(resident) {
>+        this._isResident = resident;
>+    },

NotificationDaemon.Source._isResident seems to be unused

>     activated: function() {
>-        if (!this._isTrayIcon)
>+        if (!this._isTrayIcon && !(this.notification && this.notification.resident))
>             this.destroy();
>     },

Do you actually need to check this.notification.resident? If there was a non-resident notification, wouldn't it already have been destroyed at this point?
Comment 7 Marina Zhurakhinskaya 2010-11-29 22:41:15 UTC
Created attachment 175503 [details] [review]
NotificationDaemon: add support for resident notifications

Resident notifications don't get removed when they are clicked or
one of their actions is invoked, and are only removed when the app
that created them exits, requests them to be removed or sends
another notification.

Remove all types of notification sources when an app that created
them exits.

Make sure that we pop down the tray when a notification is clicked
or one of its actions is selected.

Based on the initial patch by Jonathan Matthew.
Comment 8 Jonathan Matthew 2010-12-01 00:41:05 UTC
With the current patch, calling notify_notification_close on a resident notification does not remove the source.  If we're not going to remove the notification source when the app exits, we need some other way to destroy it.
Comment 9 Florian Müllner 2010-12-01 00:51:14 UTC
(In reply to comment #8)
> With the current patch, calling notify_notification_close on a resident
> notification does not remove the source.  If we're not going to remove the
> notification source when the app exits, we need some other way to destroy it.

Hmm, maybe we can remove the source when the application has exited and there is no source associated with it, e.g. defer the removal until both conditions are met? Jon?
Comment 10 William Jon McCann 2010-12-01 01:22:34 UTC
Seems to me like calling close on a notification should remove the source.  What reason is there for it not to?
Comment 11 Marina Zhurakhinskaya 2010-12-02 00:21:12 UTC
Created attachment 175685 [details] [review]
NotificationDaemon: add support for resident notifications

The attached patch doesn't remove a source if an applications associated
with it exits, but it removes a source if the notification associated
with it is removed.
Comment 12 Marina Zhurakhinskaya 2010-12-06 21:57:52 UTC
Created attachment 175960 [details] [review]
Move ICON_SIZE from Source to be a constant defined in messageTray.js

This is done in preparation for transient notifications that don't
have a source.
Comment 13 Marina Zhurakhinskaya 2010-12-06 22:02:59 UTC
Created attachment 175961 [details] [review]
Support transient notifications

Transient notifications are removed after being shown and are not
represented in the tray by a source icon.

Transient notifications are implemented as not being associated
with any source, so that they don't replace the latest non-transient
notification associated with the source.
Comment 14 Marina Zhurakhinskaya 2010-12-06 22:07:55 UTC
Created attachment 175963 [details] [review]
NotificationDaemon: add support for transient notifications

Just updated the commit message title to match the commit message title
for resident notifications support.
Comment 15 Dan Winship 2010-12-08 12:16:45 UTC
Comment on attachment 175685 [details] [review]
NotificationDaemon: add support for resident notifications

>+        this.resident = false;

You're adding the resident property in messageTray.js, but its semantics are entirely defined by notificationDaemon.js... It would be better to have NotificationDaemon subclass Notification, and put the resident property there.

Either that, or Notification could destroy itself on click, if !resident, and then the Source behavior would be updated to deal with that.
Comment 16 Dan Winship 2010-12-08 12:31:12 UTC
Comment on attachment 175963 [details] [review]
NotificationDaemon: add support for transient notifications

I'm a little worried about this, given that it used to be a hard requirement that notifications were always associated with sources. Seems like something that's going to get broken repeatedly in the future. It might be cleaner to have transient *sources* rather than transient *notifications*, with the summary area knowing not to display transient sources, and then any other special behavior that they need would be handled by the transient source code, rather than being spread out across Notification, Source, and NotificationDaemon

comments from my review before I wrote the above:

>+        this.transientProperty = false;

why not just "transient" like the other properties?

>+        // params.icon should always be defined for transient notifications
>+        // that don't have a source.

You should enforce that rather than just having a comment.
if (!this.source && !params.icon) throw new Error...

>+    notify: function(notification) {
>+        this._onNotify(null, notification);
>+    },
>+
>     _onNotify: function(source, notification) {

would be clearer to do this the other way around. (Have notify() contain the code, and _onNotify just calls this.notify(notification))

>         notification.setResident(hints.resident == true);
>+        notification.setTransient(hints['transient'] == true);

"hints.transient", to be consistent with the others
Comment 17 Marina Zhurakhinskaya 2010-12-10 20:27:44 UTC
Created attachment 176204 [details] [review]
NotificationDaemon: add support for resident notifications

Resident notifications don't get removed when they are clicked or
one of their actions is invoked, and are only removed when the app
that created them requests them to be removed or sends another
notification.

Remove the source when a notification associated with it is removed.
Except if the source is a tray icon.

Make sure that we pop down the tray when a notification is clicked
or one of the actions of a non-resident notification is selected.

Based on the initial patch by Jonathan Matthew.
Comment 18 Marina Zhurakhinskaya 2010-12-15 21:32:35 UTC
Created attachment 176503 [details] [review]
NotificationDaemon: add support for transient notifications

Transient notifications are removed after being shown. If the summary
is being shown while they appear, they are represented in it by a new
source icon.

We always create a new source for new transient notifications to
ensure that they don't replace the latest persistent notification
associated with the source. Because we generally don't want any
new or resident notifications to be replaced by others, associating
multiple notifications with a source is the next thing we will
implement.
Comment 19 Dan Winship 2010-12-16 13:20:43 UTC
Comment on attachment 176204 [details] [review]
NotificationDaemon: add support for resident notifications

good to go, except:

>+    activated: function() {
>+        if (this.notification && !this.notification.resident)
>+            this.notification.destroy();
>+    },

AFAICT, this is now only used by _onFocusAppChanged. If so, then it should get a clearer name, like "appFocused" or something. (Or maybe even just get merged into _onFocusAppChanged?)
Comment 20 Dan Winship 2010-12-16 13:26:43 UTC
Comment on attachment 176503 [details] [review]
NotificationDaemon: add support for transient notifications

seems right
Comment 21 Marina Zhurakhinskaya 2010-12-16 21:00:52 UTC
Comment on attachment 176204 [details] [review]
NotificationDaemon: add support for resident notifications

Rolled in the code from activated() into _onFocusAppChanged() .