Use baselayerchange/overlaylayerchange instead of layeradd/layerremove

[deevroman]

Description

This is fix #5466 After merging openstreetmap/leaflet-osm#43 flame graph looks like this:

https://share.firefox.dev/4a2Wc9Z

Most of the time is spent on two handlers that update information about the current location and elements in the right sidebar.

Unfortunately, replacing layeradd layerremove with baselayerchange overlaylayerchange (#5466 (comment)) is difficult, since in addition to the Map Data layer, other elements may be displayed on the map. For example, the currently selected object on the map. It is not clear how to separate changes to Map Data objects and changes to the selected element. It is necessary to track changes in the active element, otherwise at least the Edit button will break.

Therefore, I implemented a little ugly, but safer in terms of breaking business logic. When adding the Map Data layer, I set the pause flag in the slowdown handlers. While the Map Data is rendering, the user cannot interact with the interface, so it seems safe to pause the event handlers.

Total 15s vs 0.5s when rendering 5000 elements.

https://share.firefox.dev/4h4IK7C

[AntonKhorev]

For example, the currently selected object on the map. It is not clear how to separate changes to Map Data objects and changes to the selected element. It is necessary to track changes in the active element, otherwise at least the Edit button will break.

addObject/removeObject in leaflet.map.js could fire some event for active object change. The listener that calls updateLinks could listen to that event.

[deevroman]

I also saw a problem in PR: when you click the Map Data checkbox, the cookie with the current location is no longer updated

[deevroman]

Okay, firing events from addObject / removeObject turned out to be the best idea. And I changed this PR to use baselayerchange/overlaylayerchange

I also replaced map.on("layeradd",...), which tracks the layer's on/off state with dataLayer.on("add", ...). Using overlaylayerchange in this case doesn't work, because this event isn't thrown when opening a new page with ...&layers=D Plus add / remove looks cleaner in my opinion.

[mmd-osm]

With the latest changes I was able to render Lake Huron locally in a reasonable amount of time (<20s, including 4.5s API response time; 455k nodes). Also map data display is extremely fast now (e.g. 100ms instead of 3s). That is definitely a significant speed-up.

Ideas for follow up PRs:

Furthermore, switching from XML to JSON has improved the response time from 20s to 8s (both values include 4.5s API time). For reference, I've uploaded the quick hack here: https://gist.github.com/mmd-osm/d45ff3b4f41908b1c075d70d1b6fdebe (edit: added fetch based + error handling alternative).

We should think about adding some error handling:

[HolgerJeromin]

Jquery uses JSON.parse for parsing of ajax() result.

In a next step switching to fetch would allow using
https://developer.mozilla.org/en-US/docs/Web/API/Response/json
Where the parsing is done without blocking the main thread.

[AntonKhorev]

We probably need to fire baselayerchange either from map.setState or from map.updateLayers. Otherwise if you edit &layers=X in the location bar, share urls are updated to its previous value.

If we follow the same principles as leaflet.layers.js, baselayerchange should be fired by the same function that does map.addLayer, so I guess map.updateLayers should fire it.

[deevroman]

Otherwise if you edit &layers=X in the location bar, share urls are updated to its previous value.

Do I understand correctly that you are talking about the current behavior of the website, and not after PR? At least in Firefox, changing &layers=X does not change the layers on the map, only if you open in a new tab

[mmd-osm]

@AntonKhorev 's comment wasn't exactly clear to me either. I think it's the following. Note that once you change the layer in the URL (in the example below from "C" to "H"), the share link "layer" attribute is updated accordingly:

This doesn't seem to work anymore with the changes in this PR.

Besides, there may be some timing issue in the current code, as the behavior is not 100% reproducible for me, even today. Sometimes the map layer changes along with the share link, in other cases, the old layer is still shown.

[deevroman]

Ah, I understand. I only tested changing the letters that represent the overlay (D/N/G). Changing the letters that respond to tile layers works now. PR broke it. I'll try to fix it

[AntonKhorev]

Leaflet ships with a layers control which lets you switch between layers. That thing is responsible for firing baselayerchange events. We don't use this control because we have our own layers menu. Therefore we're responsible for firing that event, which we do:

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

Line 70 in 914fff9

map.fire("baselayerchange", { layer: layer });

The problem is that our layers menu is not the only thing that can switch layers. All other places where layer change happens need also to fire baselayerchange. I think that map.updateLayers is the only such place. Lack of baselayerchange there becomes more noticeable with this PR because it uses this event.

Similar story with overlaylayerchange, although it's not a standard Leaflet event. Leaflet layers controluses overlayadd and overlayremove.

UPD: we're doing this wrong, see the following comments...

[deevroman]

Okay, I added the baselayerchange firing. So, when changing D/N/G, the update does not work now, so firing overlaylayerchange is better done in a separate PR.

[AntonKhorev]

This fires the event for every possible base layer. It's supposed to be fired only for the layer that became visible, that's how it works in leaflet.layers.js.

[deevroman]

Implemented a check that there is already a layer on the map. But the code doesn’t look so attractive anymore, so it might be better to add add/remove handlers for all base layers. For some reason the naive addition in initialize() does not work yet

layer.on("add remove", function () {
  this.fire("baselayerchange", { layer: layer });
})

[AntonKhorev]

You could put this.fire ... next to this.addLayer ..., that's one extra line of code. (with a caveat that if you're switching to the same layer, baselayerchange still fires; if that's wrong, our layer menu is also wrong)

[AntonKhorev]

Yes, looks like our layers menu is wrong. Leaflet's layer selector doesn't fire baselayerchange if you clicking an already active layer, and if you add an already added layer using map.addLayer, layer events also don't happen.

[AntonKhorev]

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

[deevroman]

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 });
	});

[AntonKhorev]

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);
	}
	}
	});

[deevroman]

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

[AntonKhorev]

Do we fix baselayerchange everywhere before doing this PR that changes listeners for this event?