Calling unselect() on feature removed from layer causes errors

[pebXX2346]

A Popup (Cloud, closebox) can't be closed in certain circumstances.

e.g.:

http://openlayers.org/dev/examples/

'Dynamic POIs via a Text Layer'

* click on marker
* zoom out
* click close box
-> nothing happens, error in FF-Console
( Error: layer is null

Source File: http://openlayers.org/dev/OpenLayers.js[[BR]] Line: 1425

)

(Note: it works in example 'Popup Matrix')

(FF 3.6.13, Win2k:-) OpenLayer-Version:? (please note that in the js file, how should i know!) other people found same bug in maybe other circumstances.

A Usecase:  http://olm.openstreetmap.de/

[pebXX2346]

I found the same bug report (shame on me)

 http://trac.osgeo.org/openlayers/ticket/2515

opened around a year ago (shame on you)

maybe my note that it works in another example helps.

[jorix]

It's easy to reproduce, I have not looked at the reason for the error.

Error: layer is null in "Control/SelectFeature.js" line 483.

Tested on current SVN 11695.

[jorix]

After "moveend" or "refresh" events on POIs layer all features have been destroyed by the Strategy.BBOX.

(destroyed by the method "BBOX.merge" after "triggerRead" called by "update")

Patch: I add an "if" to handle the case, and explanations to the user about the reason for this "if".

NOTE: I have not found more examples affected by the same problem.

[jorix]

attachment:example-dynamic-POIs-3046.2.patch : Added in map.addPopup exclusive = true. After zooming and selecting another point not close the inicial popup as expected.

Please review.

[pebXX2346]

Well, ...

This bug is not about the example!

It is a bug in the library. I told you about the example since that is where e.g. the bug shows up. The url of the initial report  http://olm.openstreetmap.de/ is another usecase.

I think the lib should handle it. E.g. if the feature is removed (1), the popup that is owned by the feature should be removed too. Or there should be code to merge old and new features, or whatever an architect of OpenLayers may find right.

(1) in olm in zoom the layer is refreshed, that is (i assume) an indirect remove of the features.

Changing the example where the bug shows is just like destroying a warning light.

[jorix]

Replying to pebXX2346:

Well, ... This bug is not about the example! It is a bug in the library... Changing the example where the bug shows is just like destroying a warning light.

I agree with you, yes!

But it's not a bug repair what needs, is a major improvement what needs.

The theme of "Popus" is weak in OpenLayers. There is much work to improve what's there. But from the library as now is the example should be written with the changes I have proposed.

NOTE: I'm not OpenLayers team.

[pebXX2346]

Replying to jorix:

Replying to pebXX2346:

Well, ... This bug is not about the example! It is a bug in the library... Changing the example where the bug shows is just like destroying a warning light.

I agree with you, yes! But it's not a bug repair what needs, is a major improvement what needs. The theme of "Popus" is weak in OpenLayers. There is much work to improve what's there.

Also docs with working examples, snippets. When i became a heavy user (will need some years) i will write some.

But from the library as now is the example should be written with the changes I have proposed.

Ok, that may help people who look for code that work's.

NOTE: I'm not OpenLayers team.

I'm also not :-) Just a nearly first time user.

[crschmidt]

(In [11707]) The dynamic text layer uses an onPopupClose which looks to the layer and first calls unselect on the selectfeaturecontrol. However, this fails if the feature in question is no longer in the map -- for example, if you zoom in or otherwise change positions. (The selected feature no longer exists, so you call close on a feature which no longer has a layer object.) This change to the examples by Jorix fixes this behavior in the example.

Note that many people use similar functionalities in their code, so this likely means that what we *should* do is armor unselect() against being called with a feature with a null layer, but that involves a longer discussion -- do we still fire onUnselect? Do we still fire the unselect events? is there other cleanup we need to do? etc. -- so this is changing the example to demonstrate one way to armor application code against the problem.

Thanks to jorix for the suggestion on the example fix. (See #3046) (See #2515)

[crschmidt]

Closed #2515 as a dupe, but it has more details: specifically, the problem is in unselect() calling unhighlight() which expects the feature to have a layer, but unselect also has other events; I *think* what this might mean is that we have to bail early in the unselect code if the feature doesn't have a layer... but at that point I *think* that selectedFeatures might be bunk, because it will have a feature in it that no longer exists... I'm not sure exactly how deep into this rabbithole we want to go.

Note that this behavior is *not* a popup-specific problem: any selectfeature code that calls unselect on a feature which has been removed from the layer will fail.