Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fixed displacing points specified from context menu #4904

Closed
wants to merge 1 commit into from
Closed

Fixed displacing points specified from context menu #4904

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 99 additions & 44 deletions app/assets/javascripts/index/directions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
OSM.Directions = function (map) {
var awaitingRoute; // true if we've asked the engine for a route and are waiting to hear back
var chosenEngine;
var coordinatesRegularExpression = "([+-]?\\d+(\\.\\d*)?)(?:\\s+|\\s*[/,]\\s*)([+-]?\\d+(\\.\\d*)?)";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid new RegExp("^" + coordinatesRegularExpression + "$") every time we use this variable, you can define it as var coordinatesRegularExpression = /^([+-]?\d+(\.\d*)?)(?:\s+|\s*[\/,]\s*)([+-]?\d+(\.\d*)?)$/; and it will automatically be regexp.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this expression have capture groups if they are not used? Instead there's endpoint.value.split(/[/,]/).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree, I could have used better solutions on many places. My intention was to fix the bug and to make it simple, short and readable as much as I can. The source code is already a little bit complex and requires refactoring. We can optimize it in some other PR.


var popup = L.popup({ autoPanPadding: [100, 100] });

Expand All @@ -20,18 +21,13 @@ OSM.Directions = function (map) {
weight: 12
});

var endpointDragCallback = function (dragging) {
if (map.hasLayer(polyline)) {
getRoute(false, !dragging);
}
};
var endpointGeocodeCallback = function () {
getRoute(true, true);
var endpointUpdatedCallback = function (fitBounds) {
getRoute(fitBounds, true);
};

var endpoints = [
Endpoint($("input[name='route_from']"), OSM.MARKER_GREEN, endpointDragCallback, endpointGeocodeCallback),
Endpoint($("input[name='route_to']"), OSM.MARKER_RED, endpointDragCallback, endpointGeocodeCallback)
Endpoint($("input[name='route_from']"), OSM.MARKER_GREEN, endpointUpdatedCallback),
Endpoint($("input[name='route_to']"), OSM.MARKER_RED, endpointUpdatedCallback)
];

var expiry = new Date();
Expand All @@ -51,7 +47,7 @@ OSM.Directions = function (map) {
select.append("<option value='" + i + "'>" + I18n.t("javascripts.directions.engines." + engine.id) + "</option>");
});

