GNOME Bugzilla – Bug 736492
Application busy status not shown correctly
Last modified: 2014-11-27 11:07:19 UTC
Recently I changed sound-juicer to use g_application_mark_busy() while it retrieves information from the web. If the call is made as the applications starts then most of the time the shell does not show the busy spinner. Looking in src/shell-app.c ensure_busy_watch() calls g_dbus_connection_signal_subscribe() to be notified when the 'Busy' property of 'org.gtk.Application' changes but does not read the current value of 'Busy'. So if the application calls g_application_mark_busy() before the shell calls g_dbus_connection_signal_subscribe() it misses the busy status change.
Created attachment 286411 [details] [review] Fix race in getting application's busy state It's possible for an application to call g_application_mark_busy() before the shell subscribes to change notifications on the application's busy state. Get the busy state when the shell connects to change notifications to fix this.
Created attachment 286412 [details] [review] Handle application's busy state more carefully When the busy state was added, not all the places where the state is checked or set were changed to take account of the fact that a running application can have state == SHELL_APP_STATE_RUNNING or state == SHELL_APP_STATE_BUSY.
Review of attachment 286411 [details] [review]: GDBus will already get all properties, no? Can't you just use get_cached_property?
(In reply to comment #3) > Review of attachment 286411 [details] [review]: > > GDBus will already get all properties, no? Can't you just use > get_cached_property? Thanks for such a quick review. The existing code doesn't use GDBusProxy, it just calls g_bus_get_sync() and g_dbus_connection_signal_subscribe() so there's no proxy object to call get_cached_property() on. I've not used GDBus before and knew next to nothing about DBus before I started writing this patch so any advise you have would be gratefully received. Maybe I should be creating a proxy but it didn't seem worth it just to get the value of a single property once.
Created attachment 286610 [details] [review] Generate GDBus proxy object for org.gtk.Application I've reworked these patches to use a GDBus proxy generated by gdbus-codegen Having a proxy object will make it easier to monitor the application's busy state. This adds a dependency on gdbus-codegen
Created attachment 286611 [details] [review] Use org.gtk.Application proxy to monitor app's busy state This simplifies the code and fixes a race where an application could call g_application_mark_busy() before the shell subscribed to change notifications on the application's busy state.
Created attachment 286612 [details] [review] Handle application's busy state more carefully When the busy state was added, not all the places where the state is checked or set were changed to take account of the fact that a running application can have state == SHELL_APP_STATE_RUNNING or state == SHELL_APP_STATE_BUSY.
Review of attachment 286612 [details] [review]: I think we should go back to SHELL_APP_STATE_RUNNING and add a separate is-busy property here. I think the busy state was hastily and poorly added. To pretty much all other other users of the states, it's the same as RUNNING.
Review of attachment 286612 [details] [review]: In fact, if you do this monitoring directly from JS (I don't think anything in C actually needs to figure out the busy state), then that lets you remove the first two patches as well. But if C needs to detect it, then that's fine, both of them look OK.
(In reply to comment #8) > Review of attachment 286612 [details] [review]: > > I think we should go back to SHELL_APP_STATE_RUNNING and add a separate is-busy > property here. I think the busy state was hastily and poorly added. To pretty > much all other other users of the states, it's the same as RUNNING. Thanks for looking at the patches. I agree a separate property would be better - I almost added it when I wrote the patch but was worried that changing the way the state property worked would break something. (In reply to comment #9) > Review of attachment 286612 [details] [review]: > > In fact, if you do this monitoring directly from JS (I don't think anything in > C actually needs to figure out the busy state), then that lets you remove the > first two patches as well. But if C needs to detect it, then that's fine, both > of them look OK. From what I remember it's only the JS stuff that uses the busy state but I'll check. The application's DBUS ID isn't currently exported by ShellApp. If I move the monitoring to JS is there a way to easily get the ID without adding a 'dbus-id' property to ShellApp?
Created attachment 290702 [details] [review] Use org.gtk.Application proxy to monitor app's busy state The only change since the last version of this patch is to remove an unnecessary null pointer check that was guarding g_clear_object (&state->application_proxy); This simplifies the code and fixes a race where an application could call g_application_mark_busy() before the shell subscribed to change notifications on the application's busy state.
Created attachment 290703 [details] [review] Add busy property to ShellApp As I couldn't see an easy way to get the applications DBUS ID in panel.js without adding a property to ShellApp I just added a busy property instead as it was less work, I hope that's ok. Using a separate property to show when the application is busy rather than cramming it into the state property makes the code clearer. In most places we only care if an app is running or not, not whether it is actually busy.
Review of attachment 290702 [details] [review]: OK.
Review of attachment 290703 [details] [review]: Yeah, let's go with this for now. I'll rewrite it in JS later if wanted.
Thanks for the reviewing these patches. Attachment 286610 [details] pushed as 943f6c9 - Generate GDBus proxy object for org.gtk.Application Attachment 290702 [details] pushed as e00bfcc - Use org.gtk.Application proxy to monitor app's busy state Attachment 290703 [details] pushed as 546ae00 - Add busy property to ShellApp