GNOME Bugzilla – Bug 736272
location: Assign values to accuracy level enums
Last modified: 2014-09-11 14:41:48 UTC
See patch.
Created attachment 285660 [details] [review] location: Assign values to accuracy level enums Without explicitly assigning values, they are different from the values assigned by geoclue and therefore we end up with incorrect accuracy.
Review of attachment 285660 [details] [review]: That doesn't seem to match what I have in /usr/include/geoclue/geoclue-types.h for GeoclueAccuracyLevel.
Review of attachment 285660 [details] [review]: how does your values look like?
typedef enum { GEOCLUE_ACCURACY_LEVEL_NONE = 0, GEOCLUE_ACCURACY_LEVEL_COUNTRY, GEOCLUE_ACCURACY_LEVEL_REGION, GEOCLUE_ACCURACY_LEVEL_LOCALITY, GEOCLUE_ACCURACY_LEVEL_POSTALCODE, GEOCLUE_ACCURACY_LEVEL_STREET, GEOCLUE_ACCURACY_LEVEL_DETAILED, } GeoclueAccuracyLevel; So with the values added: typedef enum { GEOCLUE_ACCURACY_LEVEL_NONE = 0, GEOCLUE_ACCURACY_LEVEL_COUNTRY = 1, GEOCLUE_ACCURACY_LEVEL_REGION, GEOCLUE_ACCURACY_LEVEL_LOCALITY, GEOCLUE_ACCURACY_LEVEL_POSTALCODE = 4, GEOCLUE_ACCURACY_LEVEL_STREET = 5, GEOCLUE_ACCURACY_LEVEL_DETAILED = 6, } GeoclueAccuracyLevel; I would expect whatever code uses the values in gsettings-desktop-schemas to "convert" them to GeoclueAccuracyLevel values.
(In reply to comment #4) > typedef enum { > GEOCLUE_ACCURACY_LEVEL_NONE = 0, > GEOCLUE_ACCURACY_LEVEL_COUNTRY, > GEOCLUE_ACCURACY_LEVEL_REGION, > GEOCLUE_ACCURACY_LEVEL_LOCALITY, > GEOCLUE_ACCURACY_LEVEL_POSTALCODE, > GEOCLUE_ACCURACY_LEVEL_STREET, > GEOCLUE_ACCURACY_LEVEL_DETAILED, > } GeoclueAccuracyLevel; > > So with the values added: > typedef enum { > GEOCLUE_ACCURACY_LEVEL_NONE = 0, > GEOCLUE_ACCURACY_LEVEL_COUNTRY = 1, > GEOCLUE_ACCURACY_LEVEL_REGION, > GEOCLUE_ACCURACY_LEVEL_LOCALITY, > GEOCLUE_ACCURACY_LEVEL_POSTALCODE = 4, > GEOCLUE_ACCURACY_LEVEL_STREET = 5, > GEOCLUE_ACCURACY_LEVEL_DETAILED = 6, > } GeoclueAccuracyLevel; As commented on IRC, you are looking at header from geoclue1.
As mentioned on IRC as well: <hadess> zeenix, i'd expected at least a "this corresponds to Geoclue..." as comments for each value zeenix, and the enum in the D-Bus XML definition
(In reply to comment #6) > As mentioned on IRC as well: > <hadess> zeenix, i'd expected at least a "this corresponds to Geoclue..." as > comments for each value You mean in the header here (as part of this patch)? > zeenix, and the enum in the D-Bus XML definition Its only linked in the XML doc comment and the docs for enum itself comes from the header file. Whats the usual convention to list integer values in doc comments for enums? Current it looks like: http://cgit.freedesktop.org/geoclue/tree/src/public-api/gclue-enums.h
(In reply to comment #7) > (In reply to comment #6) > > As mentioned on IRC as well: > > <hadess> zeenix, i'd expected at least a "this corresponds to Geoclue..." as > > comments for each value > > You mean in the header here (as part of this patch)? Yep. > > zeenix, and the enum in the D-Bus XML definition > > Its only linked in the XML doc comment and the docs for enum itself comes from > the header file. Whats the usual convention to list integer values in doc > comments for enums? Current it looks like: > > http://cgit.freedesktop.org/geoclue/tree/src/public-api/gclue-enums.h Looks like there's no way to export the enum in D-Bus itself. Having the corresponding geoclue values as requested above would be enough for me.
Created attachment 285665 [details] [review] location: Assign values to accuracy level enums Would this correct the integer values on existing systems?
Review of attachment 285665 [details] [review]: Looks good!
Attachment 285665 [details] pushed as 29dd1f8 - location: Assign values to accuracy level enums
We still need either: - all the values from the geoclue header in the gsettings enum header or: - apps (which ones?) to know the correspondance between the gsettings enum values and the geoclue values.
(In reply to comment #12) > We still need either: > - all the values from the geoclue header in the gsettings enum header Thats what my patch does, doesn't it? > or: > - apps (which ones?) to know the correspondance between the gsettings enum > values and the geoclue values. You mean apps accessing the dconf value? If so, its only control-center and gnome-shell. Why do they need to know the acual values behind?
So Ryan says that updating the values of enums should work and I have tested it with a simple app to work. However Ryan recommends not accessing enums as integers so if you like I can instead patch gnome-shell to read this enum setting as string and translate it to appropriate enum/integer before passing it to geoclue. Your choice but I wouldn't want to change the agent interface for this now (which has been a part of stable releases already).
(In reply to comment #14) > So Ryan says that updating the values of enums should work and I have tested it > with a simple app to work. Oh and he says gaps in enums aren't an issue.
(In reply to comment #14) > So Ryan says that updating the values of enums should work and I have tested it > with a simple app to work. However Ryan recommends not accessing enums as > integers so if you like I can instead patch gnome-shell to read this enum > setting as string and translate it to appropriate enum/integer before passing > it to geoclue. The problem isn't really the GSettings values, but the ones passed from gnome-shell to Geoclue. > Your choice but I wouldn't want to change the agent interface > for this now (which has been a part of stable releases already). But it hasn't been used at all... In any case, that means that the piece of code using the GSettings we're storing should use the enum values as symbolic names. So gnome-shell gets its "if (value == G_DESKTOP_LOCATION_ACCURACY_LEVEL_CITY) accuracy = GCLUE_ACCURACY_LEVEL_CITY;", and there are no changes needed in gsettings-desktop-schemas.
(In reply to comment #16) > (In reply to comment #14) > > So Ryan says that updating the values of enums should work and I have tested it > > with a simple app to work. However Ryan recommends not accessing enums as > > integers so if you like I can instead patch gnome-shell to read this enum > > setting as string and translate it to appropriate enum/integer before passing > > it to geoclue. > > The problem isn't really the GSettings values, but the ones passed from > gnome-shell to Geoclue. How is that a problem? Ryan said its bad to pass enums on dbus as ints but i didn't quite understand why he thinks so. Unlike in case of gsettings, on D-Bus they are ints all the way so I don't see whats the big deal. > > Your choice but I wouldn't want to change the agent interface > > for this now (which has been a part of stable releases already). > > But it hasn't been used at all... Its used by gnome-shell at least. If I simply change the type of the arguments from int to string, newer geoclue wont work with older shell or viceversa. > In any case, that means that the piece of code using the GSettings we're > storing should use the enum values as symbolic names. So gnome-shell gets its > "if (value == G_DESKTOP_LOCATION_ACCURACY_LEVEL_CITY) accuracy = > GCLUE_ACCURACY_LEVEL_CITY;", and there are no changes needed in > gsettings-desktop-schemas. Yeah but that would also mean gnome-shell reading this enum as string and then doing the translation to int/enum. I could do that but I must point out that it just seems wrong becase 1. why convert when we can simply use int all the way 2. why does g_settings_get_enum() return an int then if enums as its in gsettings are so wrong?
gnome-shell patch in bug#736479.
You're supposed to use strings over dbus, not enums/ints because otherwise you need both ends to agree on what each value corresponds to. You'd use g_enum_get_value_by_nick() and g_enum_get_value(). A bit late to change the Geoclue API, and I've been guilty of that as well. But the representation is different, and the values come from different places in our case. If we had a single source for the enum (like g-c-c using g-s-d enums) this would be a different matter.