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.

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

layeradd event is not working #647

Closed
wsenjer opened this issue Mar 16, 2016 · 7 comments
Closed

layeradd event is not working #647

wsenjer opened this issue Mar 16, 2016 · 7 comments

Comments

@wsenjer
Copy link

wsenjer commented Mar 16, 2016

Hello,
I wonder how to get the layeradd event works for MarkerCluster. For instance, the following code is not working.

cluster.on('layeradd', function(layer){
   console.log(layer);
});

Is there any workaround to make this works?

@ghybs
Copy link
Contributor

ghybs commented Mar 18, 2016

Hi,

You are right, normal "layeradd" and "layerremove" events are not fired by MCG, even though it is declared as an extension of L.FeatureGroup

As a workaround, you could simply "patch" the L.MarkerClusterGroup class before starting instantiating it, to wrap the addLayer and removeLayer into methods that fire the corresponding event at the end, e.g.:

L.MarkerClusterGroup.include({
  _originalAddLayer: L.MarkerClusterGroup.prototype.addLayer,

  addLayer: function (layer) {
    this._originalAddLayer(layer);

    return this.fire('layeradd', {layer: layer});
  }
});

Demo: https://jsfiddle.net/yez9wn01/8/

It might be more complicated to include it into the plugin, as we would have to wonder what to do when layer is a Layer Group: the group is not actually added, only its child layers…

@wsenjer
Copy link
Author

wsenjer commented Mar 18, 2016

@ghybs thanks that's working great.
My intention is to use the events with Leaflet.MarkerCluster.LayerSupport, I tried to modify the code in jsfiddle to work with LayerSupport but with no luck. I tried to include the patch into L.MarkerClusterGroup.layerSupport factory but the events never triggered. Am I miss something?

Here is the modified demo: https://jsfiddle.net/n74rayv5/

@ghybs
Copy link
Contributor

ghybs commented Mar 19, 2016

This should be do-able, provided that you rename the alias from _originalAddLayer to something else, as it is actually already used within Leaflet.MarkerCluster.LayerSupport.

Please note that this plugin only works with current stable version of Leaflet (0.7.7), whereas the JSFiddle was built with Leaflet 1.0.0-beta.2. Same with Leaflet.markercluster, make sure you use the appropriate version.

@danzel
Copy link
Member

danzel commented Mar 23, 2016

If this doesn't cause performance troubles then we should probably add the layeradd/remove events.

@ghybs
Copy link
Contributor

ghybs commented Mar 23, 2016

Hi,

It should be quite straight forward for non-group layers. We could test the performance impact. It would probably simply depend on Leaflet's performance at firing thousands of events.

I think the problem is really what to do with Layer Groups: as of today, MCG actually uses only the individual child layers, and discards the parent groups. Stated differently, MCG just transforms groups into arrays.

Therefore, mcg.hasLayer(group) currently returns false, but true for child non-group layers, even though they were not directly added.
This is a very different behaviour from L.LayerGroup.hasLayer (where children of child groups return false).

Therefore, should MCG fire the layeradd event for the group, even though it would later return false on hasLayer(group)?
Or fire the event for each of the individual child layers?

The above patch does it the first way.

Furthermore, what to do when trying to remove a group? Currently, MCG removes all child non-group layers, even though the group may never have been added itself to MCG.
Whereas a standard Layer Group will look only into its immediately known children, and do nothing if it was not previously there.

@danzel
Copy link
Member

danzel commented Mar 23, 2016

Yeah, the group vs child markers is a bit of a weird one.
Probably as things are right now we should just fire the events for the child markers.

There could be a plugin for MCG that keeps track of the FeatureGroups that are added to extend this functionality (Could also listen for when children are added to these FeatureGroups and add them to the MCG).
Not sure if this code should be part of MCG, doesn't seem like something most people would use.
^^ This could also implement hasLayer(group) and the other bits you've mentioned.

@danzel
Copy link
Member

danzel commented Jan 26, 2017

Support for this is in the codebase now. Pretty certain I've covered everything, but there is a chance I've missed something.
If you add or remove a layer twice, there are some circumstances where the event will be fired twice (even though the layer is only added once), fixing these is not nice, so I'm leaving that for now.

Please test and give feedback if you have a case that doesn't work correctly.

brunob added a commit to geodiversite/geodiversite_monolithe that referenced this issue Aug 14, 2018
brunob added a commit to geodiversite/geodiversite that referenced this issue May 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants