geoext / geoext2

GeoExt 2 — JavaScript Toolkit for Rich Web Mapping Applications
http://geoext.github.io/geoext2/
Other
142 stars 106 forks source link

Dispose layer tree node event listeners. #357

Closed bentrm closed 9 years ago

bentrm commented 9 years ago

I think there are some dangling event listeners associated with the layer node. I hope to fix this with this PR.

As far as I can tell, there is no reasonable way to hook into the destruction of the LayerTreeModel of the LayerNode to fix this so I used the maps 'removelayer' event. 'preremovelayer' is not an option as other listeners outside of this scope may cancel removing the layer. That's also the reason for caching a reference to the actual map. When 'removelayer' is called the layers map property is already lost.

marcjansen commented 9 years ago

Any chance you can add some tests? On first glance this looks fine. Also, is #358 a replacement?

bentrm commented 9 years ago

There are no explicit tests for GeoExt.tree.LayerNode right now so I was a bit hesitant and deferred it. I'm happy to help but will not have time to look into this before the weekend.

marcjansen commented 9 years ago

As far as I can tell, there is no reasonable way to hook into the destruction of the LayerTreeModel of the LayerNode to fix this …

Have you tried (untested):

// …
init: function(target) {
    this.target = target;
    this.target.addListener('beforedestroy', this.cleanupMethod, this);
    // …
}
// …

Except for this and two other comments this is good to go.

bentrm commented 9 years ago

Yes, I tried that. this.target is an instance of GeoExt.data.LayerTreeModel which is an Ext.data.Model. The only event that is advertised seems to be idchanged. Furthermore, I tried to destroy the LayerNode plugin by hooking into the destroy method of the LayerTreeModel but it is never called as "destroy" of Ext.data.Model is used to delete the record which seems not to be the case here. I think the proposed solution is borderline hackish but I'm out of ideas.

marcjansen commented 9 years ago

I will give my OK for a merge if we add a big TODO somewhere and maybe even an issue as the approach seems more than hackish to me.

Nonetheless the result is better than what we had before, so plus one for merging this once it is updated with a big TODO.

Can you update this @bentrm?

bentrm commented 9 years ago

as the approach seems more than hackish to me.

I agree, absolutely. Will add a TODO.

chrismayer commented 9 years ago

Hi guys, any news on this? thanks!

bentrm commented 9 years ago

Promise to take care of this this afternoon. Sorry about that, I know you want to ship on 4th. :shipit:

bentrm commented 9 years ago

I amended the last commit to include TODOs hope you're ok with that. Thanks for your patience.

marcjansen commented 9 years ago

:+1: thanks!