GNOME Bugzilla – Bug 672018
Need API to set global application state (busy, counters, ...)
Last modified: 2018-05-24 13:50:06 UTC
It seems to be desirable to have API allowing clients to export an application state to the shell. Ryan agreed on IRC this is something belonging to GApplication/GtkApplication. For example - busy: specifies the application is performing a long operation. The shell can show e.g. a spinner next to the application name - counters: specifies the application has X new items. This can be used by the shell to compose an emblem with the counter over the application icon - progress: specifies the application has an ongoing transfer. The shell could compose a progressbar over the application icon to display task completion. This would probably need a bit more of thinking to cope with multiple operations going on at the same, or in different windows of the same application.
Created attachment 240526 [details] [review] Add version macros for 2.38
Created attachment 240527 [details] [review] application: introduce a "busy" property Together with a setter and a getter for it. This feature is intended for clients that want to signal a desktop shell their busy state, for instance because a long-running operation is pending. The property is mirrored read-only on the org.gtk.Application interface for clients to use.
Created attachment 240528 [details] [review] bloatpad: add a test for GApplication's busy state
Review of attachment 240526 [details] [review]: sure
Review of attachment 240527 [details] [review]: Other than that, looks great to me ::: gio/gapplication.c @@ +1870,3 @@ + * The busy state will be propagated over DBus, so a session shell might + * use that information to indicate the state to the user (e.g. with a + * spinner). 'Might' is a bit weak, imo. If we don't assure that this has any effect, apps will probably set a busy cursor in addition to this, or some other undesirable extra things. ::: gio/gapplicationimpl-dbus.c @@ +122,3 @@ + return g_variant_new_boolean (g_application_get_busy (impl->app)); + + g_assert_not_reached (); Isn't that a bit unfriendly ? Essentially, you're inviting anybody to crash the app by getting a nonexisting property @@ +324,3 @@ return FALSE; + g_signal_connect (impl->app, "notify", G_CALLBACK (send_property_change), impl); Given that we're only exporting busy, how about connecting to "notify::busy" ?
Review of attachment 240528 [details] [review]: sure
Created attachment 240612 [details] [review] application: introduce a "busy" property -- Updated for review comments.
Created attachment 240622 [details] [review] application: introduce methods to mark the application as busy -- A different version of the API, based on Ryan's suggestion. This works now similarily to g_application_hold() and g_application_release(), so clients can stack up busy indications for multiple operations, without worrying about bookkeeping the global state.
Review of attachment 240622 [details] [review]: Just needs a few small tweaks, but looks mostly good. ::: gio/gapplication.c @@ +1860,3 @@ + * Use this function to indicate that the application is busy, for instance + * while a long running operation is pending. + * The busy state will be propagated over DBus, so a session shell will if this is supposed to be a paragraph break, you need a blank line. maybe avoid mentioning dbus here... just say that the shell may do something. @@ +1899,3 @@ +g_application_unmark_busy (GApplication *application) +{ + g_return_if_fail (G_IS_APPLICATION (application)); can you add a g_return_if_fail() check here to prevent dropping the busy count below 0? maybe it also makes sense to add a check to _mark_busy() to avoid it going above a million or so... just in case someone is leaking busyness... ::: gio/gapplicationimpl-dbus.c @@ +135,3 @@ + + g_variant_builder_init (&builder, G_VARIANT_TYPE_ARRAY); + g_variant_builder_init (&invalidated_builder, G_VARIANT_TYPE ("as")); don't need to bother with the invalidated_builder. quoth the docs: As a special exception, if the given type string is a definite type, then NULL may be given to mean an empty array of that type. @@ +141,3 @@ + "Busy", g_variant_new_boolean (impl->busy)); + + g_dbus_connection_emit_signal (impl->session_bus, ugh... not much can be done about this, I guess...
Created attachment 240631 [details] [review] application: introduce methods to mark the application as busy -- Updated for review
(In reply to comment #9) > maybe avoid mentioning dbus here... just say that the shell may do something. Okay, I changed it to "exposed to other processes". > @@ +1899,3 @@ > +g_application_unmark_busy (GApplication *application) > +{ > + g_return_if_fail (G_IS_APPLICATION (application)); > > can you add a g_return_if_fail() check here to prevent dropping the busy count > below 0? Fixed. > maybe it also makes sense to add a check to _mark_busy() to avoid it going > above a million or so... just in case someone is leaking busyness... I don't think this makes a lot of sense, and if you're leaking busyness it will be a (visible) problem way before it reaches one million :) I left it as is for now. > ::: gio/gapplicationimpl-dbus.c > @@ +135,3 @@ > + > + g_variant_builder_init (&builder, G_VARIANT_TYPE_ARRAY); > + g_variant_builder_init (&invalidated_builder, G_VARIANT_TYPE ("as")); > > don't need to bother with the invalidated_builder. quoth the docs: Fixed this too.
Review of attachment 240631 [details] [review]: Good to commit with one small change. ::: gio/gapplicationimpl-dbus.c @@ +123,3 @@ + return g_variant_new_boolean (impl->busy); + + g_set_error (error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS, missed this the first time: GDBus does this check already (against the introspection xml), so an assert_not_reached() is a better fit here.
Attachment 240631 [details] pushed as db325cd - application: introduce methods to mark the application as busy Pushed to master with that fixed, thanks for the review. I'll keep this open for the other states proposed in comment #0.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/523.