Added plugin LayerGroupWithEvents.

[glathoud]

Rationale:

• TileLayer fires "loading" and "load" events, making e.g. tile loading indicators possible (e.g. plugin Leaflet.loading).
• LayerGroup does not support any event.

Content: LayerGroupWithEvents extends LayerGroup to add event support ("loading" and "load").

For more details: https://github.com/Outdooractive/leaflet-layergroup-with-events_0.7

Remark: a direct modification of Leaflet's core's LayerGroup could be possible, but might be too much code for what it does.

[IvanSanchez]

A few commens about the plugin.

• Please make it obvious in the PR that the plugin is meant for 0.7.x only.
• Why extend L.LayerGroup instead of patching L.FeatureGroup? A L.FeatureGroup is expected to fire events, whereas a L.LayerGroup is not.
• Cannot this be done by simply patching up L.FeatureGroup.EVENTS in 0.7.x?

[glathoud]

Thanks Ivan for the comment, I simply missed FeatureGroup. Whether plugin or patch, it would be a better place.

Considering L.FeatureGroup.EVENTS: simply adding loading and load would lead to propagate the fired events of each layer of the group. I am not sure that that behaviour would be desirable for loading and load:

"loading" (L1 starts), "loading" (L2 starts), "load" (L1 finishes), "loading" (L3 starts), "load" (L3 finishes), "load" (L2 finishes)

instead, it might be more desirable to fire "loading" and "load" less often, ideally one time at the beginning and one time at the end:

"loading" (L1 starts), <silent> (L2 starts, L1 finishes, L3 starts, L3 finishes), "load" (L2 finishes)

This requires some logic, to check whether at least one layer is still loading. The proposed plugin implements that logic - at a suboptimal place, though.

[IvanSanchez]

This requires some logic, to check whether at least one layer is still loading.

Yeah, I see the point. But your logic is flawed: if your layergroup has a layer which never fires a load event (for example, a L.Marker), the group will never fire the load event. You cannot know a priori if a layer does implement the loading and load events (and what about loaderror?), so the logic will fail in more than a few use cases.

Besides, I just had a look at the code of Leaflet.loading, and it watches all layers which are firing loading and load events, so I still don't see the usefulness of this plugin.

(Also, by the Leaflet conventions on plugin naming, this should be something like L.LayerGroup.LoadEvents, inside the L.LayerGroup namespace).

[glathoud]

the logic will fail in more than a few use cases.

Yes, in a general sense, I agree. Now, for a loading indicator, not receiving "loading" and "load" at all would still be adequate when none of the layer is doing any asynchronous loading.

The use case I have only involves asynchrously loading layers like TileLayer. That is BTW one reason to prefer a plugin, which can be used for such use cases only - as opposed to patching the core code.

Besides, I just had a look at the code of Leaflet.loading, and it watches all layers which are firing loading and load events, so I still don't see the usefulness of this plugin.

Ok I have to describe my use case in more details :)

We have been providing tilelayer instances to our clients, as well as groups of tilelayers. In the group case, it would be bad API design to let our clients add and remove each layer of the group themselves, so we have been providing them LayerGroup instances. So far, fine.

Now some clients use Leaflet.loading to display a loading indicator. They reported that this works fine in the single tilelayer case, but does not in the LayerGroup case.

On the other hand, I looked at the place you mentionned in Control.Loading, as well as Map.eachLayer and LayerGroup.addLayer and I could not see anything contradicting your analysis.

I'll look very soon deeper into this and comment here again.

[glathoud]

I narrowed down the issue: when using Control.Loading in conjunction with a Control instance, and a LayerGroup or FeatureGroup as one of the basemaps fed to the Control instance, then the loading indicator appears and never disappears.

The use case looks like this:

var control = L.control.layers( { a : some_tilelayer, b : some_layergroup } );
var map = L.Map( 'map', { ... , layers : [ some_tilelayer ], loadingControl : true } );
control.addTo( map );

To determine: Whether this problem is only due to Control.Loading, or something like a FeatureGroup.LoadEvents plugin is desirable.

[glathoud]

Here is the technical issue: as soon as one uses a Control instance, and the users switches to a LayerGroup basemap, the map will fire a baselayerchange, to which Control.Loading listens:

map.on({
  baselayerchange: this._handleLoading,

...but the LayerGroup or FeatureGroup never fires load, since it does not implement it. So, even if the Control.Loading instance correctly tracks all sublayers being loaded, it also tracks the LayerGroup, and expects it to fire load as well - which never happens. Hence the loading indicator stays forever.

One could argue that Control.Loading should not listen to baselayerchange, but in the past this has led to an issue. So in the present case, filing another issue for Control.Loading might not lead very far.

Unless someone objects, I am going to write a FeatureGroup.LoadEvents plugin. In any case, thanks for all the feedback so far.

[IvanSanchez]

@glathoud Yeah, that looks like a bug in L.Control.Loading rather than a need for a new plugin.

You could also modify L.Control.Loading to do something like

if (layer instanceof L.GridLayer) {
    layer.on('loading load', ....)
}

, as (if memory serves) only gridlayers (and tilelayers) fire these events.

[glathoud]

Good point. One could also be less restrictive and have a black list instead of a white list, excluding only instances of LayerGroup:

            _handleLoading: function(e) {
                if (!(e.layer  &&  e.layer instanceof L.LayerGroup))  // <<< inserted this line
                    this.addLoader(this.getEventId(e));
            },

In any case, then, the loading indicator does not appear right away, i.e. much later in case of a limited bandwidth, which basically revives that issue.

From a design perspective, the fact that Control.Loading, as it is now, shows its loading indicator right away after baselayerchange is not necessarily a bad thing. There is no guarantee how fast the layer or layergroup will actually start loading.

[glathoud]

I opened an issue in Leaflet.loading.

In addition, I also wrote a FeatureGroup.LoadEvents plugin, because:

• (1) I had to deliver a solution,
• (2) an extension of FeatureGroup that fires "loading" and "load" can be generally useful,
• (3) at this point of time I am honestly not sure whether the specific issue with Control.Loading can be solved within Control.Loading, including from a design point of view.

There is a corresponding pull request for plugin.md.