GNOME Bugzilla – Bug 702403
Add geolocation UI (pin clock)
Last modified: 2021-06-01 22:44:26 UTC
this bug report covers the status of the geolocation implementation
Created attachment 246975 [details] autogenerated vapi file This vapi file was autogenerated from the gir file, which is shipped with the geolocation-glib library.
Created attachment 247962 [details] [review] geocode-glib patch: Add support for Vala language based on vapigen The maintainer Bastien Nocera has declined the friendly request to support vala binding within the geocode-glib project. For more details please refer to the bug report: https://bugzilla.gnome.org/show_bug.cgi?id=702115 So the auto-generated vapi file should be shipped within gnome-clocks project.
Created attachment 247963 [details] [review] patch proposal for the geolocation support This is the first patch proposal for the geolocation support. Due to the coupling lack between geocode-glib and libgweather libraries the binding of the corresponding location matches is done manually, based on the smallest distance between those locations. The distance is calculated from two pairs of longitude and latitude data. This method is in my opinion not optimal and slow. However it is only used once per application start. Current design: first the saved world clocks are loaded and displayed (if present). then the geolocation lookup is performed successfully, this means the match is found, so the new world clock is added and saved if not already present. Current drawbacks: Some major cities like Berlin, London, New York, etc. have and display in the gweather-location-entry multiple entries (for new york more than 20), so if a user had previously added one, geolocation may add another one. should we spot it? Remaining questions: I am not sure, if an additional linkage to the math.h library is necessary! Do we need to include -lm. Or should we bother at all because we will move from a custom search to a better method?))
Review of attachment 247963 [details] [review]: ::: src/utils.vala @@ +66,3 @@ +} + +private double get_distance (double latitude1, double longitude1, double latitude2, double longitude2) { geocode_location_get_distance_from() instead? Saves you from reimplementing it. @@ +96,3 @@ + } + + search_locations_helper (lat, lon, locations[i], ref minimal_distance, ref found_location); Why do you call the location helper again? ::: src/world.vala @@ +352,3 @@ + GWeather.Location? found_location = null; + + Geocode.Ipclient ipclient = new Geocode.Ipclient (); You should use the geoclue2 API instead: http://gitorious.org/geoclue2 This API is not for application consumption (and will be removed shortly).
(In reply to comment #4) > Review of attachment 247963 [details] [review]: > > ::: src/utils.vala > @@ +66,3 @@ > +} > + > +private double get_distance (double latitude1, double longitude1, double > latitude2, double longitude2) { > > geocode_location_get_distance_from() instead? Saves you from reimplementing it. > > @@ +96,3 @@ > + } > + > + search_locations_helper (lat, lon, locations[i], ref > minimal_distance, ref found_location); > > Why do you call the location helper again? > > ::: src/world.vala > @@ +352,3 @@ > + GWeather.Location? found_location = null; > + > + Geocode.Ipclient ipclient = new Geocode.Ipclient (); > > You should use the geoclue2 API instead: > http://gitorious.org/geoclue2 > > This API is not for application consumption (and will be removed shortly). At the time of writing geoclue2 had no IpClient functionality
the geocode-glib library has removed the ipclient library, thus the uploaded vapi file in comment 2 is of no use anymore.
The tentative design of the geolocation integration can be found here https://wiki.gnome.org/Apps/Clocks/GeolocationIntegration
Created attachment 254388 [details] [review] rework of 247963: patch proposal for the geolocation support
Created attachment 254389 [details] [review] rework of 247963: patch proposal for the geolocation support
Review of attachment 254388 [details] [review]: When I said squashing the patches I meant with "git rebase -i", providing a single clean changeset with a single commit message. This still seems to carry all the development churn More comments below. ::: src/alarm.vala @@ +32,3 @@ } + public bool automatic { get; set; default = false; } empty line after this ::: src/geoclue.vala @@ +1,2 @@ +namespace Clocks { + No empty lines between namespace lines @@ +1,3 @@ +namespace Clocks { + +namespace GClue { why GClue if the module is called geoclue? Actually geoclue is the name of the library we are using, but the feature is called geocoding, so I'd call both the file and the namespace "geocode" @@ +135,3 @@ + } + + private async void search_locations (GWeather.Location location) { As discuessed on irc, we need to make this async since it noticeably blocks the UI when first parsing the libgweather db. @@ +167,3 @@ + } + + public bool is_location_similar (GWeather.Location location) { I guess this is the function that decides whether we show the city or not in the world clocks, right? If yes, I think it belongs in the world.vala file not here. Also I think we shold compare nation and time, not city and time: if I have a pinned "rome" clock and I travel to milan, I think I should not see an automated milan clock. As a user I know that italy is all in the same tz and I probably picked rome as a representation of "time in italy" and another clock with the same time would be redundant @@ +181,3 @@ + +} // GClue + no blank line here ::: src/widgets.vala @@ +57,3 @@ } +private class CustomTitleRenderer : Gtk.CellRendererText { everything in our name sapce is "custom". Let's simply call this TitleRenderer @@ -59,0 +59,51 @@ +private class CustomTitleRenderer : Gtk.CellRendererText { + private int ICON_XOFF; + private int ICON_YOFF; ... 48 more ... indentation is off here (tabs vs spaces) @@ -59,0 +59,52 @@ +private class CustomTitleRenderer : Gtk.CellRendererText { + private int ICON_XOFF; + private int ICON_YOFF; ... 49 more ... I think it would be cleaner if the icon to use was passed in from the caller, or null if no icon should be used @@ -292,2 +360,3 @@ } + public void add_first (Object item) { maybe "prepend" instead of "add_first"?
> +namespace GClue { > > why GClue if the module is called geoclue? > > Actually geoclue is the name of the library we are using, but the feature is > called geocoding, so I'd call both the file and the namespace "geocode" As per irc discussion, lets call the file geocoding.vala and the namespace just "Geo" so that it does not conflicts with the Geocode namespace of geocode-glib
Review of attachment 254389 [details] [review]: More nitpicks :) ::: src/geoclue.vala @@ +1,1 @@ +namespace Clocks { The new file is also missing the license etc. Please copy the text from one of the other files and put your name instead of mine :) @@ +11,3 @@ +private interface Client : Object { + public abstract string location { owned get; } + public abstract uint distance_threshold { get; set; } leave blank lines between the properties and the methods and between a method and another @@ +26,3 @@ +} + +public class GeoInfo : Object { If we call the namespace Geo, name this class just Info @@ +27,3 @@ + +public class GeoInfo : Object { + public signal void location_changed (GWeather.Location location); Put the signal after the properties and the private members, leave a blank line around it (I try to order things in this sequence properties, members, signals, methods) @@ +125,3 @@ + + private double get_distance (double latitude1, double longitude1, double latitude2, double longitude2) { + const double radius = 6372.795; I guess this is the earth radius, lets call it earth_radius for clarity :)
(In reply to comment #11) > > +namespace GClue { > > > > why GClue if the module is called geoclue? > > > > Actually geoclue is the name of the library we are using, but the feature is > > called geocoding, so I'd call both the file and the namespace "geocode" > > As per irc discussion, lets call the file geocoding.vala and the namespace just > "Geo" so that it does not conflicts with the Geocode namespace of geocode-glib :-) Then, we should think also of the class name, which is GeoInfo :-) at the moment we write GClue.GeoInfo ...
ok, you have considered that case already.
(In reply to comment #10) > Review of attachment 254388 [details] [review]: > > When I said squashing the patches I meant with "git rebase -i", providing a > single clean changeset with a single commit message. This still seems to carry > all the development churn > > More comments below. > > ::: src/alarm.vala > @@ +32,3 @@ > } > > + public bool automatic { get; set; default = false; } > > empty line after this > done! > ::: src/geoclue.vala > @@ +1,2 @@ > +namespace Clocks { > + > > No empty lines between namespace lines > > @@ +1,3 @@ > +namespace Clocks { > + > +namespace GClue { > done! > why GClue if the module is called geoclue? > > Actually geoclue is the name of the library we are using, but the feature is > called geocoding, so I'd call both the file and the namespace "geocode" > Okey, so I have renamed it to "Geo" and the class just to "Info" as you have suggested below. > @@ +135,3 @@ > + } > + > + private async void search_locations (GWeather.Location location) { > > As discuessed on irc, we need to make this async since it noticeably blocks the > UI when first parsing the libgweather db. > I think, that I already did so, so in the recursive function there is a yield part, which should unblock instead of stepping into the next recursion step. > @@ +167,3 @@ > + } > + > + public bool is_location_similar (GWeather.Location location) { > > I guess this is the function that decides whether we show the city or not in > the world clocks, right? If yes, I think it belongs in the world.vala file not > here. > > Also I think we shold compare nation and time, not city and time: if I have a > pinned "rome" clock and I travel to milan, I think I should not see an > automated milan clock. As a user I know that italy is all in the same tz and I > probably picked rome as a representation of "time in italy" and another clock > with the same time would be redundant > > @@ +181,3 @@ > + > +} // GClue > + > > no blank line here > thanks, done! > ::: src/widgets.vala > @@ +57,3 @@ > } > > +private class CustomTitleRenderer : Gtk.CellRendererText { > > everything in our name sapce is "custom". > > Let's simply call this TitleRenderer > definitely a better name:-) > @@ -59,0 +59,51 @@ > +private class CustomTitleRenderer : Gtk.CellRendererText { > + private int ICON_XOFF; > + private int ICON_YOFF; > ... 48 more ... > > indentation is off here (tabs vs spaces) thanks, corrected > > @@ -59,0 +59,52 @@ > +private class CustomTitleRenderer : Gtk.CellRendererText { > + private int ICON_XOFF; > + private int ICON_YOFF; > ... 49 more ... > > I think it would be cleaner if the icon to use was passed in from the caller, > or null if no icon should be used > hmm, need to think about this one... why would it be cleaner? > @@ -292,2 +360,3 @@ > } > > + public void add_first (Object item) { > > maybe "prepend" instead of "add_first"? yep, a good suggestion
(In reply to comment #12) > Review of attachment 254389 [details] [review]: > > More nitpicks :) > > ::: src/geoclue.vala > @@ +1,1 @@ > +namespace Clocks { > > The new file is also missing the license etc. Please copy the text from one of > the other files and put your name instead of mine :) enhanced > > @@ +11,3 @@ > +private interface Client : Object { > + public abstract string location { owned get; } > + public abstract uint distance_threshold { get; set; } > > leave blank lines between the properties and the methods and between a method > and another > done > @@ +26,3 @@ > +} > + > +public class GeoInfo : Object { > > If we call the namespace Geo, name this class just Info > done > @@ +27,3 @@ > + > +public class GeoInfo : Object { > + public signal void location_changed (GWeather.Location location); > > Put the signal after the properties and the private members, leave a blank line > around it (I try to order things in this sequence properties, members, signals, > methods) > done, but you better recheck that one :) > @@ +125,3 @@ > + > + private double get_distance (double latitude1, double longitude1, double > latitude2, double longitude2) { > + const double radius = 6372.795; > > I guess this is the earth radius, lets call it earth_radius for clarity :) Here, I am still thinking of to do what you has suggested, or to remove the custom distance calculation as Bastien has suggested long time ago. Because now we again depend on geocode-glib and therefore can as well use this function. but for this we need to turn the gweather.location instance into the geocode.location to make use of it, what do you think?
(In reply to comment #10) > Review of attachment 254388 [details] [review]: > > When I said squashing the patches I meant with "git rebase -i", providing a > single clean changeset with a single commit message. This still seems to carry > all the development churn > so, you were not satisfied with that?:-) What I did was: creating a new branch based on master and then merging without modifying the HEAD with "$ git merge --squash wip/geolocation" and then committing all as a single commit. Actually it was word by word that what you have said on irc something like "rebase and squash it" :-) > @@ +167,3 @@ > + } > + > + public bool is_location_similar (GWeather.Location location) { > > I guess this is the function that decides whether we show the city or not in > the world clocks, right? yep you are correct > If yes, I think it belongs in the world.vala file not > here. > > Also I think we shold compare nation and time, not city and time: if I have a > pinned "rome" clock and I travel to milan, I think I should not see an > automated milan clock. As a user I know that italy is all in the same tz and I > probably picked rome as a representation of "time in italy" and another clock > with the same time would be redundant > hmm, I understand your point, and I think it's worth to discuss it, we for sure both agree that this function should be better somehow. What is about this user cases: What if I am a tourist traveling in Italy, then it's not 100% clear to if there a timezone change or not? What if it's not a small country, but USA or russia? for example, if one travels from moscow to novosibirsk, you pass through three or four timezones. So, should we check for a country and timezone change? What would a user expect from geolocation in clocks? should clocks somehow respond, saying "hey, I know you are in another city!" or just keep quiet? Will a german citizen as a Clocks user go insane thinking "c'mon Clocks tell me something new", when pending between berlin and frakfurt, so that every time a frankfurt clock would appear or disappear for him?
Created attachment 254454 [details] [review] rework of 254389: patch proposal for the geolocation support
( > hmm, I understand your point, and I think it's worth to discuss it, we for sure > both agree that this function should be better somehow. What is about this user > cases: > > What if I am a tourist traveling in Italy, then it's not 100% clear to if there > a timezone change or not? As I see it, if you pinned in advance the "Rome" clock, chances are that you know that is the tz for the whole country. > What if it's not a small country, but USA or russia? > for example, if one travels from moscow to novosibirsk, you pass through three > or four timezones. As I said we need to check if *both* the country and the time match, if the country matches but the tz is different we show the geolocation clock. another city!" or just keep quiet? > > Will a german citizen as a Clocks user go insane thinking "c'mon Clocks tell me > something new", when pending between berlin and frakfurt, so that every time a > frankfurt clock would appear or disappear for him? The point here is whether he has a pinned clock or not. In the normal case I would not have a pinned clock and clocks will happily show Berlin or Frankfurt depending or where I am. But I *do* have a pinned clock for Berlin, showing also the one if Frankfurt would be annoying.
(In reply to comment #17) > so, you were not satisfied with that?:-) What I did was: creating a new branch > based on master and then merging without modifying the HEAD with "$ git merge > --squash wip/geolocation" and then committing all as a single commit. No, and I am still not satsified :-) I *never* want to see a "merge". I want a clean, linear, rebased commit history. That's why we use "wip" branches which allow rebased push.
geocode-glib bindings were added to vala cp. bug# 707275
Created attachment 254554 [details] [review] rework of 254454: patch proposal for the geolocation support
Created attachment 254555 [details] [review] rework of 254554: patch proposal for the geolocation support
Created attachment 254556 [details] [review] rework of 254555: patch proposal for the geolocation support
(In reply to comment #19) > ( > > hmm, I understand your point, and I think it's worth to discuss it, we for sure > > both agree that this function should be better somehow. What is about this user > > cases: > > > > What if I am a tourist traveling in Italy, then it's not 100% clear to if there > > a timezone change or not? > > As I see it, if you pinned in advance the "Rome" clock, chances are that you > know that is the tz for the whole country. > > > What if it's not a small country, but USA or russia? > > for example, if one travels from moscow to novosibirsk, you pass through three > > or four timezones. > > As I said we need to check if *both* the country and the time match, if the > country matches but the tz is different we show the geolocation clock. > another city!" or just keep quiet? > > > > > > Will a german citizen as a Clocks user go insane thinking "c'mon Clocks tell me > > something new", when pending between berlin and frakfurt, so that every time a > > frankfurt clock would appear or disappear for him? > > The point here is whether he has a pinned clock or not. In the normal case I > would not have a pinned clock and clocks will happily show Berlin or Frankfurt > depending or where I am. > > But I *do* have a pinned clock for Berlin, showing also the one if Frankfurt > would be annoying. yep, you are right actually :) so it would be something like this: diff --git a/src/geocoding.vala b/src/geocoding.vala index 14cd9ba..9c373a8 100644 --- a/src/geocoding.vala +++ b/src/geocoding.vala @@ -189,10 +189,19 @@ public class Info : Object { public bool is_location_similar (GWeather.Location location) { if (this.found_location != null) { - string? city_name = location.get_city_name (); - string? found_city_name = found_location.get_city_name (); - if (city_name == found_city_name) { - return true; + string? country_code = location.get_country (); + string? found_country_code = found_location.get_country (); + if (country_code != null && country_code == found_country_code) { + GWeather.Timezone? timezone = location.get_timezone(); + GWeather.Timezone? found_timezone = found_location.get_timezone(); + + if (timezone != null && found_timezone != null) { + string? tzid = timezone.get_tzid (); + string? found_tzid = found_timezone.get_tzid (); + if (tzid == found_tzid) { + return true; + } + } } }
Created attachment 254600 [details] [review] rework of 254556: patch proposal for the geolocation support
Review of attachment 254600 [details] [review]: This looks pretty good now. Do we need to bump the vala requirement? Next steps: - handle selection mode so that automatic items are not selectable: add a selectable bool prop to Item and set it to false for automatic locations. - add a way to "pin" an automatic child - (maybe) add a setting to turn off geolocation. Should this be a system wide setting?
(In reply to comment #27) > Review of attachment 254600 [details] [review]: > > This looks pretty good now. Do we need to bump the vala requirement? > yep, we should > Next steps: > - handle selection mode so that automatic items are not selectable: add a > selectable bool prop to Item and set it to false for automatic locations. can we use automatic property for that? > - add a way to "pin" an automatic child may be a button in the bottom panel, this would look more natural? > - (maybe) add a setting to turn off geolocation. Should this be a system wide > setting? We can have for sure one for Clocks. I am for this:)
(In reply to comment #28) > > - handle selection mode so that automatic items are not selectable: add a > > selectable bool prop to Item and set it to false for automatic locations. > can we use automatic property for that? I want in the same way you did for title_icon: at the World level we have "automatic", at the Widgets level we have "selectable" and we set one depending on the other. > > - add a way to "pin" an automatic child > may be a button in the bottom panel, this would look more natural? It is tricky... what happens when selecting both automatic and not automatic locations? I would stick to Allan's mockups for now, but on the other hand they are a bit tricky to implement. One thing that came to my mind is that the automatic location is just one: maybe in the bottom bar on the left we could have a button which says "Add Rome permanently" that does not require you to select the automatic location. We need to discuss this with Allan when he's back. > > - (maybe) add a setting to turn off geolocation. Should this be a system wide > > setting? > We can have for sure one for Clocks. I am for this:) I am pretty sure we can add a gsetting for this. Not sure whether we should have an UI or not, we'll see with Allan.
Created attachment 254611 [details] [review] do not allow selection for automatic locations Please review this one, I pushed it also to the wip/geoinfo branch
(In reply to comment #29) > > > - (maybe) add a setting to turn off geolocation. Should this be a system wide > > > setting? > > We can have for sure one for Clocks. I am for this:) > > I am pretty sure we can add a gsetting for this. Not sure whether we should > have an UI or not, we'll see with Allan. I have added a basic support for it. Should clocks react immediately on the property change? (like remove the automatic clocks when property is untoggled)
Comment on attachment 247962 [details] [review] geocode-glib patch: Add support for Vala language based on vapigen rejected upstream
Created attachment 254648 [details] [review] gsetting for geolocation actually the translation team should take care of its strings translation.
Geolocation support is implemented, what we are missing is: 1 - UI for "pinning" an automatic clock 2 - decide if we want to expose disabling geolocation in the UI With regard to (1) the current mockups have a proposed solution. I am not 100% convinced that is ideal and it poses some implementations problem. Variations of the idea proposed in the mockups are: - Use a symbolic icon (e.g) a "+" instead of "Add permanently" - Put an "Add <city> permanently" in the bottom bar Updating the title to reflect current status.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new enhancement request ticket at https://gitlab.gnome.org/GNOME/gnome-clocks/-/issues/ Thank you for your understanding and your help.