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

Possible bug with onRemove #10

Open
OAGr opened this issue Nov 16, 2016 · 4 comments
Open

Possible bug with onRemove #10

OAGr opened this issue Nov 16, 2016 · 4 comments

Comments

@OAGr
Copy link

OAGr commented Nov 16, 2016

I experienced strange buggy behavior when calling removeLayer on a SimpleGraticule layer, then inspected it and found something odd. I believe this.map was typed instead of this._map.

onRemove: function(map) {
  map.off('viewreset '+ this.options.redraw, this.map);
  this.eachLayer(this.removeLayer, this);
},

Notice that it's calling map.off, with the input this.map. I don't believe this.map exists, just this._map.

Would something like this make more sense?

onRemove: function(map) {
  var graticule = this;
  this._map.off('viewreset ' + this.options.redraw, graticule.redraw, graticule);
  this.eachLayer(this.removeLayer, this);
},
@ablakey
Copy link
Owner

ablakey commented Nov 16, 2016

I think you're right that there's a problem, but I don't think the suggestion you provided makes sense. I don't understand what the map.off line is trying to do. The context has to be the same context used for map.on (http://leafletjs.com/reference-1.0.0.html#map-event)

Try this and see if it resolves your issue. If it works, let's get it merged. Notice the only change is to replace this.map with map which should be the correct context.

onRemove: function(map) {
  map.off('viewreset ' + this.options.redraw, map);
  this.eachLayer(this.removeLayer, this);
},

@ablakey
Copy link
Owner

ablakey commented Nov 16, 2016

Also, based on the contract: http://leafletjs.com/reference-1.0.0.html#map-event

onAdd and onRemove should return this. Note to self to do this as well.

@OAGr
Copy link
Author

OAGr commented Nov 16, 2016

[edited]
I spoke too soon. Your solution doesn't seem to work for me. It works if there was no zoom applied, but if there was a zoom applied, it doesn't.

That seems to also work (both solutions do), at least in my one use case.

@OAGr
Copy link
Author

OAGr commented Nov 16, 2016

This section of their docs makes it seem to me like the map.off should basically be on the same things as the map.on.

Also, in their docs for removeEventListener it reads,

"Removes a previously added listener function. If no function is specified, it will remove all the listeners of that particular event from the object. Note that if you passed a custom context to addEventListener, you must pass the same context to removeEventListener in order to remove the listener."

From them:

    onAdd: function (map) {
        this._map = map;

        // create a DOM element and put it into one of the map panes
        this._el = L.DomUtil.create('div', 'my-custom-layer leaflet-zoom-hide');
        map.getPanes().overlayPane.appendChild(this._el);

        // add a viewreset event listener for updating layer's position, do the latter
        map.on('viewreset', this._reset, this);
        this._reset();
    },

    onRemove: function (map) {
        // remove layer's DOM elements and listeners
        map.getPanes().overlayPane.removeChild(this._el);
        map.off('viewreset', this._reset, this);
    },

Looking closer at the docs, I think I recommend something similar to the following:

    onAdd: function(map) {
      this._map = map; // Just set inner var
      this.redraw(); // Redraw
      map.on('viewreset ' + this.options.redraw, this.redraw, this); //On the event 'viewreset x', run the redraw function on this.
      this.eachLayer(map.addLayer, map); //For each of this layers, add them to the map
    },

    onRemove: function(map) {
      map.off('viewreset ' + this.options.redraw, this.redraw, this); //On the event 'viewreset x', stop the event to redraw this.
      this.eachLayer(map.removeLayer, map);
    },

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

2 participants