GNOME Bugzilla – Bug 723684
Geolocation controls
Last modified: 2014-02-17 15:50:27 UTC
Gotta implement the system menu items for geolocation control: https://wiki.gnome.org/Design/OS/Location This will require new API in geoclue.
https://wiki.gnome.org/Design/OS/SystemStatus has some information.
Created attachment 269142 [details] [review] location: Provide a way to disable geolocation Now that we are indicating 'geolocation in use' to user, we better also provide at least a way to disable geolocation. Once this is in place, we can provide slightly better controls rather than simply on/off switch.
Review of attachment 269142 [details] [review]: Overall looks good, I don't really like sync dbus call in the compositor though. ::: js/ui/status/location.js @@ +74,3 @@ + if (this._userSetAccuracy) { + // If user set the max accuracy level, don't let geoclue override + this._userSetAccuracy = false; Why do we reset it here? @@ +83,3 @@ + // We (and geoclue) have currently no way to reliably identifying apps so + // for now, lets just authorize all apps as long as they provide a valid + // desktop ID. We also ensure they don't get more accuracy than global max. Why does having a valid desktop id matter? If we can't identify apps can't we just do return [true, allowedAccuracyLevel]; and add a fixme to fix it once we have sandboxing? I mean it seems rather arbitrary. @@ +152,3 @@ + + this._userSetAccuracy = true; + this._onGeoclueAppeared(); // Ensure geoclue is running Why would it not be running at this point? _onGeoclueAppeared is called in init (and runs sync!) so it should be there already. A sync dbus call inside the compositor is evil doing it multiple times is even more evil. @@ +158,3 @@ }); + +function clamp(value, min, max) { You are only using it in one place so probably not worth factoring out (even if we want to should be some generic place)
Review of attachment 269142 [details] [review]: ::: js/ui/status/location.js @@ +74,3 @@ + if (this._userSetAccuracy) { + // If user set the max accuracy level, don't let geoclue override + this._userSetAccuracy = false; When user turn on/off, here is how things go if geoclue isn't running already: 1. we set our private maxAccuracyLevel 2. We launch geoclue by creating a proxy to it a. We register as agent b. We emit notify about maxAccuracyLevel 3. geoclue (upon launch) sets maxAccuracyLevel from user config (It doesn't know if we launched it for providing it a new value) a. Our setter for maxAccuracyLevel is called Now 2.b can happen before or after 3.a. So we set this._userSetAccuracy before 2. and only unset it at 3.a to ensure geoclue doesn't override the new value. @@ +83,3 @@ + // We (and geoclue) have currently no way to reliably identifying apps so + // for now, lets just authorize all apps as long as they provide a valid + // desktop ID. We also ensure they don't get more accuracy than global max. Just to have some sanity check at least. We can't stop apps from lieing about this but IMO it would be nice to not make that super easy for them. As long as we don't pretend to user that we are implementing security here, its fine. @@ +152,3 @@ + + this._userSetAccuracy = true; + this._onGeoclueAppeared(); // Ensure geoclue is running Because geoclue times out and we don't want to keep it running over the whole session just so we may set a property on it. About the sync calls, well I see that gnome-shell codebase is quite full of those so I didn't think of it as such a big thing. I can change that if you like. Just keep in mind that this method will typically not be called a lot. @@ +158,3 @@ }); + +function clamp(value, min, max) { well, it also makes it clear what is happening. I prefer functions with descriptive names over comments.
I forgot to mention that this patch requires latest changes to geoclue that I haven't yet merged: http://cgit.freedesktop.org/geoclue/log/?h=wip/agent-acc-level
(In reply to comment #4) > Review of attachment 269142 [details] [review]: > > ::: js/ui/status/location.js > @@ +74,3 @@ > + if (this._userSetAccuracy) { > + // If user set the max accuracy level, don't let geoclue override > + this._userSetAccuracy = false; > > When user turn on/off, here is how things go if geoclue isn't running already: > > 1. we set our private maxAccuracyLevel > 2. We launch geoclue by creating a proxy to it > a. We register as agent > b. We emit notify about maxAccuracyLevel > 3. geoclue (upon launch) sets maxAccuracyLevel from user config (It doesn't > know if we launched it for providing it a new value) > a. Our setter for maxAccuracyLevel is called > > Now 2.b can happen before or after 3.a. So we set this._userSetAccuracy before > 2. and only unset it at 3.a to ensure geoclue doesn't override the new value. OK. > @@ +83,3 @@ > + // We (and geoclue) have currently no way to reliably identifying apps so > + // for now, lets just authorize all apps as long as they provide a valid > + // desktop ID. We also ensure they don't get more accuracy than global > max. > > Just to have some sanity check at least. We can't stop apps from lieing about > this but IMO it would be nice to not make that super easy for them. As long as > we don't pretend to user that we are implementing security here, its fine. OK. > @@ +152,3 @@ > + > + this._userSetAccuracy = true; > + this._onGeoclueAppeared(); // Ensure geoclue is running > > Because geoclue times out and we don't want to keep it running over the whole > session just so we may set a property on it. > > About the sync calls, well I see that gnome-shell codebase is quite full of > those so I didn't think of it as such a big thing. We should get rid of them not add more (we have a tracker bug somewhere). > I can change that if you > like. Just keep in mind that this method will typically not be called a lot. Yes please do, shouldn't be that hard. As for not being called often it blocks the compositor whenever it happens. > @@ +158,3 @@ > }); > + > +function clamp(value, min, max) { > > well, it also makes it clear what is happening. I prefer functions with > descriptive names over comments. OK.
(In reply to comment #5) > I forgot to mention that this patch requires latest changes to geoclue that I > haven't yet merged: > http://cgit.freedesktop.org/geoclue/log/?h=wip/agent-acc-level OK, are you going to merge them before the freeze? We should not have code that depends on unmerged / unreleased changes at this point in a release.
(In reply to comment #6) > > About the sync calls, well I see that gnome-shell codebase is quite full of > > those so I didn't think of it as such a big thing. > > We should get rid of them not add more (we have a tracker bug somewhere). That's bug 687362
(In reply to comment #7) > (In reply to comment #5) > > I forgot to mention that this patch requires latest changes to geoclue that I > > haven't yet merged: > > http://cgit.freedesktop.org/geoclue/log/?h=wip/agent-acc-level > > OK, are you going to merge them before the freeze? We should not have code that > depends on unmerged / unreleased changes at this point in a release. Yes. That is my plan. I just didn't want to merge them before I'm sure the new API is actually going to be used by someone.
(In reply to comment #8) > (In reply to comment #6) > > > > About the sync calls, well I see that gnome-shell codebase is quite full of > > > those so I didn't think of it as such a big thing. > > > > We should get rid of them not add more (we have a tracker bug somewhere). > > That's bug 687362 OK, I'll update the patch.
Created attachment 269205 [details] [review] location: Create Geoclue proxy asynchronously
Created attachment 269206 [details] [review] location: Rename a function Rename _onGeoclueAppeared to _connectToGeoclue as in the following patch we'll call it from other contexts then geoclue appearing.
Created attachment 269207 [details] [review] location: Make _connectToGeoclue() reentrent
Created attachment 269208 [details] [review] location: Provide a way to disable geolocation Now that we are indicating 'geolocation in use' to user, we better also provide at least a way to disable geolocation. Once this is in place, we can provide slightly better controls rather than simply on/off switch.
Created attachment 269210 [details] [review] location: Don't show menu if geoclue is totally incapable If geoclue can't provide location at all (no GPS or network), don't bother to show controls that don't serve any purpose.
Review of attachment 269205 [details] [review]: LG.
Review of attachment 269206 [details] [review]: Looks good but the subject is a bit odd .. maybe "location: Rename _onGeoclueAppeared to _connectToGeoclue" ?
Review of attachment 269207 [details] [review]: LG.
Created attachment 269211 [details] [review] location: Don't show menu if geoclue is totally incapable v2: Create the menu but just don't show it. In a following patch, I'll hook up to any changes to AvailableAccuracyLevel to update visibility of the menu.
Review of attachment 269208 [details] [review]: LG.
Review of attachment 269211 [details] [review]: OK.
Created attachment 269215 [details] [review] location: React to changes in available accuracy level GPS could be plugged in and out, network can go up and down and therefore available accuracy level can also change.
Review of attachment 269215 [details] [review]: ::: js/ui/status/location.js @@ +192,3 @@ + _onGeocluePropsChanged: function(proxy, properties) { + let unpacked = properties.deep_unpack(); + if ("InUse" in unpacked) This looks hacky. @@ +194,3 @@ + if ("InUse" in unpacked) + this._syncIndicator(); + if ("AvailableAccuracyLevel" in unpacked) Same here.
Review of attachment 269215 [details] [review]: ::: js/ui/status/location.js @@ +192,3 @@ + _onGeocluePropsChanged: function(proxy, properties) { + let unpacked = properties.deep_unpack(); + if ("InUse" in unpacked) is there a better way?
(In reply to comment #24) > Review of attachment 269215 [details] [review]: > > ::: js/ui/status/location.js > @@ +192,3 @@ > + _onGeocluePropsChanged: function(proxy, properties) { > + let unpacked = properties.deep_unpack(); > + if ("InUse" in unpacked) > > is there a better way? Seems like we are using this in other places as well so should be fine.
Attachment 269205 [details] pushed as 7826fb4 - location: Create Geoclue proxy asynchronously Attachment 269207 [details] pushed as 12ef034 - location: Make _connectToGeoclue() reentrent Attachment 269208 [details] pushed as 32a49b7 - location: Provide a way to disable geolocation Attachment 269211 [details] pushed as 9217f2c - location: Don't show menu if geoclue is totally incapable Attachment 269215 [details] pushed as 3113bac - location: React to changes in available accuracy level I'm keeping the bug open as I'd like to add control of GPS as well, as you can see in the mockup.
Created attachment 269306 [details] [review] location: Rename _setAccuracy to setMaxAccuracyLevel More descriptive name to avoid any confusion.
Created attachment 269307 [details] [review] location: Coding style fixes No space between identifier and '('.
Created attachment 269308 [details] [review] location,schema: Keep max accuracy in gsetting Instead of relying on geoclue to store this user configuration, lets keep it in gsettings. Geoclue is a system service and therefore is not the appropriate entity to keep this info.
Created attachment 269309 [details] [review] location: 'MaxAccuracyLevel' property is now read-only
Review of attachment 269306 [details] [review]: OK.
Review of attachment 269307 [details] [review]: LG.
Review of attachment 269308 [details] [review]: ::: data/org.gnome.shell.gschema.xml.in.in @@ +154,3 @@ + allowed to see. Valid options are 'off' (disable location tracking), + 'country', 'city', 'neighborhood', 'street', and 'exact' (typically + requires GPS receiver). I am not sure I like this language it somehow implies that we can enforce that but we currently can't. ::: js/ui/status/location.js @@ +87,3 @@ } + var allowedAccuracyLevel = clamp(reqAccuracyLevel, 0, this._getMaxAccuracyLevel()); Uh somehow missed that earlier we use "let" not "var" @@ +170,2 @@ + _notifyMaxAccuracyLevel: function() { + var variant = new GLib.Variant('u', this._getMaxAccuracyLevel()); Same here.
Review of attachment 269309 [details] [review]: OK.
Review of attachment 269308 [details] [review]: ::: data/org.gnome.shell.gschema.xml.in.in @@ +154,3 @@ + allowed to see. Valid options are 'off' (disable location tracking), + 'country', 'city', 'neighborhood', 'street', and 'exact' (typically + requires GPS receiver). Well these controls give that impression already but I'm ready to change the language. What do you suggest? ::: js/ui/status/location.js @@ +87,3 @@ } + var allowedAccuracyLevel = clamp(reqAccuracyLevel, 0, this._getMaxAccuracyLevel()); ah, didn't realize.
Created attachment 269346 [details] [review] location,schema: Keep max accuracy in gsetting v2: Use 'let' instead of 'var'.
Created attachment 269347 [details] [review] location: disconnect from 'g-properties-changed' signal
Created attachment 269348 [details] [review] location: Rename _updateMenuVisibility to _updateMenu We'll soon be doing more than just toggling the accuracy of menu itself in this function. Hence the renaming.
Created attachment 269349 [details] [review] location: Allow user to disable GPS When exact accuracy (i-e GPS) is available, allow user to disable that. When users don't want application to get their precise location, they can now opt for network-based geolocation only, which can be street-level at best.
(In reply to comment #39) > Created an attachment (id=269349) [details] [review] > location: Allow user to disable GPS > > When exact accuracy (i-e GPS) is available, allow user to disable that. > When users don't want application to get their precise location, they > can now opt for network-based geolocation only, which can be > street-level at best. Why just in that case? I'd show a toggle for all accuracy levels, as long as the location services have been used in the X last minutes. It wouldn't stop applications from doing geoip lookups themselves but in the future we could blacklist geoip services for containerised applications.
(In reply to comment #40) > It wouldn't stop > applications from doing geoip lookups themselves but in the future we could > blacklist geoip services for containerised applications. That doesn't work (ignoring that you can't blacklist all services for a moment) the app can use a proxy that calls the service and bypass the blacklist this way. The only way to stop it is to hide the ip from the app by only allowing connecting to the internet using a (specific) proxy. But that by itself is a privacy issue so ...
(In reply to comment #40) > (In reply to comment #39) > > Created an attachment (id=269349) [details] [review] [details] [review] > > location: Allow user to disable GPS > > > > When exact accuracy (i-e GPS) is available, allow user to disable that. > > When users don't want application to get their precise location, they > > can now opt for network-based geolocation only, which can be > > street-level at best. > > Why just in that case? I'd show a toggle for all accuracy levels, I had the same idea but Allan didn't agree that we should expose all the accuracy levels to user: https://raw.github.com/gnome-design-team/gnome-mockups/master/shell/system-menu/system-status-menu-geolocation.png > as long as > the location services have been used in the X last minutes. How is that relevant?
(In reply to comment #35) > Review of attachment 269308 [details] [review]: > > ::: data/org.gnome.shell.gschema.xml.in.in > @@ +154,3 @@ > + allowed to see. Valid options are 'off' (disable location tracking), > + 'country', 'city', 'neighborhood', 'street', and 'exact' (typically > + requires GPS receiver). > > Well these controls give that impression already Yeah we had that discussion on IRC already but well ... but I'm ready to change the > language. What do you suggest? Well it is what the apps can do through geoclue not what they can access but given that this isn't really exposed to the user ... I don't know. Maybe add "Apps that can access the network can use that to identify your location".
Review of attachment 269347 [details] [review]: LG.
Review of attachment 269348 [details] [review]: OK.
Review of attachment 269349 [details] [review]: ::: js/ui/status/location.js @@ +191,3 @@ + if (maxAccuracyLevel == 0) { + this._item.status.text = _("Off"); + this._onoffAction.label.text = "Turn On"; Call this onOffAction (easier to read that way imo). @@ +194,3 @@ + } else { + this._item.status.text = _("On"); + this._onoffAction.label.text = "Turn Off"; Those should be translateable as well.
Created attachment 269374 [details] [review] location,schema: Keep max accuracy in gsetting v2: The description of gsettings key now warns of applications still being able to query user's location on their own.
Review of attachment 269374 [details] [review]: LG.
Review of attachment 269349 [details] [review]: ::: js/ui/status/location.js @@ +191,3 @@ + if (maxAccuracyLevel == 0) { + this._item.status.text = _("Off"); + this._onoffAction.label.text = "Turn On"; Sure, it was more of a typo. Since this commit doesn't change the name and the patch that introduce it has already been pushed, I'll rather correct this in a separate patch after this one if you don't mind? @@ +194,3 @@ + } else { + this._item.status.text = _("On"); + this._onoffAction.label.text = "Turn Off"; Same here.
(In reply to comment #49) > Review of attachment 269349 [details] [review]: > > ::: js/ui/status/location.js > @@ +191,3 @@ > + if (maxAccuracyLevel == 0) { > + this._item.status.text = _("Off"); > + this._onoffAction.label.text = "Turn On"; > > Sure, it was more of a typo. Since this commit doesn't change the name and the > patch that introduce it has already been pushed, I'll rather correct this in a > separate patch after this one if you don't mind? > > @@ +194,3 @@ > + } else { > + this._item.status.text = _("On"); > + this._onoffAction.label.text = "Turn Off"; > > Same here. Yeah a separate patch is fine (and more "correct").
Created attachment 269375 [details] [review] location: Mark 2 user-visible strings for translation
Created attachment 269376 [details] [review] location: Rename _onoffAction to _onOffAction It was just a typo that this patch now corrects.
Review of attachment 269375 [details] [review]: OK.
Review of attachment 269376 [details] [review]: OK.
Review of attachment 269349 [details] [review]: Fine as is then.
Attachment 269306 [details] pushed as aa45999 - location: Rename _setAccuracy to setMaxAccuracyLevel Attachment 269307 [details] pushed as 38f2414 - location: Coding style fixes Attachment 269309 [details] pushed as 19406a2 - location: 'MaxAccuracyLevel' property is now read-only Attachment 269347 [details] pushed as 7c3892f - location: disconnect from 'g-properties-changed' signal Attachment 269348 [details] pushed as 59634b2 - location: Rename _updateMenuVisibility to _updateMenu Attachment 269349 [details] pushed as 5d05b66 - location: Allow user to disable GPS Attachment 269374 [details] pushed as d614619 - location,schema: Keep max accuracy in gsetting Attachment 269375 [details] pushed as e80c28a - location: Mark 2 user-visible strings for translation Attachment 269376 [details] pushed as 8057848 - location: Rename _onoffAction to _onOffAction I'll mark this as fixed now. Next for me would be to make geoclue report the actual available accuracy level and not lie about it.
Reverted "location: Allow user to disable GPS" in git master Had a long discussion with Bastien Nocera and Allan Day on IRC about this and in the end we decided to go with the simple on/off controls for now.