GNOME Bugzilla – Bug 765006
privacy: React to changes in permissions store
Last modified: 2016-07-25 11:29:08 UTC
See attached patch.
Created attachment 325876 [details] [review] privacy: React to changes in permissions store Currently if control-center is already running with privacy panel in foreground and user authorizes a new application to gain access to location information from gnome-shell dialog, this change doesn't get reflected in the privacy panel to user until they exit privay panel. This change fixes this by reacting to changes to permissions store.
Review of attachment 325876 [details] [review]: > privay privacy ::: panels/privacy/cc-privacy-panel.c @@ +530,3 @@ data->pending_state = state; + g_variant_iter_init (&iter, self->priv->location_apps_perms); Those changes don't look related to me, can you split them up? @@ +589,3 @@ + if (w != NULL) + { + gtk_switch_set_active (GTK_SWITCH (w), enabled); You can remove the linefeed. @@ +743,3 @@ + error->message); + g_error_free (error); + Remove the linefeed. @@ +821,3 @@ + g_str_equal, + g_free, + g_object_unref); You shouldn't keep a reference to the widget here, it's likely to cause more problem than it's worth (like the panel and all its children being destroyed, but you still holding a ref to the widget. Can applications be removed from the list (like if the app is removed while the panel is opened)? In which case you'd know which app is being removed, and you'd remove it from the hash table. If not, maybe that's something to look out for.
Review of attachment 325876 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +530,3 @@ data->pending_state = state; + g_variant_iter_init (&iter, self->priv->location_apps_perms); well, they are needed to be able to re-use code. See update_perm_store() and it's usage from two functions below but I can still split them if you'd like. @@ +589,3 @@ + if (w != NULL) + { + gtk_switch_set_active (GTK_SWITCH (w), enabled); I really prefer an empty line before 'return' so it's very clear to readers that we're returning early here from the function, still your call. @@ +821,3 @@ + g_str_equal, + g_free, + g_object_unref); About keeping ref to the widget, the hashtable only lives as long as the panel itself so the widget switch widgets won't outlive that but yeah, I guess I'm being overcautious here. Applications are never removed and if they are going to be removed in future, it's going to be from the panel itself.
Review of attachment 325876 [details] [review]: ::: panels/privacy/cc-privacy-panel.c @@ +530,3 @@ data->pending_state = state; + g_variant_iter_init (&iter, self->priv->location_apps_perms); Just to be clear, I'm awaiting on you to tell me if you'd prefer me to split these after my explanation. @@ +589,3 @@ + if (w != NULL) + { + gtk_switch_set_active (GTK_SWITCH (w), enabled); Same here. Please let me know if you disagree with my rationale and I'll update the patch.
Attachment 325876 [details] pushed as 336851f - privacy: React to changes in permissions store