openstreetmap
Fixed displacing points specified from context menu
This PR addresses "Directions from here often places the point hundreds of meters elsewhere" issue mentioned in #2982
Fixes #2982 by updating directions.js so it doesn't perform unnecessary geocoding anymore for previously selected point (either source or destination), when entering second point. Also, improved setting markers by positioning it to selected position (not to result of geocoding).
How does it "address" that issue? What is the change you are making to address it?
Dear @tomhughes ,
Thank you for your review and comment. I updated PR and commit messages. Please, let me know if I can improve it further.
Thanks, Nenad.
so it doesn't perform unnecessary geocoding anymore for the previously selected point (either source or destination), when entering second point
Doesn't it still do unnecessary geocoding for the first point? And then its marker may get shifted as a result of this geocoding.
What an endpoint should do is to detect that it has coordinates as a value and don't try to query nominatim if it does. Search has server-side regexps to detect coordinates: https://github.com/openstreetmap/openstreetmap-website/blob/7cd900eca0af0f2eaeb54360ec8d1b9153434455/app/controllers/geocoder_controller.rb#L196 Here something simpler might suffice like number, number format without all those degree signs, N/S/E/W letters etc.
Dear @AntonKhorev ,
I've just updated PR with the latest, simplified and corrected sources. I made short video displaying new functionality (I added debugging alert("GEOCODING") call to directions.js just before $.getJSON(OSM.NOMINATIM_URL .. line to catch geocoding requests).
https://github.com/openstreetmap/openstreetmap-website/assets/25298521/c4247a43-3f3f-4100-a04b-7debab859945
Now, geocoding is called only once when user selects "Directions from here" or "Directions to there" or when new address is selected. Another way is to call geocoding only when address is typed, but, then, address will be lost when context menu options are selected (like when dragging markers, i.e. in From / To fields will be drawn Lat / Lng only).
Please, let me know which one option is ideal or if it could be further improved?
Thanks, Nenad.
Dear @AntonKhorev ,
Good catch, thank you very much! I've just fixed it and now it works fine! Please, let me know if I could further improve it.
Thanks, Nenad.
Going to Nominatim with coordinates still happens. Try entering "0,0" into from/to inputs.
Before the changes the code always went to Nominatim and searched for these numbers, found them somewhere in Taiwan (or elsewhere depending on the current map view) and placed the marker there.
After the changes the same happens except the marker is at 0, 0.
We should
- either decide that we're not searching for input values when they match the coordinate format,
- or never populate inputs with coordinates (and maybe populate them with reverse geocoding results).
Dear @AntonKhorev ,
Thanks for reviewing and comments. Now, we are contacting nominatim only if: point is defined using context menu (right click + Directions from/to here) or something is entered that doesn't look like coordinates (Number "," or "/" Number). Also, we improved focusing of map view, so it focuses on union of markers and path (if exists), previously it focused only on path.
Please, let me know if we could further improve it.
Thanks, Nenad.
Another bug:
- open the directions panel
- select directions from here from the context menu
https://github.com/user-attachments/assets/9b7a2e50-c768-4c5b-8c4b-c31c1e38f96a
Dear @AntonKhorev ,
Thank you for catching this! I've just fixed it. Please, let me know if I could further improve it.
Thanks, Nenad.
When user tries to add a marker out of the borders of countries, alert is thrown "Sorry - couldn't locate", but marker is still added and if user drags it to the valid position, route is not updated (see Video 1).
Video 1:
https://github.com/user-attachments/assets/0c503351-5a3f-4050-bbc8-9f5ed605a0f2
Contrary to this, if user adds marker in the valid location and then drags it out of the borders, route is still calculated and shown without any alerts (see Video 2).
Video 2:
https://github.com/user-attachments/assets/b10d91c4-5c47-4b1e-8975-69f1c97bf088
Maybe, in the first case, we should still show coordinates and calculate route even though Nominatim location was not found (as it is done in the second case).
The same logic is with "Couldn't find a route between those two places" error. If it is thrown, dragging location pin doesn't update route (see Video).
Video:
https://github.com/user-attachments/assets/4d45b9d1-be6a-4120-9704-519f71845b13
I know that we are using var everywhere, but I think, it would be great if we stick to let and const, where using var is not absolute necessity. let and const are less vague about scopes and much bug-proof. There are some examples Stackoverflow link. These are the lines you used "var" on:
Line 129: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR129
Line 179: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR179
Line 301: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR301
Line 316: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR316
Line 456: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR456
Line 469: https://github.com/openstreetmap/openstreetmap-website/pull/4904/files#diff-fdb9558c2ba79ddf9fbcb92594a8a8fa54abb1b3e25ce264de37951b428a958bR469