GNOME Bugzilla – Bug 736572
Reverse geocode the geoclue location and use the name
Last modified: 2015-08-15 20:58:02 UTC
Zeeshan told us via IRC that he miss the place name on the user location bubble, let's be kind :)
Created attachment 286058 [details] [review] userLocationBubble: Show place name
Created attachment 286059 [details] Screenshot
Review of attachment 286058 [details] [review]: You are fast. :) Looks good to me but as I said on IRC, you'd also want to do a reverse-lookup since geoclue will not give you name of the place at all anymore.
Review of attachment 286058 [details] [review]: but that can/should be done in additional patch(es). I didn't mark as 'commitnow' cause i'd like Jonas to look at it before its pushed.
(In reply to comment #3) > You are fast. :) Looks good to me but as I said on IRC, you'd also want to do a > reverse-lookup since geoclue will not give you name of the place at all > anymore. Oh... ping me the next time :), then this patch could not work in all the cases, let's leave it pending for now.
(In reply to comment #5) > (In reply to comment #3) > > You are fast. :) Looks good to me but as I said on IRC, you'd also want to do a > > reverse-lookup since geoclue will not give you name of the place at all > > anymore. > > Oh... ping me the next time :), next time? > then this patch could not work in all the > cases, let's leave it pending for now. No, it can already go in even if it won't actually do anything w/o the following patch to add reverse lookup.
Created attachment 286078 [details] [review] Move reverse geocoding to GeocodeService
Created attachment 286079 [details] [review] geoclue: Convert location property to place
Created attachment 286080 [details] [review] geoclue: reverse geocode the place name
Created attachment 286083 [details] [review] sidebar: Use current location in entry When we reveal the sidebar and no entries are filled, use the current location as from.
Created attachment 286084 [details] [review] userLocationBubble: Show place name
Created attachment 286085 [details] [review] sidebar: Use current location in entry When we reveal the sidebar and no entries are filled, use the current location as from.
Issue: If you open the sidebar, the current location will be there. Now close the sidebar. If you change your location, for instance with 'im-here' in the context-menu and then open the sidebar again the old current location will be there. Since there is now entries filled we do not replace it.
Oh btw sorry, I re-attached your patch by mistake.
Created attachment 286086 [details] [review] userLocationMarker: Check for accuracy unknown
Created attachment 286088 [details] [review] geoclue: Convert location property to place
Created attachment 286089 [details] [review] geoclue: reverse geocode the place name
Created attachment 286090 [details] [review] sidebar: Use current location in entry When we reveal the sidebar and no entries are filled, use the current location as from. For this to work when current location changes make sure we bind the name property to the entries text field.
(In reply to comment #13) > Issue: If you open the sidebar, the current location will be there. > > Now close the sidebar. > If you change your location, for instance with 'im-here' in the context-menu > and then open the sidebar again the old current location will be there. Since > there is now entries filled we do not replace it. Latest version changes placeEntry to bind_property (SYNC_CREATE) instead of setting the text. So if place.name changes so does the text property of the entry.
However I must say that I am not entirely convinced we want this. Reverse geocoding the place of the current location leaves us in the hands of the available data. For me Maps say that I am at a bar some hundred meters away. That is what the userlocation marker name shows and the route search with these patches. Whatever accuracy we have, the name will reflect a concrete place near those coordinates. And depending in where you live and the mapping done the result will wary. And there is the question of what coordinates to use. When ẃe reverse geocode we will get back the lat/lon of the place that was found. Do we use those or the ones we get from geoclue? If we use the ones from geoclue it will not match the place which name we just took and show to the users. We could even route from current location, with that name, to the place with the same name and get a route. Or do we use the coordinates from the reverse geocoded place? Isn't more common to just have "current location" as the name of our location. Since we are not sure where we are?
Review of attachment 286058 [details] [review]: ::: src/userLocationBubble.js @@ +43,3 @@ + ', ' + this.place.location.longitude.toFixed(5); + ui.labelPlaceName.label = this.place.name; If we want this we should bind_property the label property to the place.name property (SYNC_CREATE) so it updates when the location updates. Minor maybe.
(In reply to comment #20) > > And there is the question of what coordinates to use. When ẃe reverse geocode > we will get back the lat/lon of the place that was found. Do we use those or > the ones we get from geoclue? > > If we use the ones from geoclue it will not match the place which name we just > took and show to the users. We could even route from current location, with > that name, to the place with the same name and get a route. > > Or do we use the coordinates from the reverse geocoded place? Why don't we compare the coordinates and if distance is beyond some threshold, we simply discard the place name. Also a bit similarly, if accuracy of location is beyond a threshold, we don't do geocoding/show name of current location. > Isn't more common to just have "current location" as the name of our location. > Since we are not sure where we are? Try clicking on location marker in Maps application on android. If location is known with good enough accuracy and we have means of finding out name/description of current location, I think it would be wrong not to show that most useful info in the bubble that is meant for user to get details.
Yeah, I am not sure. The UX of it needs thought. Maybe it is something we shouldn't throw in now but wait for 3.16. We could also write "Near: <name>" in the bubble and use coords or "current location" in the route entry.
(In reply to comment #23) > Yeah, I am not sure. How so? Even if the algorithms I described aren't in any way perfect, I don't see slightly incorrect place name shown in the bubble as a big enough issue to delay this IMO but I'm not the one doing the work so up to you. >The UX of it needs thought. Maybe it is something we > shouldn't throw in now but wait for 3.16. > > We could also write "Near: <name>" in the bubble Thats would be wrong if accuracy is exact or very good. >and use coords or "current location" in the route entry. Sure. coords would seem very weird though.
(In reply to comment #20) > However I must say that I am not entirely convinced we want this. > > Reverse geocoding the place of the current location leaves us in the hands of > the available data. > > For me Maps say that I am at a bar some hundred meters away. That is what the > userlocation marker name shows and the route search with these patches. > Whatever accuracy we have, the name will reflect a concrete place near those > coordinates. > > And depending in where you live and the mapping done the result will wary. You are right, we need a smarter algorithm that include accuracy as a variable, if the accuracy is poor, we want to show in the bubble the name of the neighborhood or city and a venue name if the accuracy is very good. > > And there is the question of what coordinates to use. When ẃe reverse geocode > we will get back the lat/lon of the place that was found. Do we use those or > the ones we get from geoclue? > > If we use the ones from geoclue it will not match the place which name we just > took and show to the users. We could even route from current location, with > that name, to the place with the same name and get a route. > > Or do we use the coordinates from the reverse geocoded place? I think we need to use the the original geoclue coordinates. See my next lines quote response. > > Isn't more common to just have "current location" as the name of our location. > Since we are not sure where we are? Indeed... I think the words "Current location" are essential for the place entry, unless we have guarantee of the accuracy of the reverse lookup so the user won't think "Why the fuck Maps is using this for the From entry?", the words Current Location clarifies our intentions to the user. Actually, when Zeeshan told me to implement this feature, I always was thinking about put "Current Location" in the place entry and thinking the reverse lookup stuff to be used for the bubble, not for the route entry. If we still want to show the place name in the entry, we need to show it as secondary information (grayed or in parentheses). (In reply to comment #23) > Yeah, I am not sure. The UX of it needs thought. Maybe it is something we > shouldn't throw in now but wait for 3.16. > Maybe, I think we still need some smarter reverse lookup (taking into account the accuracy as I mentioned above or some of what Zeeshan mentioned in #c22) if we want to match the current location with a name. I think the "set current location in From entry" can go in 3.14 but the "show place name in bubble" need a bit of work. > We could also write "Near: <name>" in the bubble and use coords or "current > location" in the route entry. Indeed... also, Current location is already a translated string, so we are not breaking translations and we can skip the word Near.
Review of attachment 286078 [details] [review]: ::: src/geocodeService.js @@ +56,3 @@ + }, + + reverseGeocode: function(location, resultCallback) { I think "reverseLookup" is a better name.
Review of attachment 286078 [details] [review]: Also, I like this commit, maybe we can include it for 3.14 regardless of whether or not we can solve the UX issues of this feature.
Review of attachment 286086 [details] [review]: ::: src/userLocationMarker.js @@ +56,2 @@ refreshGeometry: function(view) { + if (this.place.location.accuracy < 0) { I would prefer "if (this.place.location.accuracy === Geocode.LOCATION_ACCURACY_UNKNOWN)"
Review of attachment 286086 [details] [review]: Now that I realize, I don't like this one, maybe we need to change the "if" in UserLocationMarker::_init instead if (this.place.location.accuracy !== 0) { for if (this.place.location.accuracy > 0) { So we don't create the AccuracyCircleMarker at all.
(In reply to comment #25) > (In reply to comment #20) > > > > Isn't more common to just have "current location" as the name of our location. > > Since we are not sure where we are? > > Indeed... I think the words "Current location" are essential for the place > entry, unless we have guarantee of the accuracy of the reverse lookup so the > user won't think "Why the fuck Maps is using this for the From entry?", the > words Current Location clarifies our intentions to the user. > > Actually, when Zeeshan told me to implement this feature, I always was thinking > about put "Current Location" in the place entry and thinking the reverse lookup > stuff to be used for the bubble, not for the route entry. If we still want to > show the place name in the entry, we need to show it as secondary information > (grayed or in parentheses). Oh, I thought you (Jonas) were talking of user location bubble and not 'From' entry of route. It is indeed common to just show 'current location' in there.
Review of attachment 286078 [details] [review]: ::: src/geocodeService.js @@ +56,3 @@ + }, + + reverseGeocode: function(location, resultCallback) { Maybe just 'reverse' Then we have geocodeService.search and geocodeService.reverse
Review of attachment 286086 [details] [review]: Yeah agreed.
> (In reply to comment #23) > > Yeah, I am not sure. The UX of it needs thought. Maybe it is something we > > shouldn't throw in now but wait for 3.16. > > > > Maybe, I think we still need some smarter reverse lookup (taking into account > the accuracy as I mentioned above or some of what Zeeshan mentioned in #c22) if > we want to match the current location with a name. > > I think the "set current location in From entry" can go in 3.14 but the "show > place name in bubble" need a bit of work. > Thanks for comments. So we could set Current location as name for the userLocation in the routing place entry. How about when a user types 'Current location' or erases the last letter and re-types it. Should we catch that? Should it be saved to place-store? Or do we need special casing?
Created attachment 286183 [details] [review] Move reverse geocode to GeocodeService * reverseGeocode() = > reverse() * resultCallback => callback
Created attachment 286184 [details] [review] userLocaiton;arer: improve accuracy guard changed to only show accuracy circle if accuracy > 0
Review of attachment 286183 [details] [review]: There's still a reference to Application.geocodeService.reverseGeocode in geoclue.js (in _updateLocation). ::: src/geocodeService.js @@ +69,3 @@ + this._latitude + ", " + + this._longitude + ": " + + e.message); Forgot to import Util. @@ +70,3 @@ + this._longitude + ": " + + e.message); + resultCallback(null); callback(null)
Created attachment 286362 [details] [review] placeEntry: Add current location action This needs the Convert location property to place patch to work. This simple patch adds a Current location action to the placeEntry. When clicking it it will set the place of the placeEntry to the place of the global geoclue module. So if that has a name, that name will be put in the entry, if it is null I guess it will show coordinates. There is some visual glitches, for instance when there is only one suggested match in the completion. I have no idea about that. This is a rfc patch.
Any comments here? Having the geoclue location as a place would help other parts of Maps. For instance when we want to add current location to route entries. Should we remove the name of it alltogether? Have the place have a name like _("Current location")? Should we add current location to placeStore? (And update it each time we get a new one from geoclue) so that we can complete against it in our placeEntry class?
Review of attachment 286089 [details] [review]: We do not want to reverse the name.
Review of attachment 286088 [details] [review]: pushed.