Wrong event counting if IDs equal

[letmaik]

This control works based on an internal map that stores an event ID according to this code:

            getEventId: function(e) {
                if (e.id) {
                    return e.id;
                }
                else if (e.layer) {
                    return e.layer._leaflet_id;
                }
                return e.target._leaflet_id;
            },

This is quite problematic I think. The problem is, when I fire multiple dataloading events on the map instance, then these are registered as a single event only since the ID is identical for all of them (it is the map id) and therefore the spinner disappears once the first dataload event comes in, even though it should have been waiting for the others if it had counted correctly.

Why is this based on IDs anyway? It could be a simple counter, couldn't it? To not break the API and remove all the "loader" management functions (like addLoader and removeLoader), the plugin could store counts for each loader id instead of just true. Then this would support the case above and be fully backwards compatible.

[ebrelsford]

Thanks for reporting this, do you have a specific example of this breaking?

The project actually started with a simple counter as you suggest. The reason we switched to IDs is mentioned in #1: Leaflet tile layers (for example) will emit loading events every time the layer begins loading tiles and load events every time it completely finishes loading tiles. So it's very likely to get more loading events for a layer than load events, and load in Leaflet always means that the layer is finished loading.

As for dataloading and dataload, I can see how this would be a problem if you have multiple AJAX calls going at once. To keep it consistent with the way tile layers are tracked, I would lean toward passing an optional unique ID for the AJAX call along with a dataloading or dataload event. Would this work for your purposes?

[letmaik]

Hm.. sounds to me like the tile layer events of Leaflet are a bit broken. There should be only a single loading and load event pair for a given layer, even when new tiles get added to the loading queue since the loading event and before the load event. I can't imagine any use case for having these non-symmetric events, at least not like that.

Your idea with using IDs for dataloading as well would solve the issue of course, but it's a lot more effort to implement and not a common thing to do in my opinion. By the way getEventId() doesn't match its semantics. Different events having the same global ID doesn't make much sense. It's more like a event group id or something.

I'm not sure what the best solution here is except fixing leaflet. But I won't put IDs on my events all over the place, in some cases it is quite hard to manage the relationship of two events like that.

[ebrelsford]

I think it has more to do with the fact that TileLayer doesn't emit anything like a loadabort event or some other way of showing that it started loading anew before finishing loading the previous round. I would consider it to be just as incorrect if load was emitted when the layer wasn't actually done loading just to make the number of events emitted symmetric, so given the semantics of the events the behaviour makes sense.

That said, I did not write Leaflet, I just try to maintain this loading indicator plugin for it, so I'm not all that interested in how "broken" you think Leaflet is. Feel free to complain in the appropriate forum.

As for fixing the issue at hand, I'm very much open to counter-proposals and pull requests.

[letmaik]

I had a look in the leaflet master (to be 1.0) at tile/GridLayer.js, and
if I understood it correctly then the behaviour will change such that
there will be only a single "loading" event, and at the end a "load"
event. So my proposal would be... let's just wait it out and later the
the leaflet.loading code can be simplified back to its original counting
logic, assuming that the tile layer was the only reason for the more
complicated code.

Am 07.12.2015 um 15:39 schrieb Eric Brelsford:

I think it has more to do with the fact that |TileLayer| doesn't emit
anything like a |loadabort| event or some other way of showing that it
started loading anew before finishing loading the previous round. I
would consider it to be just as incorrect if |load| was emitted when
the layer wasn't actually done loading just to make the number of
events emitted symmetric, so given the semantics of the events the
behaviour makes sense.

That said, I did not write Leaflet, I just try to maintain this
loading indicator plugin for it, so I'm not all that interested in how
"broken" you think Leaflet is. Feel free to complain in the
appropriate forum https://github.com/Leaflet/Leaflet/issues.

As for fixing the issue at hand, I'm very much open to
counter-proposals and pull requests.

—
Reply to this email directly or view it on GitHub
#26 (comment).