GNOME Bugzilla – Bug 638071
shouldn't emit NotificationClosed dbus signal when a resident notification is clicked
Last modified: 2011-01-28 18:12:13 UTC
Currently if you click on a rhythmbox track notification, it stays in the tray but the action buttons stop working. This happens because the NotificationClosed dbus signal is emitted.
Created attachment 177078 [details] [review] simple fix This just checks if the notification is resident before emitting the NotificationClosed signal. Maybe we should add an accessor to Notification for this.
Review of attachment 177078 [details] [review]: The solution looks right for resident notifications, but it occurred to me that we are not emitting NotificationClosed in some other cases when we probably should emit it and that we should probably just move it to be on 'destroy', which will fix the behavior for resident notifications because we don't destroy them. We currently only emit NotificationClosed when - a notification is clicked - a notification is closed by the app - on all Empathy notifications We destroy the notification, but don't emit NotificationClosed now when - a transient notification is destroyed after being shown - a non-resident notification is destroyed after an app associated with it is focused - a notification is destroyed after a tray icon it was associated with is removed We should probably emit NotificationClosed in all of the above cases and also make sure we are supplying the right reason. We also destroy the notification after we emit ActionInvoked, and in this case I'm not sure if it is ok to also emit NotificationClosed. Jon, would that be fine? What would the reason then be? DISMISSED? I think we should pass the information on why the notification was destroyed when emitting the 'destroy' signal and emit NotificationClosed from where we connect to it based on that. (The information on why the notification was destroyed is useful if we don't want to emit NotificationClosed after emitting ActionInvoked or if we want to use a reason other than DISMISSED in some of the above cases, such as when a transient notification is destroyed after being shown (is that DISMISSED or EXPIRED?) or when a notification is destroyed because the app that sent it is focused (which one of the existing reasons should we use or do we need a new one?). ::: js/ui/notificationDaemon.js @@ +325,3 @@ + if (!n.resident) { + this._emitNotificationClosed(id, NotificationClosedReason.DISMISSED); + } We don't use curly braces for a single line block.
(In reply to comment #2) > We destroy the notification, but don't emit NotificationClosed now when > - a transient notification is destroyed after being shown Probably wrong, although I don't know if there's a formal writeup of the behavior of "transient" yet? > - a non-resident notification is destroyed after an app associated with it is > focused > - a notification is destroyed after a tray icon it was associated with is > removed Those two are definitely wrong. > We also destroy the notification after we emit ActionInvoked, and in this case > I'm not sure if it is ok to also emit NotificationClosed. The spec doesn't say... see what notification-daemon does I guess? > above cases, such as when a transient notification is destroyed after being > shown (is that DISMISSED or EXPIRED?) http://www.galago-project.org/specs/notification/0.9/x408.html#signal-notification-closed Looks like EXPIRED. > or when a notification is destroyed > because the app that sent it is focused (which one of the existing reasons > should we use or do we need a new one?). Again, EXPIRED is closest, although perhaps we would eventually want to add more reasons.
Created attachment 179481 [details] [review] Emit NotificationClosed for a notification iff we destroy it This fixes emitting NotificationClosed for resident notifications that are clicked, but are not actually destroyed. This also ensures that we emit NotificationClosed in all cases when a notification is destroyed, which can happen when: - a non-resident notification is clicked - an action is invoked on a non-resident notification - a transient notification is done showing - a notification was requested to be closed by the application - a tray icon the notification was associated with is removed
Created attachment 179482 [details] [review] Emit NotificationClosed for a notification iff we destroy it This fixes emitting NotificationClosed for resident notifications that are clicked, but are not actually destroyed. This also ensures that we emit NotificationClosed in all cases when a notification is destroyed, which can happen when: - a non-resident notification is clicked - an action is invoked on a non-resident notification - an application the notification was associated with is focused - a transient notification is done showing - a notification was requested to be closed by the application - a tray icon the notification was associated with is removed Jon said that we should emit NotificationClosed in all cases when we destroy the notification, including when we destroy it because an action was invoked on it and emit ActionInvoked. He didn't think it is very important what reasons we use in what cases, but EXPIRED made sense for transient notifications that the user did not interact with, and DISMISSED made sense for any case when a notification is dismissed due to the user's action (which includes the user focusing an associated application).
Comment on attachment 179482 [details] [review] Emit NotificationClosed for a notification iff we destroy it looks good