GNOME Bugzilla – Bug 710137
Implement support for the new GTK+ notifications API
Last modified: 2013-10-21 19:21:12 UTC
This is an extremely simple API that will be used by GNotification. It's going to be a private API at first, but if wanted, we could specify it at the end of the next cycle or so. Right now, it's fairly simple and doesn't support sounds, title/body markup, icons for actions, text entries, silent updates, etc., but these things could be added if app authors want them. The API is intentionally very dumb for the first release. This API, since it uses app-as-service activation with the new FDO Application interface to trigger actions, also supports persistence even after the app exits, so write the state of all of the new notifications to disk so that they will continue to be around after a shell restart.
Created attachment 257294 [details] [review] messageTray: Replace setButtonSensitive by simply returning the button We want to remove 'id's from buttons, and simply returning the button actor is more powerful anyway.
Created attachment 257295 [details] [review] messageTray: Split out addButton to allow consumers to pass a pre-made button Some consumers may want to construct their buttons specially, so allow them to do that by adding a new API that takes a button instead of a label.
Created attachment 257296 [details] [review] messageTray: Remove useActionIcons feature This can be put in the legacy notification daemon with the new addButtonFull API, to create icon names for actions.
Created attachment 257297 [details] [review] messageTray: Make addButton take a callback This is a much simpler API for consumers to manage.
Created attachment 257298 [details] [review] messageTray: Don't always open the source when clicking on the notification Some consumers may not always want to open the app, so make clients that want to do this explicitly connect to the 'clicked' signal.
Created attachment 257299 [details] [review] notificationDaemon: Move nextNotificationId inside the daemon class This won't be used by the new notification daemon.
Created attachment 257300 [details] [review] notificationDaemon: Rename the existing implementation to the FdoNotificationSource We'll add a new, simpler private implementation that's used by the new GNotification API in gio.
Created attachment 257301 [details] [review] notificationDaemon: Implement the new GTK+ notifications API The new API is designed to support features like persistence and uses the new org.freedesktop.Application specification for activating actions on notifications. While we won't add support for persistence yet, implement the new notification spec with parity of the old one.
Created attachment 257302 [details] [review] shell-global: Handle empty variants better In cases where we have an array of 0 elements or similar, the data returned may be NULL. Since g_file_replace_contents will assert in this case, simply check for this and delete the file instead.
Created attachment 257303 [details] [review] shell-global: Add new APIs for saving/loading persistent state
Created attachment 257304 [details] [review] notificationDaemon: Write notifications out to disk This allows notifications to persist even after reboots and gnome-shell restarts.
Review of attachment 257294 [details] [review]: Ok
Review of attachment 257295 [details] [review]: Mh... addButtonFull() is really a bad name. What about accepting either a button or a string in addButton()?
Review of attachment 257296 [details] [review]: Ok
Review of attachment 257297 [details] [review]: ::: js/ui/components/telepathyClient.js @@ +1172,1 @@ })); This one is resident for some reason, so you probably want to destroy() at the end? ::: js/ui/status/bluetooth.js @@ +248,1 @@ }, This one is resident too, so you must destroy() at the end of _ok and _cancel
Review of attachment 257298 [details] [review]: If the consumer doesn't want to open the app, then it should make open() an empty function. Also, I'm not sure which consumer you're referring to here. ::: js/ui/notificationDaemon.js @@ +455,3 @@ + this.open(); + this._emitActionInvoked(ndata.id, 'default'); + })); 'default' should be emitted only if the notification has a default action, and only if no other action is performed (such as opening the application)
Review of attachment 257299 [details] [review]: Ok
Review of attachment 257300 [details] [review]: Ok
(In reply to comment #16) > If the consumer doesn't want to open the app, then it should make open() an > empty function. > Also, I'm not sure which consumer you're referring to here. So, right-clicking on the source and choosing "Open" will do nothing? The consumer here is the new GtkNotification API, which will do one of two things when clicking a notification: * If a default action is specified, call ActivateAction on the app with the default action specified. * Otherwise, call Activate on the app. In particular, we don't want to call Activate in the case where we have a default action, but we want to keep the right-click-on-source "Open" menu item working.
(In reply to comment #13) > Review of attachment 257295 [details] [review]: > > Mh... addButtonFull() is really a bad name. What about accepting either a > button or a string in addButton()? I don't like the name addButtonFull either. Perhaps we should make addButton take a button, and have addButtonFromLabel? We're changing the API to take a callback anyway, so I don't care about breakage.
Review of attachment 257301 [details] [review]: ::: js/ui/notificationDaemon.js @@ +709,3 @@ + _init: function(source, notification) { + let { title: title, body: body, gicon: gicon, actions: actions, defaultAction: defaultAction } = notification; + this.parent(source, title.unpack(), body ? body.unpack() : null, { gicon: gicon }); You can't send null across dbus, so how can body be null or undefined? @@ +711,3 @@ + this.parent(source, title.unpack(), body ? body.unpack() : null, { gicon: gicon }); + + this._defaultAction = defaultAction ? defaultAction.unpack() : null; Same here for defaultAction. @@ +718,3 @@ + this._activateAction(actionId, target); + })); + })); Braces around an if statement with a multiline body. @@ +747,3 @@ +</method> +</interface>; +const FdoApplicationProxy = Gio.DBusProxy.makeProxyWrapper(FdoApplicationIface); Can we use a Gio.RemoteActionGroup instead? @@ +755,3 @@ +function getPlatformData() { + let startupId = GLib.Variant.new('s', '_TIME' + global.get_current_time()); + return { "desktop-startup-id": startupId }; A desktop-startup-id without proper startup notification is invalid. Use a Gdk.AppLaunchContext. @@ +764,3 @@ + _init: function(appId) { + this._appId = appId; + this._objectPath = objectPathFromAppId(appId); What is appId? The GApplication ID or the desktop file id? Or is the new API only for apps with reverse-dns desktop files? @@ +766,3 @@ + this._objectPath = objectPathFromAppId(appId); + + this._app = Shell.AppSystem.get_default().lookup_app(appId + '.desktop'); If appId is a GApplication ID, this will very likely fail. The "proper" way is GetConnectionUnixProcessID + WindowTracker.get_app_from_pid(), if the app has a mapped window. @@ +769,3 @@ + this._notifications = {}; + + this.parent(this._app.get_name()); Must check this._app != null (the appId could be invalid) @@ +781,3 @@ + + _createApp: function() { + return new FdoApplicationProxy(Gio.DBus.session, this._appId, this._objectPath); Async @@ +796,3 @@ + addNotification: function(notificationId, notificationParams) { + if (this._notifications[notificationId]) + this._notifications[notificationId].destroy(MessageTray.NotificationDestroyedReason.EXPIRED); No, if the notification already exists it should be transparently updated. Also, EXPIRED is the wrong reason for an application action.
Review of attachment 257302 [details] [review]: Ok
Review of attachment 257303 [details] [review]: No. The filesystem is not a database. It works for /run because it's a tmpfs, it's all in memory and it's fast enough, but it's a terrible idea for a physical disk. We should use some database, for example gvdb or gkeyfile, that is backed by one file, is stored sequentially and can be mapped in one disk read.
Review of attachment 257304 [details] [review]: I'm not sure we want all notifications to persist after reboots, in most cases they refer to transient state that doesn't apply anymore (and the application is not there to tell gnome-shell to close). I think it should be controlled by the app. ::: js/ui/notificationDaemon.js @@ +827,3 @@ + notifications.push([notificationId, notification.serialize()]); + })); + return [this._appId, notifications]; Shouldn't serialize return a GVariant, according to the name? @@ +902,3 @@ + let source = this._sources[appId]; + sources.push(source.serialize()); + })); Object.keys() forces the creation of an intermediate array. Use a for..in please, which is also more readable. @@ +903,3 @@ + sources.push(source.serialize()); + })); + global.set_persistent_state('notifications', GLib.Variant.new('a(sa(sv))', sources)); Why a(sa(sv)) instead of a(sa(sa{sv})) ?
(In reply to comment #21) > You can't send null across dbus, so how can body be null or undefined? If it's not in the a{sv}, it will be undefined. > Braces around an if statement with a multiline body. I thought it was multi-statement, but OK. > @@ +747,3 @@ > Can we use a Gio.RemoteActionGroup instead? Ryan wanted me to use FDO apps instead of GDBusActionGroup. Ask him why. > @@ +755,3 @@ > A desktop-startup-id without proper startup notification is invalid. > Use a Gdk.AppLaunchContext. This is an existing convention that we use apparently, where if the DESKTOP_STARTUP_ID contains "_TIME", it just contains the timestamp. See our copy of gactionmuxer.c and the support code in GDK. > @@ +764,3 @@ > Is the new API only for apps with reverse-dns desktop files? Yes. See the corresponding part in the desktop-entry-spec: """ The application must name its desktop file in accordance with the naming recommendations in the introduction section (e.g. the filename must be like org.example.FooViewer.desktop). The application must have a D-Bus service activatable at the well-known name that is equal to the desktop file name with the .desktop portion removed (for our example, org.example.FooViewer). The above interface must be implemented at an object path determined as follows: starting with the well-known D-Bus name of the application, change all dots to slashes and prefix a slash. For our example, this is /org/example/FooViewer. """ > @@ +766,3 @@ > Must check this._app != null (the appId could be invalid) Application error, in which case I do not care at all if we error out on the DBus call. > @@ +781,3 @@ > Async I thought creating proxies was already async? > @@ +796,3 @@ > No, if the notification already exists it should be transparently updated. No, it is replaced and re-notified. That's the semantics we established at the hackfest. > Also, EXPIRED is the wrong reason for an application action. I debated between EXPIRED and SOURCE_CLOSED. I don't really think it matters, since we don't expose the reason back to the app, but I'll change it.
(In reply to comment #23) > No. > The filesystem is not a database. It works for /run because it's a tmpfs, it's > all in memory and it's fast enough, but it's a terrible idea for a physical > disk. > > We should use some database, for example gvdb or gkeyfile, that is backed by > one file, is stored sequentially and can be mapped in one disk read. GKeyFile is not a database and can't be mmap'd, and Ryan doesn't want to make GVDB public. Yes, it would be much better if we could simply have an mmap-able database, but this was to just get things working. (This one is backed by one file, stored sequentially and can be mapped in one disk read. I exposed it as a directory because I thought we would be better off using different files for different purposes, if we eventually wanted to e.g. store the state of windows, we're better off storing that in a separate file as it would be touched separately) (In reply to comment #24) > I'm not sure we want all notifications to persist after reboots, in most cases > they refer to transient state that doesn't apply anymore (and the application > is not there to tell gnome-shell to close). I think it should be controlled by > the app. We talked about having a "not persistent" hint at the hackfest, but we want apps to persist by default. We couldn't think of any examples notifications that should close when the app quits, and now that we can activate actions on apps at startup, they can send notifications with whatever actions they want, and get feedback after the process is closed. > ::: js/ui/notificationDaemon.js > Shouldn't serialize return a GVariant, according to the name? Yeah, I chose this name when it originally returned a GVariant, but I can't append a pre-constructed GVariant to the array as far as I can tell, so I fixed it to return an array-as-tuple. Suggestions for the name welcome. > @@ +903,3 @@ > + sources.push(source.serialize()); > + })); > + global.set_persistent_state('notifications', > GLib.Variant.new('a(sa(sv))', sources)); > > Why a(sa(sv)) instead of a(sa(sa{sv})) ? The same reason actions come over the wire that way: DBus/GVariant dictionaries are unordered, and actions have an order.
(In reply to comment #20) > (In reply to comment #13) > > Review of attachment 257295 [details] [review] [details]: > > > > Mh... addButtonFull() is really a bad name. What about accepting either a > > button or a string in addButton()? > > I don't like the name addButtonFull either. Perhaps we should make addButton > take a button, and have addButtonFromLabel? We're changing the API to take a > callback anyway, so I don't care about breakage. Maybe addAction(), like in PopupMenu, for the simple version, and addButton() for the full one? (In reply to comment #25) > (In reply to comment #21) > > @@ +755,3 @@ > > A desktop-startup-id without proper startup notification is invalid. > > Use a Gdk.AppLaunchContext. > > This is an existing convention that we use apparently, where if the > DESKTOP_STARTUP_ID contains "_TIME", it just contains the timestamp. See our > copy of gactionmuxer.c and the support code in GDK. All DESKTOP_STARTUP_ID contain _TIME, the difference is what's before and after it. > > @@ +766,3 @@ > > Must check this._app != null (the appId could be invalid) > > Application error, in which case I do not care at all if we error out on the > DBus call. Then you should check it and throw at a predictable place (and possibly send a decent DBus error like InvalidArgs, not org.gnome.gjs.JSError.TypeError) > > @@ +781,3 @@ > > Async > > I thought creating proxies was already async? No, you must pass a callback if you want the async version. > > @@ +796,3 @@ > > No, if the notification already exists it should be transparently updated. > > No, it is replaced and re-notified. That's the semantics we established at the > hackfest. This is not what I see implemented by the Gtk patch for the freedesktop backend. (In reply to comment #26) > (In reply to comment #23) > > No. > > The filesystem is not a database. It works for /run because it's a tmpfs, it's > > all in memory and it's fast enough, but it's a terrible idea for a physical > > disk. > > > > We should use some database, for example gvdb or gkeyfile, that is backed by > > one file, is stored sequentially and can be mapped in one disk read. > > GKeyFile is not a database and can't be mmap'd, and Ryan doesn't want to make > GVDB public. Yes, it would be much better if we could simply have an mmap-able > database, but this was to just get things working. > > (This one is backed by one file, stored sequentially and can be mapped in one > disk read. I exposed it as a directory because I thought we would be better off > using different files for different purposes, if we eventually wanted to e.g. > store the state of windows, we're better off storing that in a separate file as > it would be touched separately) mmapping is not useful, if you read sequentially, so GKeyFile would not lose performance over gvdb. But it gains a lot over multiple seeks and inode loads. And no, using different files for different purposes is a bad idea, because, yes, you gain something on write, but you lose a lot on read. And by batching the writes, you can get the same performance for a single file, with better load times. Btw, g_file_set_contents() is atomic and fdatasync-ed, which is bad for something that is wrote often. I think we should use some less safe primitive (maybe a raw fwrite()?), as it's not terrible if we lose notifications after a crash... (Also, we already store the state of windows, see ~/.config/mutter/sessions) > (In reply to comment #24) > > I'm not sure we want all notifications to persist after reboots, in most cases > > they refer to transient state that doesn't apply anymore (and the application > > is not there to tell gnome-shell to close). I think it should be controlled by > > the app. > > We talked about having a "not persistent" hint at the hackfest, but we want > apps to persist by default. We couldn't think of any examples notifications > that should close when the app quits, and now that we can activate actions on > apps at startup, they can send notifications with whatever actions they want, > and get feedback after the process is closed. Closing when the app quits is one thing, closing when the system shuts down is another. It would be really buggy if after reboot you get notifications about chat contacts offline or critical battery (and it's a bad API if the app needs to load and tell gnome-shell to withdraw them every login, just in case) > > ::: js/ui/notificationDaemon.js > > Shouldn't serialize return a GVariant, according to the name? > > Yeah, I chose this name when it originally returned a GVariant, but I can't > append a pre-constructed GVariant to the array as far as I can tell, so I fixed > it to return an array-as-tuple. Suggestions for the name welcome. g_variant_new_tuple() takes an array of GVariants, and so does g_variant_new_array(). > > @@ +903,3 @@ > > + sources.push(source.serialize()); > > + })); > > + global.set_persistent_state('notifications', > > GLib.Variant.new('a(sa(sv))', sources)); > > > > Why a(sa(sv)) instead of a(sa(sa{sv})) ? > > The same reason actions come over the wire that way: DBus/GVariant dictionaries > are unordered, and actions have an order. And...? I proposed replacing the wrapping "v" with "a{sv}", which doesn't affect any order.
(In reply to comment #27) > (In reply to comment #25) > All DESKTOP_STARTUP_ID contain _TIME, the difference is what's before and after > it. So I'm simply copying what we do in our action muxer, and what we do in other places where we pass platform data. This convention is understood fine by GDK. > This is not what I see implemented by the Gtk patch for the freedesktop > backend. So I'll ask Jon for feedback on this, but I have a feeling that Lars's patch is wrong, probably. > Btw, g_file_set_contents() is atomic and fdatasync-ed, which is bad for > something that is wrote often. I think we should use some less safe primitive > (maybe a raw fwrite()?), as it's not terrible if we lose notifications after a > crash... It's wrong to lose any user data. (In reply to comment #24) > Closing when the app quits is one thing, closing when the system shuts down is > another. It would be really buggy if after reboot you get notifications about > chat contacts offline or critical battery (and it's a bad API if the app needs > to load and tell gnome-shell to withdraw them every login, just in case) This is going to need design review, then. I'm just implementing what we agreed upon in Montreal. > g_variant_new_tuple() takes an array of GVariants, and so does > g_variant_new_array(). Right, OK. The way the auto-packing/unpacking works in the gjs bindings is unfortunately inconsistent. > And...? I proposed replacing the wrapping "v" with "a{sv}", which doesn't > affect any order. Ah, I misread. I'll fix this.
(In reply to comment #28) > So I'll ask Jon for feedback on this, but I have a feeling that Lars's patch is > wrong, probably. Wait, no, even if it is found to replace an existing notification, we call update();, but then call source.notify(notification); in processNotification (unless the app is the focused window, but that's shell policy, not an API guarantee, and it's something we said we'd look at doing later at the hackfest)
(In reply to comment #29) > (In reply to comment #28) > > So I'll ask Jon for feedback on this, but I have a feeling that Lars's patch is > > wrong, probably. > > Wait, no, even if it is found to replace an existing notification, we call > update();, but then call source.notify(notification); in processNotification > (unless the app is the focused window, but that's shell policy, not an API > guarantee, and it's something we said we'd look at doing later at the hackfest) Mh... so?
(In reply to comment #30) > Mh... so? So, as far as I can see, the FDO notification backend in GTK+ won't silently update either.
(In reply to comment #31) > (In reply to comment #30) > > Mh... so? > > So, as far as I can see, the FDO notification backend in GTK+ won't silently > update either. It will silently update if the notification is already open (summary or banner mode), as required by the notification spec.
(In reply to comment #27) > Why a(sa(sv)) instead of a(sa(sa{sv})) ? So, I finally remembered the actual reason. I can't use 'a(sa(s@a{sv})' in the gjs bindings, so I can't supply the pre-constructed a{sv} variant. So I would have to unpack it again, and gjs would re-pack it back into a variant. Yay. I suppose I could add support for '@' to gjs, which would probably be super helpful regardless. (aside: I'd really love it if the GDBus bindings didn't always call deep_unpack(), and if variants were handled better in gjs. It's quite painful to think about the amount of copying we do all over the place)
Attachment 257294 [details] pushed as 88d0731 - messageTray: Replace setButtonSensitive by simply returning the button Attachment 257295 [details] pushed as 5023542 - messageTray: Split out addButton to allow consumers to pass a pre-made button Attachment 257296 [details] pushed as 5f081b8 - messageTray: Remove useActionIcons feature Attachment 257299 [details] pushed as 27a86a4 - notificationDaemon: Move nextNotificationId inside the daemon class Attachment 257300 [details] pushed as 394743e - notificationDaemon: Rename the existing implementation to the FdoNotificationSource Attachment 257301 [details] pushed as e0b87f1 - notificationDaemon: Implement the new GTK+ notifications API Attachment 257302 [details] pushed as c9b73ac - shell-global: Handle empty variants better Attachment 257303 [details] pushed as 1ac4ab7 - shell-global: Add new APIs for saving/loading persistent state Attachment 257304 [details] pushed as 9d8fb19 - notificationDaemon: Write notifications out to disk Pushing this now with most of the suggested fixes, and stuff worked on out IRC. Will open new bugs if stuff breaks.