Created attachment 134283 [details] [review]
Please bear with me as this is my first time using Bugzilla to submit a patch.
While working on an indicator that appears in the panel/tray in elementary OS when applications are using GeoClue to fetch the devices location, one feature that we wish to implement is a list of those applications.
In the case above, this allows the user to have some awareness of what applications are accessing their location and I believe this would be useful feature in other desktop environments. This patch enables that functionality by adding a method that returns an array of the desktop IDs that are currently accessing GeoClue.
I look forward to hearing your feedback.
Thanks. While I understand the use case, there is a privacy issue involved: apps should not have access to this API but only agents. i-e we need to ensure this property is not accessible to anyone but the agents.
See gclue_service_client_handle_get_property() and gclue_service_client_handle_set_property() function definitions to see how to implement this.
Comment on attachment 134283 [details] [review]
Review of attachment 134283 [details] [review]:
Looks very good for a first patch actually. We already have a term for "indicators/prefs app": agent. :)
@@ +272,5 @@
> + GClueServiceManager *self = GCLUE_SERVICE_MANAGER (manager);
> + GClueServiceManagerPrivate *priv = self->priv;
> + GList *clients, *l;
> + int i = 0;
> + GStrv s = g_new (gchar*, g_hash_table_size (priv->clients) + 1);
* No reason for a GStrv if we are going to have to transform it into a flat array afterwards. Just create a flat char * directly.
* Please prefer slightly descriptive variable names.
@@ +282,5 @@
> + }
> + g_list_free (clients);
> + s[g_hash_table_size (priv->clients)] = NULL;
> + gclue_dbus_manager_complete_get_client_list (manager, invocation, (const gchar * const *)s);
does this call take ownership of s? If not, it's leaking here and you can just use an array on the stack instead with dynamically allocated members that you can free using g_strfreev().
@@ +60,5 @@
> + <!--
> + GetClientList:
> + @ids: An array of Desktop IDs (excluding .desktop) connected to geoclue
> + -->
> + <method name="GetClientList">
This should be a property: Clients
Created attachment 135199 [details] [review]
Add clients property to manager
I believe this patch addresses the comments you made on the previous one.
The method in which you access the client list is now via a property instead of a method and only agents can access this property.
To be able to identify an agent, I had to add a hash set to the manager that keeps track of DBus sender IDs. Agents cannot be XDG applications, so there is no way to use the already built XDG validation as used in gclue_service_client_handle_get_property().
Let me know your thoughts.
-- GitLab Migration Automatic Message --
This bug has been migrated to freedesktop.org'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.freedesktop.org/geoclue/geoclue/issues/57.