GNOME Bugzilla – Bug 607375
Allow notification updates / removal
Last modified: 2010-03-15 19:52:52 UTC
According to the notification spec, applications should be able to modify/close notifications on-the-fly. Currently, modifications will queue a second notification, while close request only affect the summary icon.
Created attachment 151722 [details] [review] Allow notification updates / removal According to the desktop notification spec, an application may update or close its notifications while shown on-screen.
Created attachment 151729 [details] [review] Allow notification updates / removal The previous patch had a problem with the message tray hiding when dicarding the most recent notification in the summary mode.
Created attachment 151857 [details] [review] Allow notification updates / removal Minor cleanup compared to the previous patch - mostly renaming of update/_update to something less general ...
Some clarification about what this patch tries to achieve (as requested by mccann on IRC) The spec[1] allows for notification updates and removals. Updates are handled by the replaces_id parameter to org.freedesktop.Notifications.Notify: "The optional notification ID that this notification replaces. The server must atomically (ie with no flicker or other visual cues) replace the given notification with this one. This allows clients to effectively modify the notification while it's active." Using libnotify this is done with notify_notification_update(), followed by notify_notification_show(). For notification cancellation, there is org.freedesktop.Notifications.CloseNotification: "Causes a notification to be forcefully closed and removed from the user's view. It can be used, for example, in the event that what the notification pertains to is no longer relevant, or to cancel a notification with no expiration time." With libnotify, this is achieved using notify_notification_close(). IMHO the wording is pretty clear: both updates and cancellations are mandatory. The notification-daemon implementation in shell does handle most of this already (the only exception being icon updates). The UI on the other hand ignores pretty much any change requests (it does remove icons from the summary view on closed signals). [1] http://people.canonical.com/~agateau/notifications-1.1/spec/ar01s09.html#commands
Created attachment 151993 [details] Small test program
Created attachment 151995 [details] [review] Allow notification updates / removal The previous version failed to delete notifications which were cancelled before being displayed (yeah test-program!)
Comment on attachment 151995 [details] [review] Allow notification updates / removal This mixes up the concepts of "notification" and "source". In particular, wrt to existing libnotify users, a "source" is the application as a whole, but it may have multiple notifications present at a given time. Given then new tray UI mockups (http://www.gnome.org/~mccann/screenshots/clips/20100122204135/) there's going to need to be some additional code reorganization, so I especially don't like the reorganizey parts of this patch right now. You might want to wait until we've figured things out a little bit more before redoing it again.
(In reply to comment #7) > (From update of attachment 151995 [details] [review]) > This mixes up the concepts of "notification" and "source". Yeah, right. Although IMO this mix-up starts when handling the Notify signal (with replace_id > 0). > In particular, wrt to existing libnotify users, a "source" is the application > as a whole, but it may have multiple notifications present at a given time. I agree. IMO we should adjust the implementation of notificationDaemon to identify "source" by the "appName" parameter and not the id (which refers to a single notification). > You might want to wait until we've figured things out a little bit more before > redoing it again. Sounds reasonable - especially since we'll probably require some extensions to the spec.
(In reply to comment #8) > > In particular, wrt to existing libnotify users, a "source" is the application > > as a whole, but it may have multiple notifications present at a given time. > > I agree. IMO we should adjust the implementation of notificationDaemon to > identify "source" by the "appName" parameter and not the id (which refers to a > single notification). Right. The other changes I was talking about are mostly in, so now would be a good time to redo this patch.
Created attachment 153039 [details] [review] [Notifications] Associate sources with applications Use the "appName" parameter in notifications to identify the source rather than the id - use the latter to enable update and removal of individual notifications as laid out in the desktop notification spec. Requires definitively more work, but should be good enough for some ui-review to determine whether this is the direction the tray should be heading to.
Comment on attachment 153039 [details] [review] [Notifications] Associate sources with applications mmm... ok, i lied, notifications are still in flux. you'll probably want to skim the non-dbus non-telepathy-speaking-to parts of the patch in bug 608999. That patch isn't ready yet (I just posted it so Owen could grab to it for a demo), but you can see where parts of it are headed. Basically the idea is that MessageTray.Notification will be a base class (like MessageTray.Source), that will have multiple implementations; for libnotify notifications, for telepathy-based chats, for graphical OSD notifications (bug 607029), possibly additional ones for trayicons-in-the-messagetray (bug 608869) and needs-attention notification (though those might be able to just use the default) So the base Notification class will need methods for building up notifications in the standard style, and the subclasses will use those to build their notifications. But eg, notification IDs belong in notificationDaemon's subclass of Notification, not in the base class, since other types aren't going to have IDs associated with them. Likewise, the notification shouldn't dismiss itself when the source is clicked, because that might not be correct for all kinds of sources. I'll work on splitting out and pushing the Notification base class modifications from the telepath patch out, and then try to rebase this on top of that.
Created attachment 154251 [details] [review] Associate sources with applications Here is a rebased patch. This makes Empathy be a single source too and replaces in-place its notifications from the same person, so we need to create a follow up patch to make Empathy a special case.
Review of attachment 154251 [details] [review]: OK, as I already said on IRC, the code is pretty close to my own patch, so 1. it makes perfect sense to me 2. I'm not the most qualified person for this review That said, I still found some minor things - mostly stylistic, with one exception: If a notification is closed by its application, the notification is removed from the message tray without showing the summary afterwards. If a notification is dismissed by the user, the notification is removed from the message tray and we show the summary afterwards. Is that by design? The second case doesn't really make sense to me (but then it may be just me + my timezone) Right, now for the nitpicking stuff: ::: js/ui/messageTray.js @@ +488,3 @@ } + for (let i in this._notificationQueue) + I think we generally avoid using for ... in with array (and yes, I know who wrote this line initially) ::: js/ui/notificationDaemon.js @@ +125,1 @@ + // Source may be null if we never receive a notification from typo? received makes more sense to me here ...
Created attachment 154254 [details] [review] Associate sources with applications Fixed the nitpicks and the whitespaces. The reason we are not typically showing the summary when the notification is removed by the application, but are showing it when the notification is dismissed by the user is that it all just depends on whether or not the set of icons in the summary has changed since the last time it was shown. When the notification is dismissed by the application, we are not removing the icon from the summary, so if the icon was there before the summary stays the same. When the notification is dismissed by the user, we are removing the icon from the summary, so if the icon was there before the summary changes and we show it. Not removing the summary icon on the application cancellation of the notification, but removing it on the user's dismissal of the notification makes some sense. We can treat the user interacting with one of the notifications associated with the source as the user's acknowledgement of all the notifications that came from it. However, we don't want to remove the summary icon as the user's reminder about multiple notifications from the source if one of them happened to be cancelled by the application. To be able to better handle these situations, know when the source no longer has any unacknowledged notifications associated with it, and prepare for showing all unacknowledged notifications on hover over the source, we should start keeping an {id, notification} map of current notifications in the Source object. We should then update all the clicked/dismissed/destroy code.
Created attachment 154255 [details] [review] Special-handle Empathy to have multiple sources and to queue notifications There is a detailed comment about how the Empathy notifications are handled and what are some of the known problems in the patch. These problems were also present in the old code before we started associating applications with a single source and enabled notification replacement. The reason to have this special handling is to keep the Empathy notifications behavior closer to the eventual design before we add all the code for that uses Empathy/Telepathy directly.
Created attachment 154259 [details] [review] [messageTray] Fix some markup issues If the notification body contains '&' it ends up empty and a warning about an invalid entity is printed on stderr, so our escape code must handle ampersands as well. There's another issue with NotificationDaemon escaping all markup in titles resulting in valid tags (b,i,u) being escaped with < > The regex used in this patch would look a lot nicer if it matched generic entities (something along the lines of &[^ ;]+;) - unfortunately, clutter will fall back to not display any text when an unknown entity is encountered, so we stick to some known good ones.
Comment on attachment 154254 [details] [review] Associate sources with applications >+ if (this._notification == notification && this._notificationState == State.SHOWN) { SHOWN or SHOWING >+ for (let i = 0; i < this._notificationQueue.length; i++) { >+ if (this._notificationQueue[i] == notification) { >+ this._notificationQueue.splice(i, 1); let index = this._notificationQueue.indexOf(notification); if (index != -1) this._notificationQueue.splice(index, 1); >+ getNotification: function(id) { this assumes that IDs are unique across different kinds of notifications, which nothing is enforcing (and in particular, notification IDs from notificationDaemon.js are just small integers, which could easily conflict with other notification types). In general, it doesn't seem like this belongs on MessageTray, it should be on Source. Then the IDs only have to be unique within a given source. And this isn't a problem, because everywhere you call getNotification(), you know the Source you're talking about already. >- if (notificationExpired) >+ if (notificationExpired || this._notificationRemoved) stylistically, it would be more consistent to add "|| this._notificationRemoved" to the definition of notificationExpired. >- let summaryPinned = this._summaryTimeoutId != 0 || this._pointerInTray || summarySummoned; >+ let summaryPinned = this._summaryTimeoutId != 0 || (this._pointerInTray && !this._notificationRemoved) || summarySummoned; I don't understand the reason for this change. >+ this._notificationRemoved = false; you should clear this in _hideNotification(), not _updateState(). As it is now, if a notification was removed while _notificationState == State.SHOWING, then the first run through _updateState() wouldn't change anything, but then you'd end up clearing _notificationRemoved at the end, and then when the tray finished SHOWING and switched to SHOWN, _notificationRemoved would already have been cleared, and so you'd end up continuing to show the already-removed notification. >+ _set_icon_and_hints: function(icon, hints) { I'd call this "_update"
Comment on attachment 154255 [details] [review] Special-handle Empathy to have multiple sources and to queue notifications if we're going to make a tarball release that includes the previous patch, then ok
Comment on attachment 154259 [details] [review] [messageTray] Fix some markup issues >There's another issue with NotificationDaemon escaping all markup in >titles resulting in valid tags (b,i,u) being escaped with < > the title is not supposed to have markup, according to the spec, so if it contains "<b>", that means that the sender wants the string "<b>" to appear in the title of the notification, not that they want the title to be bold. (More likely, it means that the sender was confused, but anyway, compare the behavior of notification-daemon here: notify-send "<b>foo</b>" bar) >+ let _text = text.replace(/&([^(amp)(lt)(gt)][^;])/g, "&$1"); I do not think it means what you think it means. [^(amp)(lt)(gt)] means "any single character except a, m, p, l, t, g, (, or )" gjs> "a&b".replace(/&([^(amp)(lt)(gt)][^;])/g, "&$1") a&b gjs> "a&ba".replace(/&([^(amp)(lt)(gt)][^;])/g, "&$1") a&ba gjs> "a&aa".replace(/&([^(amp)(lt)(gt)][^;])/g, "&$1") a&aa
Created attachment 154276 [details] [review] [messageTray] Fix text with ampersands not being displayed (In reply to comment #19) > the title is not supposed to have markup, according to the spec, so if it > contains "<b>", that means that the sender wants the string "<b>" to appear > in the title of the notification OK, I wasn't aware what the spec says about markup there - sorry about that then; removed from patch. > >+ let _text = text.replace(/&([^(amp)(lt)(gt)][^;])/g, "&$1"); > > I do not think it means what you think it means. Mmmmh, crap ... not much JS experience before gnome-shell hacking, so it's nice to learn that negative lookaheads are actually supported - again, sorry for the noise.
Mmmh, I got a rhythmbox notification containing ' - is it OK to squash that into the ampersand patch before pushing?
yes. ought to make sure we accept everything pango accepts eventually
Comment on attachment 154276 [details] [review] [messageTray] Fix text with ampersands not being displayed Attachment 154276 [details] pushed as 79fe60e - [messageTray] Fix text with ampersands not being displayed
Review of attachment 154254 [details] [review]: OK, so I spotted another problem which escaped my last review: ::: js/ui/notificationDaemon.js @@ +238,3 @@ MessageTray.Source.prototype._init.call(this, sourceId); + }, + this._set_icon_and_hints(icon, hints); This split is no longer correct - only the icon/hint stuff can be moved to a separate function, the initialization of the application-opening stuff has to remain in _init() or notification updates will reset this.app to null and break the app-opening functionality.
Created attachment 154441 [details] [review] Associate sources with applications Applied all of Dan's feedback. > >- let summaryPinned = this._summaryTimeoutId != 0 || this._pointerInTray || summarySummoned; > >+ let summaryPinned = this._summaryTimeoutId != 0 || (this._pointerInTray && !this._notificationRemoved) || summarySummoned; > > I don't understand the reason for this change. > It was meant to take care of hiding the summary and the tray when the user receives the notification while the summary is being shown and then dismisses the notification by clicking on its icon. I think the more reasonable thing to happen in that case is for the tray to disappear, even though the pointer is in tray and the summary was being shown prior to the notification. However, this change alone doesn't seem to result in this behavior, so I removed it for now and will fix that behavior in a separate patch later (since it's not a big deal either way).
Created attachment 154445 [details] [review] Special-handle Empathy to have multiple sources and to queue notifications A rebase on top of the previous patch.
this was all committed a while ago