GNOME Bugzilla – Bug 688492
Add a notification API
Last modified: 2013-10-21 18:32:09 UTC
We want to have a better story for app notifications in GTK, that doesn't require libnotify. See https://mail.gnome.org/archives/gtk-devel-list/2012-November/msg00009.html for a first draft proposal; this is a bug to track its implementation.
Created attachment 242949 [details] [review] Add GNotification
Created attachment 242950 [details] [review] A simple example
Review of attachment 242949 [details] [review]: I may have missed this at the hackfest: what happened to the idea of using GActionGroup and/or GActionMap for the notification's actions ? ::: gio/gfdonotificationbackend.c @@ +132,3 @@ + g_variant_get (parameters, "(uu)", &id, NULL); + else if (g_str_equal (signal_name, "ActionInvoked")) + g_variant_get (parameters, "(us)", &id, &action); Could have an else g_assert_not_reached(); here, just for sanity. Otherwise we _might_ end up handling a signal without knowing its name... @@ +157,3 @@ + + if (g_str_has_prefix (action, "app.")) + g_action_group_activate_action (G_ACTION_GROUP (n->application), action + 4, target); Should there be an else here ? Can you have actions on a notification that are not app. prefixed ? @@ +218,3 @@ + "org.freedesktop.Notifications", "CloseNotification", + g_variant_new ("(u)", id), G_VARIANT_TYPE_TUPLE, + G_DBUS_CALL_FLAGS_NONE, -1, NULL, NULL, NULL); No need to specify a return type if you are not connecting a callback, I think ? @@ +289,3 @@ + g_dbus_connection_signal_unsubscribe (backend->session_bus, backend->notify_subscription); + backend->notify_subscription = 0; + g_clear_object (&backend->session_bus); Is there a chance that you could leak the session_bus ref here ? I'd rather move the clear_object call out of the if to remove any doubt... @@ +340,3 @@ + case SERVER_NONE: + g_bus_get (G_BUS_TYPE_SESSION, NULL, g_fdo_notification_backend_got_bus, backend); + self->state = SERVER_PENDING; You set the state to PENDING here, but the switch does not handle this state. So, if another send call comes in before we get to got_bus, we hit the assert_not_reached in this switch statement, I'm afraid. The case may not actually ever occur, because the app has to be registered before sending a notification, so g_bus_get will always return synchronously. But still, the code is at least confusing without a case SERVER_PENDING: /* wait for the bus */ break; ::: gio/gnotification.c @@ +297,3 @@ + g_return_if_fail (title != NULL); + + notification->title = g_strdup (title); These are call-only-once setters ? Should we not free the previous title/body/image/etc ? @@ +329,3 @@ + +GIcon * +g_notification_get_image (GNotification *notification) Probably need a (transfer-none) annotation here ::: gio/gnotification.h @@ +56,3 @@ + const gchar *action, + const gchar *target_format, + ...); Can we call this g_notification_add_action ? I'd rather avoid talking about buttons in GIO
Thanks for the review. (In reply to comment #3) > Review of attachment 242949 [details] [review]: > > I may have missed this at the hackfest: what happened to the idea of using > GActionGroup and/or GActionMap for the notification's actions ? I removed that after we decided that notifications can be activated even when an application is not running. Otherwise an app would need to recreate all notifications on startup. > [...] Fixed these issues. > ::: gio/gnotification.h > @@ +56,3 @@ > + const gchar *action, > + const gchar > *target_format, > + ...); > > Can we call this g_notification_add_action ? > I'd rather avoid talking about buttons in GIO Good point. I think we're already overloading 'action' too much, though. Maybe g_notification_add_response?
Created attachment 243004 [details] [review] Add GNotification
Review of attachment 242949 [details] [review]: ::: gio/gfdonotificationbackend.c @@ +201,3 @@ + g_notification_get_body (notification), + &action_builder, + NULL, /* hints */ Where do the notification's image and urgent hint get used? Do we want support for resident? @@ +258,3 @@ + + backend->session_bus = g_bus_get_finish (result, &error); + if (backend->session_bus == NULL) If this happens, we should fail much earlier in the GtkApplication. What's wrong with g_bus_get_sync in this case, given that GDBus will cache the session bus at startup. Do we even need the state management to manage a session bus, if we're pretty much guaranteed for one existing?
(In reply to comment #6) > Where do the notification's image and urgent hint get used? Do we want support > for resident? Added image and urgency in the latest iteration. It only supports file icons for now. Adding support for arbitrary icons for the fdo backend is not possible without depending on gdk-pixbuf for loadable, themed, and emblemed icons. To be honest, I'm not a big fan of resident notifications. The only use-case I know of are music players which might want to keep information about the currently playing track in the message tray. I much prefer the approach that we're taking with chat notifications, i.e. treating it as a totally separate thing. Can we do the same for music players? Are there other use-cases for resident notifications? > @@ +258,3 @@ > + > + backend->session_bus = g_bus_get_finish (result, &error); > + if (backend->session_bus == NULL) > > If this happens, we should fail much earlier in the GtkApplication. What's > wrong with g_bus_get_sync in this case, given that GDBus will cache the > session bus at startup. > > Do we even need the state management to manage a session bus, if we're pretty > much guaranteed for one existing? I added this originally because GApplication also works without D-Bus. Calling g_bus_get_sync() and waiting for it to time out every time a notification is sent seemed unacceptable. You're probably right though, this never happens in practice. Luckily, there's g_application_get_dbus_connection(), which I'm using now. This allowed me to get rid of all of the state management. Thanks for pointing it out.
Created attachment 248112 [details] [review] Add GNotification
(In reply to comment #7) > Added image and urgency in the latest iteration. I just want to point this out: we should probably establish the semantics of urgency in the new notification specification. I think it makes sense to have the urgency flag only work in between notifications in the same GApplication, and to have user determine urgency between different apps? > Are there other use-cases for resident notifications? We use them for file transfer status notifications in nautilus. Perhaps we want this in the Transfers app, or an in-shell thing so we can implement a progress bar widget. > I added this originally because GApplication also works without D-Bus. It does? Huh.
Review of attachment 248112 [details] [review]: Would be great to figure out a way to allow GtkApplication to support non-file icons. ::: gio/gapplication.c @@ +1950,3 @@ + "of an application"); + return; + } I would be ok with just using g_return_if_fail for these checks. The text can go in the docs for this function. ::: gio/gfdonotificationbackend.c @@ +208,3 @@ + "", /* app name */ + replace_id, + "", /* app icon */ What are we going to do about the application data here ? I guess we really want this to be inferred from the desktop-entry hint, or even better, from the app-id == bus name == desktop filename @@ +336,3 @@ + call_cancel (self->session_bus, n->notify_id); + + g_hash_table_remove (n->backend->notifications, id); could just say self->notifications instead of n->backend->notifications here ::: gio/gnotification.c @@ +236,3 @@ +/** + * g_notification_new: + * @id: (allow-none): a string identifying the notification Are you sure about allow-none here ? Should say something about the uniqueness requirements for the id here. On second thought, the id seems purely internal anyway, and could just as well be generated ? You never emit a signal with the id in it, or anything of that sort. @@ +289,3 @@ + * + * Sets the title of @notification to @title. + */ Should be documented as 'plain text only', I think @@ +324,3 @@ + * @body: the new body for @notification + * + * Sets the body of @notification to @body. Should add some documentation about supported markup here.
Lets wrap this up at guadec. It looks pretty close to mergable.
Review of attachment 248112 [details] [review]: .
Created attachment 257262 [details] [review] Add GNotification
Why do add_button() and set_default_action() use varargs that are introspection-unfriendly, and not an optional GVariant?
Created attachment 257365 [details] [review] Add GNotification
Created attachment 257366 [details] [review] Add GNotification
Created attachment 257367 [details] [review] Add gtk notification backend This is a notification backend that uses the new org.gtk.Notifications interface that we have been talking about on the summit. Note that I didn't have time yet to test this, but I wanted to put this out here anyway for people to look at.
Florian, I've fixed this in the latest patch. Both functions now have version that take a detailed action string, a GVariant format string with positional arguments, and a GVariant directly.
Created attachment 257368 [details] A simple example Updated the example to use the new API.
Review of attachment 257366 [details] [review]: Missing Since tags in per-function doc comments, and a long description for GNotification. ::: gio/gapplication.c @@ +1970,3 @@ + /* silently ignore notifications if there's no backend for a platform */ + if (application->priv->notifications == NULL) + return; Might be good to warn once about this ? ::: gio/gapplication.h @@ -197,3 @@ void g_application_set_default (GApplication *application); -GLIB_AVAILABLE_IN_2_38 2.40 by now, right ? ::: gio/gnotification.c @@ +187,3 @@ + properties[PROP_IMAGE] = g_param_spec_object ("image", + "Image", + "An image to be displayed wit hthe notification", Typo: "wit hthe" @@ +193,3 @@ + properties[PROP_URGENT] = g_param_spec_boolean ("urgent", + "Urgent", + "Whether the notification is urgent", It would be good to have a doc comment here with some hints about the expected usage of urgency. @@ +214,3 @@ + * + * Use g_application_send_notification() to send the notification to the + * desktop shell. Would be good to mention that you need to populate the notification first. @@ +228,3 @@ + * @notification: a #GNotification + * + * Gets the current title of @notification. The talk about 'current' here seems a little misleading, maybe ? If I call set_title() on an already-sent notification, thats not going to do any good, right ? Would be good to mention that, either here, or in the (still missing) long description. @@ +466,3 @@ + if (!g_str_has_prefix (action, "app.")) + { + g_warning ("g_notification_add_button: action '%s' does not start with 'app.'." Wrong function name - better to use G_STRFUNC to avoid this problem. @@ +694,3 @@ + if (!g_str_has_prefix (action, "app.")) + { + g_warning ("g_notification_set_default_action: action '%s' does not start with 'app.'." Wrong function name ::: gio/gnotificationbackend.c @@ +56,3 @@ + + /* Avoid ref cycle by not taking a ref to the application at all. The + * backend only lives as long as the application does. */ Nitpick; I prefer asterisks to line up all the way: /* bla * bla */
Review of attachment 257367 [details] [review]: I think we should ship the xml for the dbus api, imo - and we should also document it somewhere publically, like https://wiki.gnome.org/GApplication/DBusAPI ::: gio/ggtknotificationbackend.c @@ +64,3 @@ + + /* Find out if the notification server is running. This is a + * synchronous call because gio extension points don't support asnyc Typo: 'asnyc'
(In reply to comment #21) > I think we should ship the xml for the dbus api, imo - and we should also > document it somewhere publically, like > https://wiki.gnome.org/GApplication/DBusAPI For notifications? It's intentionally private, for now, as it will very likely change before the 3.12 release.
yeah, the apis that are documented on that page are not guaranteed to be stable either. private != undocumented
Review of attachment 257366 [details] [review]: ::: gio/Makefile.am @@ +154,3 @@ gdbusmenumodel.h \ + gnotification.h \ + gnotificationbackend.h \ is gnotificationbackend.h supposed to be public API? if so, you need to add GLIB_AVAILABLE_IN_2_40 to all functions so they're visible, else, put them with the gnotification.c / gnotificationbackend.c below as it's private @@ +179,3 @@ gdbusmenumodel.c \ + gnotification.c \ + gnotificationbackend.c \ missing gnotification-private.h ::: gio/gapplication.h @@ +197,3 @@ void g_application_set_default (GApplication *application); +GLIB_AVAILABLE_IN_2_40 this should not be changed ::: gio/gnotification-private.h @@ +1,1 @@ + missing gpl header ::: gio/gnotification.c @@ +204,3 @@ +{ + notification->title = g_strdup (""); + notification->body = g_strdup (""); should we make these NULL by default? strdup'ing an empty string seems wrong to me. @@ +302,3 @@ + * Gets the image currently set on @notification. + * + * Returns: (transfer-none): the image associated with @notification (transfer none), not (transfer-none) @@ +450,3 @@ + * + * If @target is non-%NULL, @action will be activated with @target as + * its parameter. should this have (rename-to g_notification_add_button_with_target) if it's intended for introspection? @@ +682,3 @@ + * + * When no default action is set, the application that the notification + * was sent on is activated. should this have (rename-to g_notification_set_default_action_and_target) if it's intended for introspection? ::: gio/gnotification.h @@ +1,1 @@ +#ifndef __G_NOTIFICATION_H__ missing gpl header ::: gio/gnotificationbackend.c @@ +44,3 @@ + GNotificationBackend *backend; + + g_return_if_fail (G_IS_APPLICATION (application)); needs to be g_return_val_if_fail to prevent warning @@ +88,3 @@ +g_notification_backend_get_application (GNotificationBackend *backend) +{ + g_return_if_fail (G_IS_NOTIFICATION_BACKEND (backend)); needs to be g_return_val_if_fail to prevent warning @@ +96,3 @@ +g_notification_backend_get_dbus_connection (GNotificationBackend *backend) +{ + g_return_if_fail (G_IS_NOTIFICATION_BACKEND (backend)); needs to be g_return_val_if_fail to prevent warning
Review of attachment 257367 [details] [review]: ::: gio/gnotification.c @@ +719,3 @@ + */ +GVariant * +g_notification_serialize (GNotification *notification) would prefer if this was in ggtknotificationbackend.c using private api as it's unlikely to be used outside of that backend.
Review of attachment 257367 [details] [review]: so, this doesn't actually work after testing. i don't blame you, considering you could never actually test it, but i had to hack it up to get it working locally the big thing is that you never actually register this backend, so we always just use fdo right now. you need to insert a line in giomodule.c like you did to the fdo backend to actually make it work ::: gio/ggtknotificationbackend.c @@ +92,3 @@ + application = g_notification_backend_get_application (backend); + + params = g_variant_new ("(ssa{sv})", g_application_get_application_id (application), needs to be "(ss@a{sv})" ofc. (this was a bit hard to debug as it produced a wacky error about invalid builders) ::: gio/gnotification.c @@ +710,3 @@ +fake_maybe_new (GVariant *v) +{ + return g_variant_new_array (G_VARIANT_TYPE ("v"), &v, v ? 1 : 0); this is wrong, because the variant you pass in is not of type 'v', but of any type. i'm not sure if there's an easy prefix char you can use to wrap it, but i just did: static GVariant * fake_maybe_new (GVariant *v_) { GVariant *v = v_ ? g_variant_new_variant (v_) : NULL; return g_variant_new_array (G_VARIANT_TYPE ("v"), &v, v ? 1 : 0); }
Created attachment 257545 [details] [review] Add GNotification Thanks for the reviews. This version fixes the raised issues except that there's still no overview documentation.
Created attachment 257546 [details] [review] Add gtk notification backend
Review of attachment 257545 [details] [review]: This seems good. I think you mostly need to delete things at this point :) ::: gio/giotypes.h @@ +64,2 @@ typedef struct _GMenuModel GMenuModel; +typedef struct _GNotificationBackend GNotificationBackend; not a public type, please. ::: gio/gnotification.c @@ +53,3 @@ +enum +{ + PROP_0, i don't really see the benefit of having these as properties ::: gio/gnotification.h @@ +42,3 @@ + +GLIB_AVAILABLE_IN_2_40 +const gchar * g_notification_get_id (GNotification *notification); i thought that we would not have ID appearing here... @@ +45,3 @@ + +GLIB_AVAILABLE_IN_2_40 +const gchar * g_notification_get_title (GNotification *notification); why do we need getters on an object that is, essentially, a builder?
Review of attachment 257545 [details] [review]: ::: gio/gapplication.c @@ +692,3 @@ g_object_unref (application->priv->actions); + g_clear_object (&application->priv->notifications); why clear in a finalizer? just unref, like above. @@ +1938,3 @@ + * There is no guarantee that the notification is displayed immediately, + * or even at all. + * Notifications persist after the application exits. @@ +1940,3 @@ + * + * @notification is only used to communicate the notification's + * properties to the shell. Modifying it after this call has no effect The way that this is stated is unclear. Just say that modifying it after the call has no effect but that it may be reused for future calls. @@ +1942,3 @@ + * properties to the shell. Modifying it after this call has no effect + * until calling this function again with the same id. + * Maybe include some language about what id can be. People are likely to get confused here. Something along the lines of "@id may be any string, and need no have any special format." will go a long way to clear up any doubts that the user may have. Also would be nice: "It may be appropriate to use a string like 'new-message' for a notification about new messages that may be replaced by an updated notification if more messages arrive." @@ +1943,3 @@ + * until calling this function again with the same id. + * + * If a notification with @id already exists, it will be replaced with If a previous notification was sent with the same @id, ... This works even for notifications sent from a previous execution of the application, as long as @id is the same string. @@ +1989,3 @@ + * + * This call does nothing if a notification with @id doesn't exist or + * the notification was never sent. "This function works even for notifications sent in previous executions of this application, as long @id is the same as it was for the sent notification." ::: gio/gfdonotificationbackend.c @@ +100,3 @@ + + g_hash_table_iter_init (&it, backend->notifications); + while (g_hash_table_iter_next (&it, NULL, (gpointer *) &n)) :/ @@ +124,3 @@ + FreedesktopNotification *n; + + n = g_fdo_notification_backend_find_notification (backend, id); id is always 0 here... i think you want the _get()s below to come before you do this. @@ +137,3 @@ + if (action) + { + if (g_str_equal (action, "default")) Magic string alert. What if someone included an action called "default" as one of their buttons? (yes: "default", not "app.default"). It's weird that this would work this way... @@ +140,3 @@ + { + g_clear_pointer (&action, g_free); + g_notification_get_default_action (n->notification, &action, &target); You're consulting the GNotification object that the user passed you. You said "changes made to this object after sending the notification are ignored" but then you consult it in a way that will allow the user to modify the default action after it is sent. @@ +146,3 @@ + gint index; + + index = g_notification_get_button_with_action (n->notification, action); This seems problematic. What if the same action is used twice in a given notification with different target? Maybe we should generate unique action names for sending over the bus, instead. That would dodge the "default" issue above as well. @@ +152,3 @@ + } + + if (action && g_str_has_prefix (action, "app.")) why not tuck this (and the frees below) into the if (action) {} block above? could move the "target;" variable declaration there as well. @@ +192,3 @@ + + g_notification_get_button (notification, i, &label, &action, NULL); + g_variant_builder_add_value (&action_builder, g_variant_new_take_string (action)); see above for comments about not reusing action names... we could also dodge all of the issues by sending the detailed action as the action name in all cases: this would prevent us from having to keep the GNotification object around to consult it for the action and target later, also avoid the "default" magic string and avoid problems with two actions with the same name but different target... @@ +200,3 @@ + g_variant_new_string (g_application_get_application_id (app))); + if (g_notification_get_urgent (notification)) + g_variant_builder_add (&hints_builder, "{sv}", "urgency", g_variant_new_byte (2)); please, a comment here explaining what this "2" is about? ::: gio/gnotification.h @@ +62,3 @@ + +GLIB_AVAILABLE_IN_2_40 +void g_notification_set_image (GNotification *notification, i'm passing a GIcon, so why not _set_icon()?
Review of attachment 257546 [details] [review]: I think we want some protocol changes here. Better talk to Jasper... ::: gio/ggtknotificationbackend.c @@ +1,1 @@ +/* This file is gloriously beautiful in its simplicity. @@ +68,3 @@ + * dbus daemon. */ + + session_bus = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); you leak the bus ::: gio/gnotification.c @@ +786,3 @@ + * g_notification_serialize: + * + * Serializes @notification into an floating variant of type a{sv}. Returns: floating... @@ +807,3 @@ + if ((serialized_image = g_icon_serialize (notification->image))) + { + g_variant_builder_add (&builder, "{sv}", "image", serialized_image); "icon".... @@ +819,3 @@ + + tuple = g_variant_new ("(s@av)", notification->default_action, + fake_maybe_new (notification->default_action_target)); My understanding is that a missing default action means that we should activate the app. Do we have any way of getting the "activate the app" functionality for a button and having the default action be something different? could split this as default-action/default-target to avoid the fake maybe. see below. @@ +839,3 @@ + } + + g_variant_builder_add (&builder, "{sv}", "actions", g_variant_builder_end (&actions_builder)); "buttons", please. these are not actions. also: consider using an a{sv} here. it's a bit nicer than the fake-maybe. { 'label': 'Open', 'action': 'app.start' } vs. { 'label': 'Open Message', 'action': 'app.show-message', 'target': 'messageid1235' } Note: 'messageid1235' is a string alone -- no variant wrapper (matching the convention we use in GMenuModel for targets and icons). and the possibility of doing future things as well -- maybe some day the designers want a way to turn one of the buttons blue?
Review of attachment 257545 [details] [review]: ::: gio/gfdonotificationbackend.c @@ +56,3 @@ + GFdoNotificationBackend *backend; + gchar *id; + GNotification *notification; We chatted about this a bit more. I didn't understand that "default" is already a magic string as per the old notifications spec, which is why some of my comments about magic strings don't make sense. The correct way here is to store the default action on the FreedesktopNotification object as a printed detailed action name instead of the GNotification. The other ones we can do as detailed action names used as the name of the action (as sent to the notifications system). This leaves only what to do in the case that the user provides the action "default" themselves -- in that case, I think we can safely rewrite it to be just about any bareword that doesn't start with "app." just to avoid the conflict -- it will do nothing, which is exactly what would happen in the case that the user provides this (bogus) string anyway.
(In reply to comment #32) > Review of attachment 257545 [details] [review]: > > ::: gio/gfdonotificationbackend.c > @@ +56,3 @@ > + GFdoNotificationBackend *backend; > + gchar *id; > + GNotification *notification; > > We chatted about this a bit more. I didn't understand that "default" is > already a magic string as per the old notifications spec, which is why some of > my comments about magic strings don't make sense. The correct way here is to > store the default action on the FreedesktopNotification object as a printed > detailed action name instead of the GNotification. The other ones we can do as > detailed action names used as the name of the action (as sent to the > notifications system). > > This leaves only what to do in the case that the user provides the action > "default" themselves -- in that case, I think we can safely rewrite it to be > just about any bareword that doesn't start with "app." just to avoid the > conflict -- it will do nothing, which is exactly what would happen in the case > that the user provides this (bogus) string anyway. We could just reserve the name 'default' ?
Bugzilla was down this morning so we bounced the branch back and forth between Lars and myself this morning and got it finished up. Committed with no major changes (just more trivial fixes).