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

Use baselayerchange/overlaylayerchange instead of layeradd/layerremove #5474

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+18 −12
Open

Use baselayerchange/overlaylayerchange instead of layeradd/layerremove #5474

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion app/assets/javascripts/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ $(document).ready(function () {
var expiry = new Date();
expiry.setYear(expiry.getFullYear() + 10);

map.on("moveend layeradd layerremove", function () {
map.on("moveend baselayerchange overlaylayerchange", function () {
updateLinks(
map.getCenter().wrap(),
map.getZoom(),
Expand Down
16 changes: 6 additions & 10 deletions app/assets/javascripts/index/layers/data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,14 @@ OSM.initializeDataLayer = function (map) {
onSelect(e.layer);
});

map.on("layeradd", function (e) {
if (e.layer === dataLayer) {
map.on("moveend", updateData);
updateData();
}
dataLayer.on("add", function () {
map.on("moveend", updateData);
updateData();
});

map.on("layerremove", function (e) {
if (e.layer === dataLayer) {
map.off("moveend", updateData);
$("#browse_status").empty();
}
dataLayer.on("remove", function () {
map.off("moveend", updateData);
$("#browse_status").empty();
});

function updateData() {
Expand Down
10 changes: 10 additions & 0 deletions app/assets/javascripts/leaflet.map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,14 +108,21 @@ L.OSM.Map = L.Map.extend({
layersAdded = "";

for (var i = this.baseLayers.length - 1; i >= 0; i--) {
var baselayerChanged = false;
if (layers.indexOf(this.baseLayers[i].options.code) >= 0) {
baselayerChanged = !this.hasLayer(this.baseLayers[i]);
this.addLayer(this.baseLayers[i]);
layersAdded = layersAdded + this.baseLayers[i].options.code;
} else if (i === 0 && layersAdded === "") {
baselayerChanged = !this.hasLayer(this.baseLayers[i]);
this.addLayer(this.baseLayers[i]);
} else {
baselayerChanged = this.hasLayer(this.baseLayers[i]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

baselayerchange is supposed to be fired once for the layer that became visible. It's not supposed to be fired for removed layers. See that null in Leafet code?
https://github.com/Leaflet/Leaflet/blob/c511e66c20dce06ac05c43cf287b3ecdba00d924/src/control/Control.Layers.js#L317-L319

"This event would be triggered after the other layer events, and it would include a layer property set to the resulting base layer." Leaflet/Leaflet#1064

Copy link
Author

@deevroman deevroman Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So the next handler also violates this expectation?

openstreetmap-website/app/assets/javascripts/leaflet.layers.js

Lines 62 to 71 in b45eb03

input.on("click", function () {
layers.forEach(function (other) {
if (other === layer) {
map.addLayer(other);
} else {
map.removeLayer(other);
}
});
map.fire("baselayerchange", { layer: layer });
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

The answer to the above question was yes but it probably didn't affect anything. However if we start using baselayerchange in more places, it might. For example, this code, if rewritten for baselayerchange wouldn't work correctly, if it expects this event to happen like Leaflet would fire it:

openstreetmap-website/app/assets/javascripts/index.js

Lines 208 to 216 in b45eb03

map.on("layeradd", function (e) {
if (e.layer.options) {
var goal = OSM.MATOMO.goals[e.layer.options.layerId];
if (goal) {
$("body").trigger("matomogoal", goal);
}
}
});

Copy link
Author

@deevroman deevroman Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that "baselayerchange overlaylayerchange" will work in this place (provided that baselayerchange overlaylayerchange is firing in all places that the layer is added)

this.removeLayer(this.baseLayers[i]);
}
if (baselayerChanged) {
this.fire("baselayerchange", { layer: this.baseLayers[i] });
}
}
},

Expand Down Expand Up @@ -295,6 +302,7 @@ L.OSM.Map = L.Map.extend({
}

if (callback) callback(this._objectLayer.getBounds());
this.fire("overlaylayerchange", { layer: this._objectLayer });
} else { // element handled by L.OSM.DataLayer
var map = this;
this._objectLoader = $.ajax({
Expand Down Expand Up @@ -326,6 +334,7 @@ L.OSM.Map = L.Map.extend({
map._objectLayer.addTo(map);

if (callback) callback(map._objectLayer.getBounds());
map.fire("overlaylayerchange", { layer: map._objectLayer });
}
});
}
Expand All @@ -335,6 +344,7 @@ L.OSM.Map = L.Map.extend({
this._object = null;
if (this._objectLoader) this._objectLoader.abort();
if (this._objectLayer) this.removeLayer(this._objectLayer);
this.fire("overlaylayerchange", { layer: this._objectLayer });
},

getState: function () {
Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/leaflet.share.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ L.OSM.share = function (options) {

marker.on("dragend", movedMarker);
map.on("move", movedMap);
map.on("moveend layeradd layerremove", update);
map.on("moveend baselayerchange overlaylayerchange", update);

$ui
.on("show", shown)
Expand Down
Loading