GNOME Bugzilla – Bug 631042
don't destroy trayicons when dismissing their notifications
Last modified: 2010-10-12 21:22:25 UTC
Right now if a trayicon shows a notification, and you click to dismiss it, then you lose the trayicon. Oops. First patch is a cleanup, second is the fix.
Created attachment 171519 [details] [review] MessageTray: untangle click notifications Previously, when you clicked on a notification, it would call this.source.clicked(), which would emit a 'clicked' signal on the source, and then various other stuff would happen from there. This used to make a little bit of sense, when clicking on a notification was supposed to do the same thing as clicking on its source, but makes less sense now, when clicking on the source itself *doesn't* call source.clicked()... Change it so that when you click on a notification, the notification emits 'clicked' itself, and the source notices that and calls its notificationClicked() method, and the various source subclasses do what they need to do with that, and Source no longer has a clicked method/signal.
Created attachment 171520 [details] [review] NotificationDaemon: don't destroy trayicons when dismissing their notifications Tray icons control their own lifespan; they're not supposed to disappear when you dismiss their notifications like non-trayicon notification sources do.
Review of attachment 171519 [details] [review]: Looks good. Good to commit once you address the comment below. ::: js/ui/messageTray.js @@ +679,3 @@ }, + notificationClicked: function(notification) { Could you add throw new Error('no implementation of notificationClicked in ' + this); here and move it next to createNotificationIcon(). Otherwise it should at least have a comment "// Nothing to do." We don't necessarily have to call the implementation of the function in the base class from the functions that override it in sub classes, do we?
Review of attachment 171520 [details] [review]: There are two other cases in the notificationDaemon.js when the source is being destroyed, which should no longer happen if the source is associated with the tray icon: in _onFocusAppChanged() and in _actionInvoked() So maybe it will make sense to have some function like destroyIfNotTrayIcon() in Source in notificationDaemon.js We also need to make sure to hide the notification if it was clicked or if its action was invoked. The question is do we destroy the notification or do we keep it around associated with the source? For example, when you click on the Rhythmbox notification, it would be right for us to keep it around. However, for some other notifications that don't have meaningful controls, it's not. I'd just say let's keep it around as a safer default action :). ::: js/ui/notificationDaemon.js @@ +435,3 @@ + + if (!this._isTrayIcon) + this.destroy(); this.destroy(); also resulted in the notification being destroyed, which ensured that it was hidden; now we need to make a separate call to hide the notification.
Created attachment 172126 [details] [review] MessageTray: untangle click notifications renames notificationClicked to _notificationClicked to be consistent with other protected methods, and clarifies that it doesn't need to be implemented
Created attachment 172127 [details] [review] NotificationDaemon: don't destroy trayicons when dismissing their notifications fix to not destroy on app activate or action click either
Review of attachment 172126 [details] [review]: Looks good.
Review of attachment 172127 [details] [review]: Looks good. As mentioned in the previous review, destroying a notification once the user interacts with it will not always be the right thing to do, but it is an ok default behavior.
Attachment 172126 [details] pushed as de16108 - MessageTray: untangle click notifications Attachment 172127 [details] pushed as bbdf88c - NotificationDaemon: don't destroy trayicons when dismissing their notifications