function Endpoint(input, iconUrl, dragCallback, geocodeCallback) {
function Endpoint(input, iconUrl, endpointCoordinatesUpdatedCallback) {
var endpoint = {};

endpoint.marker = L.marker([0, 0], {
Expand All @@ -71,8 +67,8 @@ OSM.Directions = function (map) {
var dragging = (e.type === "drag");
if (dragging && !chosenEngine.draggable) return;
if (dragging && awaitingRoute) return;
endpoint.setLatLng(e.target.getLatLng());
dragCallback(dragging);

endpoint.setLatLng(e.target.getLatLng(), true, !dragging);
});

input.on("keydown", function () {
Expand All @@ -82,17 +78,31 @@ OSM.Directions = function (map) {
input.on("change", function (e) {
// make text the same in both text boxes
var value = e.target.value;
endpoint.setValue(value);
endpoint.setValue(value, false, null, true);
});

endpoint.setValue = function (value, latlng) {
endpoint.setValue = function (value, force_reverse_geocoding, latlng, fitMap) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a single endpoint control fitting?

Then below you have:

endpoints[0].setValue(params.from || "", !ll_from_search_form, from, false);
endpoints[1].setValue(params.to || "", !ll_from_search_form, to, true);

And not it's not clear why the first endpoint gets fitMap = false but the second one gets fitMap = true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a single endpoint control fitting?

Because the route can change on single endpoint update, so, it can go out of viewing area and we need to fit bounds.

Then below you have:

endpoints[0].setValue(params.from || "", !ll_from_search_form, from, false);
endpoints[1].setValue(params.to || "", !ll_from_search_form, to, true);

And not it's not clear why the first endpoint gets fitMap = false but the second one gets fitMap = true.

Because in some situations there is a flickering if we are doing fitting both times.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a single endpoint control fitting?

Because the route can change on single endpoint update

It's for the code outside of endpoints to determine what to do with the route.

if (!value && !latlng) {
endpoint.value = value;
delete endpoint.latlng;
input.val(value);

return;
}

if (value === endpoint.value) {
endpoint.setLatLng(endpoint.latlng, false, fitMap);
return;
}

endpoint.value = value;
delete endpoint.latlng;
input.removeClass("is-invalid");
input.val(value);

if (latlng) {
endpoint.setLatLng(latlng);
endpoint.setLatLng(latlng, true, fitMap);
} else if (!force_reverse_geocoding && endpoint.value.match(new RegExp("^" + coordinatesRegularExpression + "$"))) {
endpoint.setLatLng(L.latLng(endpoint.value.split(/[/,]/)), false, fitMap);
} else {
endpoint.getGeocode();
}
Expand All @@ -109,31 +119,64 @@ OSM.Directions = function (map) {

var viewbox = map.getBounds().toBBoxString(); // <sw lon>,<sw lat>,<ne lon>,<ne lat>

$.getJSON(OSM.NOMINATIM_URL + "search?q=" + encodeURIComponent(endpoint.value) + "&format=json&viewbox=" + viewbox, function (json) {
endpoint.awaitingGeocode = false;
endpoint.hasGeocode = true;
if (json.length === 0) {
input.addClass("is-invalid");
alert(I18n.t("javascripts.directions.errors.no_place", { place: endpoint.value }));
return;
}

endpoint.setLatLng(L.latLng(json[0]));
if (endpoint.value.match(new RegExp("^" + coordinatesRegularExpression + "$"))) {
var latlng_array = endpoint.value.split(/[/,]/);
$.getJSON(OSM.NOMINATIM_URL + "reverse?lat=" + latlng_array[0] + "&lon=" + latlng_array[1] + "&format=json&viewbox=" + viewbox, function (json) {
endpoint.awaitingGeocode = false;
endpoint.hasGeocode = true;

endpoint.setLatLng(L.latLng(latlng_array), false, true);

if (!json || !json.display_name) {
input.addClass("is-invalid");
alert(I18n.t("javascripts.directions.errors.no_place", { place: endpoint.value }));
return;
}

if (endpoint.latlng.distanceTo(L.latLng(json.lat, json.lon)) > 200.0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was this distance check about? Was it to filter out useless reverse geocoding results that are too far away from the endpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly! If it is found something near (e.g. 200m) the clicked coordinates, accept it, otherwise, display coordinates.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't do this in #5064 because it's a different problem, but it probably should be solved somewhere.

What's "near" probably depends on the current zoom level if the coordinates came from a click on the map.

input.val(endpoint.value);
} else {
endpoint.value = json.display_name;
input.val(json.display_name);
}
});
} else {
$.getJSON(OSM.NOMINATIM_URL + "search?q=" + encodeURIComponent(endpoint.value) + "&format=json&viewbox=" + viewbox, function (json) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When concatenating many strings, I think, using Template literals are more suitable and easier to read way.

endpoint.awaitingGeocode = false;
endpoint.hasGeocode = true;
if (!json || json.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be shortened as !json?.length

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let/const, template literals, ?. etc - we should decide on these in a separate issue, not in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it needs a separate issue, we can create it, however I didn't mean refactoring of the old code (which definitely would be a separate issue), but to change them in the new code. Also, if somewhere we have coding style guidelines, please, link it and I'll create an issue linked to it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at what is already in use by included libraries:

  • let/const - used by Bootstrap and Turbo
  • ?. - used by Turbo
  • template literals - used by Bootstrap and Turbo

input.addClass("is-invalid");
alert(I18n.t("javascripts.directions.errors.no_place", { place: endpoint.value }));
return;
}

endpoint.setLatLng(L.latLng(json[0]), false, true);

endpoint.value = json[0].display_name;
input.val(json[0].display_name);
});
}
};

input.val(json[0].display_name);
endpoint.setLatLng = function (ll, populate_input, fitMap) {
if (!ll) {
return;
}

geocodeCallback();
});
};
input.removeClass("is-invalid");

endpoint.setLatLng = function (ll) {
var precision = OSM.zoomPrecision(map.getZoom());
input.val(ll.lat.toFixed(precision) + ", " + ll.lng.toFixed(precision));
Comment on lines -130 to -131
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was used by $("#map").on("drop" ... to populate an input when the marker next to the input is dragged and dropped on the map.

What happens now:
Screencast from 04.07.2024 06:35:02.webm

What used to happen:
Screencast from 04.07.2024 06:38:26.webm

if (populate_input) {
var precision = OSM.zoomPrecision(map.getZoom());
endpoint.value = ll.lat.toFixed(precision) + ", " + ll.lng.toFixed(precision);
input.val(endpoint.value);
}
endpoint.hasGeocode = true;
endpoint.latlng = ll;
endpoint.marker
.setLatLng(ll)
.addTo(map);

endpointCoordinatesUpdatedCallback(fitMap);
};

return endpoint;
Expand Down Expand Up @@ -200,7 +243,7 @@ OSM.Directions = function (map) {
select.val(index);
}

function getRoute(fitRoute, reportErrors) {
function getRoute(fitBounds, reportErrors) {
// Cancel any route that is already in progress
if (awaitingRoute) awaitingRoute.abort();

Expand Down Expand Up @@ -246,15 +289,28 @@ OSM.Directions = function (map) {
$("#sidebar_content").html("<div class=\"alert alert-danger\">" + I18n.t("javascripts.directions.errors.no_route") + "</div>");
}

if (fitBounds) {
var partiallyCombinedBounds = L.latLngBounds();
partiallyCombinedBounds.extend(L.latLngBounds(o, o));
partiallyCombinedBounds.extend(L.latLngBounds(d, d));

map.fitBounds(partiallyCombinedBounds.pad(0.05));
}

return;
}

polyline
.setLatLngs(route.line)
.addTo(map);

if (fitRoute) {
map.fitBounds(polyline.getBounds().pad(0.05));
if (fitBounds) {
var fullyCombinedBounds = L.latLngBounds();
fullyCombinedBounds.extend(polyline.getBounds());
fullyCombinedBounds.extend(L.latLngBounds(o, o));
fullyCombinedBounds.extend(L.latLngBounds(d, d));

map.fitBounds(fullyCombinedBounds.pad(0.05));
}

var distanceText = $("<p>").append(
Expand Down Expand Up @@ -385,14 +441,13 @@ OSM.Directions = function (map) {
var pt = L.DomEvent.getMousePosition(oe, map.getContainer()); // co-ordinates of the mouse pointer at present
pt.y += 20;
var ll = map.containerPointToLatLng(pt);
endpoints[type === "from" ? 0 : 1].setLatLng(ll);
getRoute(true, true);
endpoints[type === "from" ? 0 : 1].setLatLng(ll, true, true);
});

var params = Qs.parse(location.search.substring(1)),
route = (params.route || "").split(";"),
from = route[0] && L.latLng(route[0].split(",")),
to = route[1] && L.latLng(route[1].split(","));
from = route[0] && L.latLng(route[0].split(/[/,]/)),
to = route[1] && L.latLng(route[1].split(/[/,]/));

if (params.engine) {
var engineIndex = findEngine(params.engine);
Expand All @@ -402,12 +457,12 @@ OSM.Directions = function (map) {
}
}

endpoints[0].setValue(params.from || "", from);
endpoints[1].setValue(params.to || "", to);
var ll_from_search_form = decodeURIComponent(location.search).match("^\\?from=" + coordinatesRegularExpression + "$");

map.setSidebarOverlaid(!from || !to);
endpoints[0].setValue(params.from || "", !ll_from_search_form, from, false);
endpoints[1].setValue(params.to || "", !ll_from_search_form, to, true);

getRoute(true, true);
map.setSidebarOverlaid(!from || !to);
};

page.load = function () {
Expand Down