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 736479 - location: Translate accuracy level for geoclue
location: Translate accuracy level for geoclue
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: system-status
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2014-09-11 14:05 UTC by Zeeshan Ali
Modified: 2014-09-11 16:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
location: Translate accuracy level for geoclue (2.23 KB, patch)
2014-09-11 14:05 UTC, Zeeshan Ali
reviewed Details | Review
location: Translate accuracy level for geoclue (2.25 KB, patch)
2014-09-11 14:50 UTC, Zeeshan Ali
reviewed Details | Review
location: Translate accuracy level for geoclue (1.93 KB, patch)
2014-09-11 15:58 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2014-09-11 14:05:40 UTC
See patch.
Comment 1 Zeeshan Ali 2014-09-11 14:05:43 UTC
Created attachment 285911 [details] [review]
location: Translate accuracy level for geoclue

The enums values of geoclue have gaps in them (so more levels could be
added in future) but enum values of settings don't have such gaps so we
gotta translate between them.

Since desrt says that enums as integers in gsettings are bad, we now
treat accuracy level settings as string.

This fixes the recent regression of geoclue only allowing geiop level
accuracy to apps.
Comment 2 Zeeshan Ali 2014-09-11 14:08:05 UTC
See bug#736272 for why we are not simply assigning values to enums in the settings.
Comment 3 Bastien Nocera 2014-09-11 14:43:25 UTC
Review of attachment 285911 [details] [review]:

::: js/ui/status/location.js
@@ +206,1 @@
             return 0;

What does 0 mean in the context of Geoclue? If it means disabled, I'd add that to the declaration of GeoclueAccuracyLevel above.
Comment 4 Zeeshan Ali 2014-09-11 14:50:05 UTC
Created attachment 285920 [details] [review]
location: Translate accuracy level for geoclue

v2: NONE=0 enum added to declaration.
Comment 5 Florian Müllner 2014-09-11 15:33:01 UTC
Review of attachment 285920 [details] [review]:

"Since desrt says that enums as integers in gsettings are bad, we now
treat accuracy level settings as string."

enums in settings aren't really the issue here, mapping between integer (G_DESKTOP_LOCATION_ACCURACY_LEVEL_EXACT) and string ("exact") is built-in, so you can use either option just fine. The problem is Geoclue's DBus API using magic numbers instead of descriptive string values, but that's nothing we can change now ...

::: js/ui/status/location.js
@@ +203,3 @@
+                return GeoclueAccuracyLevel.CITY;
+            else if (level == 'country')
+                return GeoclueAccuracyLevel.COUNTRY;

You could do something fancy like:

  let level = this._settings.get_string(MAX_ACCURACY_LEVEL);
  return GeoclueAccuracyLevel[level.toUpperCase()] || GeoclueAccuracyLevel.NONE;

here. If you do prefer a big if-else block, please write it as:

  let level = this._settings.get_enum(MAX_ACCURACY_LEVEL);
  if (level == GDesktopEnums.LocationAccuracyLevel.EXACT)
      return GeoclueAccuracyLevel.EXACT;
  else if (...)

@@ +207,1 @@
             return 0;

GeoclueAccuracyLevel.NONE
Comment 6 Zeeshan Ali 2014-09-11 15:53:30 UTC
Review of attachment 285920 [details] [review]:

> enums in settings aren't really the issue here, mapping between integer (G_DESKTOP_LOCATION_ACCURACY_LEVEL_EXACT) and string ("exact") is built-in, so you can use either option just fine. 

Not according to desrt, unless I misunderstood him.
Comment 7 Zeeshan Ali 2014-09-11 15:58:34 UTC
Created attachment 285932 [details] [review]
location: Translate accuracy level for geoclue

v3: Less code.

I didn't get to test this cause all of a sudden I'm getting this error from shell:

(gnome-shell:27339): Gjs-CRITICAL **: JS ERROR: SyntaxError: missing : after property id @ resource:///org/gnome/shell/ui/endSessionDialog.js:143
** Message: Execution of main.js threw exception: JS_EvaluateScript() failed
Comment 8 Florian Müllner 2014-09-11 16:11:45 UTC
(In reply to comment #7)
> I didn't get to test this cause all of a sudden I'm getting this error from
> shell:

My fault, fixed.
Comment 9 Florian Müllner 2014-09-11 16:15:07 UTC
Review of attachment 285932 [details] [review]:

OK
Comment 10 Zeeshan Ali 2014-09-11 16:49:26 UTC
Attachment 285932 [details] pushed as 7653175 - location: Translate accuracy level for geoclue

I tested it a but before pushing.