Remove map callbacks when DrawControl removed from map

[clydebw]

This fixes a bug wherein if a DrawControl is added and removed from a Map several times, callbacks start accumulating, resulting in duplicate layers.

For example, the following adds a DrawControl to the map, and adds a polygon to it:

from ipyleaflet import Map, DrawControl
m = Map()
dc = DrawControl()
dc.data = [{
    'type': 'Feature',
    'properties': {
        'style': {
            'stroke': True,
            'color': '#3388ff',
            'weight': 4,
            'opacity': 0.5,
            'fill': True,
            'fillColor': None,
            'fillOpacity': 0.2,
            'clickable': True
        }
    },
    'geometry': {
        'type': 'Polygon',
        'coordinates': [
            [[-0.104027, 0.005665],
             [-0.069351, 0.023518],
             [-0.062141, 0.00103],
             [-0.104027, 0.005665]]
        ]
    }
}]
m.add_control(dc)
m

Now, by removing and adding the DrawControl several times, we can see that it becomes much darker in opacity, which is due to the same layer getting re-added to the map:

for i in range(10):
    m.remove_control(dc)
    m.add_control(dc)

Additionally, if we clear the DrawControl, and remove and add it again, we can observe it leaves behind a "ghost" polygon that we previously cleared:

dc.clear()
m.remove_control(dc)
m.add_control(dc)

This "ghost" disappears when we start drawing more Polygons, it appears to be the result of not updating the DrawControl model after updating the view.

Two additional callbacks appear to get added each time the DrawControl is added to the Map which I did not attempt to address:

this.model.on('msg:custom', this.handle_message.bind(this));
this.model.on('change:data', this.data_to_layers.bind(this));

Although I think they can probably be removed when the DrawControl is removed from the map too?

[clydebw]

Actually, I'm thinking it might be best to remove the callbacks specific to a particular DrawControl, in the event that multiple DrawControls exist on the map. 🤔

I'll test/make the changes.

[martinRenou]

Thanks a lot! I guess we've not been very careful on the "removing layers" use case. There might be issues with all kinds of layers

[clydebw]

@martinRenou no prob—this is an absolutely fantastic project, and thank you for all that you (and others) do!

I've went ahead and removed those other callbacks when the DrawControl is removed from the Map. On the multiple DrawControls case, I'm actually not sure how to tell events from different DrawControls apart. Some quick searching around seems to indicate that maybe there isn't a way? For instance, see this issue. Multiple DrawControls isn't a use case of mine, anyways.

[clydebw]

Hey @martinRenou, do you think this is OK to merge without addressing the "multiple draw control" use case described above?

[martinRenou]

Yes I agree we shouldn't address this use-case (not sure it's even a use-case we want to support anyway). Thanks!