Created attachment 282914 [details] [review]
location: Don't mind available accuracy level

Not only tracking this property of geoclue makes the code a bit hard to
follow/maintain but its also not strictly needed. Just set the max
accuracy level to 'exact' when geolocation is enabled.

Created attachment 282915 [details] [review]
location: Separate setting for enabling/disabling

Having the on/off setting be backed by a boolean in dconf makes sense
anyway but this is mainly to be able to remember the max accuracy set
before user disabled geolocation so that when they enable it next time,
we have the max accuracy level on same value as before.

There hasn't been a real need for this but now we are about to add
geolocation settings in control center and it'll be easiser for
control-center to simply toggle a boolean property rather than to have
to know about and deal with accuracy levels.

Later we might also want to add accuracy level settings to privacy panel
so keeping the accuracy level setting around still.

Review of attachment 282914 [details] [review]:

Not sure - everything you add here is immediately removed with the next patch. Maybe it makes more sense to do the next patch first and then remove the (then-unused) available accuracy level?

Review of attachment 282915 [details] [review]:

Not sure about _updateMenuLabels() - assuming the '0' is not a valid accuracy level users can choose, the fallback in _getMaxAccuracyLevel() works, but checking the 'enabled' key to update the 'Enabled'/'Disabled' label looks far more obvious ...

Review of attachment 282915 [details] [review]:

Sorry, not exactly sure what you meant. -ETOOLONGSENTENCE. :)

Review of attachment 282914 [details] [review]:

Well I'm not adding anything other than the enum but i can rebase if you like.

Current code in _updateMenuLabels:

        if (this._getMaxAccuracyLevel() == 0) {
            this._item.status.text = _("Disabled");
            this._onOffAction.label.text = _("Enable");
        }

This reads slightly clearer to me:

        if (!this._settings.get_boolean('enabled')) {
            this._item.status.text = _("Disabled");
            this._onOffAction.label.text = _("Enable");
        }

(Jasper dislikes the !enabled form, so bonus points for swapping if and else block)

It's not a big deal, just that there's this function to update the menu items depending on whether location services are enabled, and a setting to enable/disable them, and the two are not directly connected.

(In reply to comment #6)
> Well I'm not adding anything other than the enum but i can rebase if you like.

You also change how _onOnOffAction works in both patches :-)

Created attachment 283001 [details] [review]
location: Separate setting for enabling/disabling

Having the on/off setting be backed by a boolean in dconf makes sense
anyway but this is mainly to be able to remember the max accuracy set
before user disabled geolocation so that when they enable it next time,
we have the max accuracy level on same value as before.

There hasn't been a real need for this but now we are about to add
geolocation settings in control center and it'll be easiser for
control-center to simply toggle a boolean property rather than to have
to know about and deal with accuracy levels.

Later we might also want to add accuracy level settings to privacy panel
so keeping the accuracy level setting around still. However we no longer
support 'off' accuracy level as the new boolean property covers that.

This also implies that we no longer track available accuracy level,
which made the code a bit hard to follow/maintain.

Created attachment 283003 [details] [review]
location: Separate setting for enabling/disabling

v2: Update of menu labels directly check the new boolean setting rather than relying on computed max accuracy level.

Review of attachment 283003 [details] [review]:

::: data/org.gnome.shell.gschema.xml.in.in
@@ -147,2 @@
   <enum id="org.gnome.shell.geoclue.AccuracyLevel">
-    <value value="0" nick="off"/>

I just tested setting 'max-accuracy-level' to 'off' before applying the patch, and ended up with location services enabled and at level 'exact' - I imagine this'll rub some people the wrong way, but I cannot think of a good way to avoid this off-hand.

Can we not use org.gnome.shell.* settings for this? Anything gnome-control-center touches should be in gsettings-desktop-schemas.

(In reply to comment #12)
> Can we not use org.gnome.shell.* settings for this? Anything
> gnome-control-center touches should be in gsettings-desktop-schemas.

Ouch, didn't know of that component even. I'll look into that.

Comment on attachment 283003 [details] [review]
location: Separate setting for enabling/disabling

Will provide separate patch to remove location settings from shell.

Attachment 283003 [details] pushed as f1957dc - location: Separate setting for enabling/disabling

Created attachment 283009 [details] [review]
location: Move settings to gsettings-desktop-schemas

Since these settings are now going to be accessed by
gnome-control-center as well, its more appropriate for them to live in
gsettings-desktop-schemas.

Review of attachment 283009 [details] [review]:

Sure

Attachment 283009 [details] pushed as ce2c90a - location: Move settings to gsettings-desktop-schemas