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 638071 - shouldn't emit NotificationClosed dbus signal when a resident notification is clicked
shouldn't emit NotificationClosed dbus signal when a resident notification is...
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:
 
 
Reported: 2010-12-27 02:01 UTC by Jonathan Matthew
Modified: 2011-01-28 18:12 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
simple fix (1.18 KB, patch)
2010-12-27 02:03 UTC, Jonathan Matthew
reviewed Details | Review
Emit NotificationClosed for a notification iff we destroy it (5.31 KB, patch)
2011-01-28 00:00 UTC, Marina Zhurakhinskaya
none Details | Review
Emit NotificationClosed for a notification iff we destroy it (5.37 KB, patch)
2011-01-28 00:06 UTC, Marina Zhurakhinskaya
committed Details | Review

Description Jonathan Matthew 2010-12-27 02:01:23 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.
Comment 1 Jonathan Matthew 2010-12-27 02:03:41 UTC
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.
Comment 2 Marina Zhurakhinskaya 2011-01-26 06:41:00 UTC
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.
Comment 3 Dan Winship 2011-01-26 22:05:48 UTC
(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.
Comment 4 Marina Zhurakhinskaya 2011-01-28 00:00:50 UTC
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
Comment 5 Marina Zhurakhinskaya 2011-01-28 00:06:08 UTC
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 6 Dan Winship 2011-01-28 17:14:43 UTC
Comment on attachment 179482 [details] [review]
Emit NotificationClosed for a notification iff we destroy it

looks good