GNOME Bugzilla – Bug 633412
add support for notification spec version 1.2 hints
Last modified: 2010-12-16 21:16:07 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.
Created attachment 173546 [details] [review] add support for resident notifications
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.
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.
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."
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 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?
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.
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.
(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?
Seems to me like calling close on a notification should remove the source. What reason is there for it not to?
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.
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.
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.
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 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 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
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.
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 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 on attachment 176503 [details] [review] NotificationDaemon: add support for transient notifications seems right
Comment on attachment 176204 [details] [review] NotificationDaemon: add support for resident notifications Rolled in the code from activated() into _onFocusAppChanged() .