destroy js errors for LegendPanel and LayerContainer

patch fixing the issue

Hey Bart - see also my comments on #86.

Same issue? If you uncomment map.destroy in the LayerContainer tests you can see the problem.

Hey Tim, I tried your patch but it did not solve my issue unfortunately.

Oh right, Tim's patch to #86 is not supposed to fix this issue, I was misreading.

Tim, when running with map.destroy from #86 I am getting:

exception: : object: 'length' is null or not an object

Is this what you mean? The error message is different from what I am getting though.

My error message is:

Error: 'undefined' is null or not an object

This actually breaks removing elements from the tree and legend. So it is not the correct solution.

Hey Bart, are you currently working on this? If not, I could investigate this a bit further today. Please let me know.

Hi Andreas, I haven't looked into it deeper unfortunately and won't be able to do that today, but the problem is I have not been able to create a reproduceable test case outside of that Geonetwork application. If you think you are able to help even without a testcase, then your help would be most welcome. Otherwise I'll try and investigate with the application over the weekend.

For a destroy issue that is easy to reproduce, see #90. May or may not turn out to be a duplicate of Bart's issue.

I did some research and found a pattern that causes the unload issue:

store.on("remove", function() {
    this.control.destroy();
    this.control = new OpenLayers.Control({layers: this.map.layers});
    this.map.addControl(this.control);
}, this);

This obviously recreates a control with the new set of layers, after a layer was removed. This works well, unless a layer is removed because we are destroying the map, in which case this.map.addControl() is likely to fail.

So as proposed by Tim, a beforedestroy event on the map would be helpful (see http://trac.openlayers.org/ticket/2136). In the meantime, we can check for map.unloadDestroy, which will be null when the map is being destroyed. When we stumble across this, we can unbind the store from the map.

The above patch does this, and adds a proper destroy method also. I tested with OpenGeo's GeoExplorer, which fails on unload without the patch, and unloads fine with the patch.

Please review.

detect map being destroyed and unbind

Andreas, I can confirm your patch works for me. Great work!

The alternative I came up with was to an extra Ext.get before actually removing something from the DOM, but your patch is fixing the root of the problem which is much better.

            if(node) {
                node.un("move", this.onChildMove, this);
                if (Ext.get(node)){
                    node.remove();
                }
            }

unbind when destroying

This looks good to me. Andreas, the only change I made was to remove the this.control stuff from the test. I don't think that was relevant - let me know if that was wrong.

Please commit.

(In [1085]) Unbind the store when the map is being destroyed. r=tschaub (closes #88)

Tim: I committed the version with the this.control stuff in the test and added a comment why it is there.

Replying to ahocevar:

Tim: I committed the version with the this.control stuff in the test and added a comment why it is there.

Yeah, I understood where it came from - and thanks for the comment. However, I still would suggest that it adds some confusion if things start failing again.

First this.control is created (where this is window). However, this.control is not added to the map. Then, the "remove" listener increments the count, destroys the control, creates another one, and adds it to the map. The test is set to fail if the count is too high. This part is clear, and if there is a regression, it will be easy to debug. In my opinion, adding the this.control stuff just confuses things.

Did you mean to put in something like the following:

            var control = new OpenLayers.Control();
            map.addControl(control);
            store.on("remove", function() {
                count++;
                control.destroy();
                control = new OpenLayers.Control();
                map.addControl(control);
            });

At least the code above doesn't leave around window.control after the test has run.

Compare how the following fails at r1084:

        function test_map_destroy(t) {
            t.plan(1);
            
            var map = new OpenLayers.Map({div: "mappanel", allOverlays: true});
            var a = new OpenLayers.Layer("a");
            var b = new OpenLayers.Layer("b");
            map.addLayers([a, b]);

            var store = new GeoExt.data.LayerStore({
                map: map
            });
            
            var count = 0;
            store.on("remove", function() {
                count++;
            });
            
            map.removeLayer(a);
            map.destroy();
            
            t.eq(count, 1, "store's remove handler called once");
        }

In my mind, this is a nice clear failure - and a regression would be easy to detect.

The existing test fails at r1084 in a weird way. The t.eq test is never run. Instead the "remove" listener is called and it tries to add a control to a map that has already been destroyed. The unhandled exception is about this.controls being null (which happens in map.destroy()). Looking at the test, I can imagine someone might think the this.control is related. It is not. The map.addControl(control) call is to blame.

Anyway, I don't want the comment to be taken the wrong way. You did a great job of debugging and fixing this. I just think it makes sense to create the simplest tests possible in case things start failing again.

(In [1102]) tschaub is right - the test will also show regressions without doing control stuff in the remove handler. (see #88)