-
Notifications
You must be signed in to change notification settings - Fork 911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify routing endpoint interface, always do reverse geocoding #5064
base: master
Are you sure you want to change the base?
Conversation
16d4433
to
a68b32c
Compare
Thanks for great PR! The sources are now much more readable and it seems a lot of bugs are fixed. Just a few remarks:
Screen.Recording.2024-08-13.125247.mp4Anyway, even with above remarks, I think this is great PR. Thanks for making it! |
One more thing, excuse me for missing it in above post:
Recording.2024-08-13.132359.mp4Thanks! |
Yes it would but not in this pull request. This is something that the code before this PR also doesn't do. |
They aren't removed in the original code either. The difference is it doesn't call nominatim, but it should have removed is-invalid as soon as the coordinates updated. |
eca5935
to
e671394
Compare
e671394
to
7aef609
Compare
7aef609
to
3adc0db
Compare
3adc0db
to
efcf2b5
Compare
efcf2b5
to
ddf8743
Compare
69664f7
to
9528487
Compare
c2c4952
to
b08845c
Compare
6d97492
to
1ae52b8
Compare
"2." form #5064 (comment) - I only see one request to Nominatim happening when "Directions from/to here" is selected. I fixed something like this in 2d1cf4c but wasn't it written before the comment? |
Recording.2024-08-21.120546.mp4If one point is valid and you change the second one, yes, there is only one request from nominatim. But, if one point is invalid and you change the second one, it will request from nominatim 2 times (once for each point). I used the latest sources at the moment on which I applied this PR. Thanks! |
@nenad-vujicic how about now? I added caching of not found results from Nominatim. |
It's perfect now. Thank you very much! There is one another issue, but probably not part of this PR. In some situations (take a look at attached video), on invalid search scene map fits to some other boundary box and route is displayed on wrong position. After pushing OK, it retrieves good route, displays it on good position and fits to good boundary box. Recording.2024-08-21.143742.mp4 |
Could be caused by |
I guess this can be merged now. I don't think there's anything else worth taking out and merging separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I agree with either part of the title of this PR as I don't really see any simplification - quite the opposite if anything. The "always do reverse geocoding" is also misleading because it implies extending an existing use when in fact this introduces reverse geocoding for the first time?
input.val(json[0].display_name); | ||
|
||
changeCallback(); | ||
}); | ||
} | ||
|
||
function getReverseGeocode() { | ||
var latlng = endpoint.latlng.clone(); | ||
var reverseGeocodeUrl = OSM.NOMINATIM_URL + "reverse?lat=" + latlng.lat + "&lon=" + latlng.lng + "&format=json"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to control the match precision here? Might this cause a location to "snap" to something a long way away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The location wouldn't snap in the same way is does on the currently deployed version. The pointer is not going to move, just the address would be too imprecise. Or too precise in an unhelpful way, for example when you zoom out to see a country and click on a city and you're told that you clicked on a house with this particular address.
Ultimately we should control the precision but I didn't want to do it in this pull request, see #4904 (comment)
Otherwise selecting "Directions from here" and then "Directions to here" results in two reverse-geocoding request for the starting point.
Do this instead of putting reversed input values into url.
49d1ab3
to
147b3f0
Compare
Reverse geocoding happens in the deployed version when Nominatim decides to do it. I don't know if it's intentional or accidental.
|
A large part of this pull request is already merged with other pull request. The simplification here is that you don't have to pass |
Can we rebase it then so we can see what's left? |
It is rebased. I mean I haven't updated the title since before any of those other pull requests. |
Now you only have to call
endpoint.setValue
with a single parameter. The parameter is checked if it's coordinates. If not, geocoding is performed. If yes, reverse geocoding is done. Reverse geocoding is also done on marker drag end.I did what I described in #5062 (comment). The coordinates are saved in data attributes on successful geocode, where "Directions from/to here" context menu can read them. This should fix #1874 and #2982.
Also addresses #1910 to some extent. But if we want to get back "Paris" from the coordinates and not some address inside Paris, we'd need to pass along a zoom level. Nominatim supposedly can use it. Or pass search queries along. Presumably that wasn't done to keep urls short, but we can pass only short queries for example.
Also should fix #1911 if the problem is markers moving from clicked locations to geocoding locations. In this case it's the step 3 of #2982 (comment).
May close #1754 because everything was rewritten.