Deactivate controls when destroying

[ahocevar]

A few controls deactivate themselves in destroy(), but most don't, leaving leftovers on the map. The proposed patch adds this.deactivate() to Control.destroy(), and does some requied cleanup in other controls' destructors to make this work reliably.

All tests pass in IE7 and FF3. Please review.

[elemoine]

IIRC crschmidt committed a patch that did this a while back. This commit was finally reverted because it led to issues, which I can't remember. Andreas, I haven't looked at your patch yet, but I thought you may have wanted to chech why this change was reverted. Cheers,

[crschmidt]

What little I remember on this topic is that due to the order of events when cleaning up the map, there were errors that were being caused in some browsers -- since destroy could be called multiple times, perhaps, or something like that, deactivate() was a problem because deactivate/destroy aren't 'safe'...

I'm not entirely sure this was deactivate, to be honest. I think it was more likely to be related to destroy, but I can't remember at the moment.

[ahocevar]

Calling deactivate in destroy causes map destruction to fail badly indeed. This can recently be seen with r10732, which we had to partially revert.

So until we have a better way to fix this (e.g. by adding a beforedestroy event to the map - see #2136 - and listening to it), we have a don't-call-deactivate-in-destroy policy.

[jorix]

Replying to ahocevar:

Calling deactivate in destroy causes map destruction to fail badly indeed. This can recently be seen with r10732, which we had to partially revert. So until we have a better way to fix this (e.g. by adding a beforedestroy event to the map - see #2136 - and listening to it), we have a don't-call-deactivate-in-destroy policy.

As refers to the Panel, if all controls are deactivated by the destruction, no error will occur. I tried to put this.active = null in control.js destroy. And map destruction with a panel with active controls not fail.

I also tested r10732 + #2210 and the tests:

        ...
        panel.activate();
        var div = panel.div;
        panel.destroy();
        t.eq(div.innerHTML , "", 
            "Panel is not displayed after destroy without any active control");
        map.destroy();
    }
    function test_destroy_map_with_panels(t) {
        t.plan(0);
        var map = new OpenLayers.Map("map");
        var control = new OpenLayers.Control();
        var panel = new OpenLayers.Control.Panel({defaultControl: control});
        panel.addControls([control]);
        map.addControl(panel);
        map.destroy(); //panel.destroy() also triggers the bug
    }

are working correctly.

Now in the case of panel, controls are added to the map after the panel. And map.destroy() destroys the controls in reverse order, when it's up to panel all controls are destroyed and if a control has control.active = true try to deactivate and then fails.

It may be interesting to study the possibility of doing a silent deactivation, and do not trigger events to destroy a control.

[jorix]

Replying to jorix:

... I also tested r10732 + #2210 and the tests ...

is ... I also tested r10834 + #2210 and the tests...

Thus #2210 is not a problem, is the solution solves two problems at once.

Note: Panel.js reqire a manual merge (without applying in panel.js test also works)

[jorix]

Replying to ahocevar:

Calling deactivate in destroy causes map destruction to fail badly indeed. This can recently be seen with r10732, which we had to partially revert.

See tickets collision described in  http://trac.osgeo.org/openlayers/ticket/2835#comment:7 This collision completely explain the problems with patchs in #2864.

(the test that r10834 removed is not the same as set by r10732)