After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 765006 - privacy: React to changes in permissions store
privacy: React to changes in permissions store
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Privacy
unspecified
Other All
: Normal normal
: ---
Assigned To: Rui Matos
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-04-13 17:40 UTC by Zeeshan Ali
Modified: 2016-07-25 11:29 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
privacy: React to changes in permissions store (7.95 KB, patch)
2016-04-13 17:40 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2016-04-13 17:40:11 UTC
See attached patch.
Comment 1 Zeeshan Ali 2016-04-13 17:40:16 UTC
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.
Comment 2 Bastien Nocera 2016-04-14 12:35:01 UTC
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.
Comment 3 Zeeshan Ali 2016-04-14 12:49:54 UTC
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.
Comment 4 Zeeshan Ali 2016-04-19 16:39:22 UTC
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.
Comment 5 Zeeshan Ali 2016-07-25 11:29:03 UTC
Attachment 325876 [details] pushed as 336851f - privacy: React to changes in permissions store