destroyFeatures should remove features from the features array

[crschmidt]

Features, once destroyed, are unusable. Calling destroyFeatures on a vector layer should therefore also remove the features from the array, since there is no way that they can be used at that point.

[ahocevar]

As the test_Layer_Vector_destroyFeatures() function in test_Vector.html shows, features are properly removed from the features array. Only the way this is done may seem a little unintuitive from the view of the layer, because what happens is:

• layer.destroyFeatures() calls feature.destroy()
• feature.destroy() calls layer.removeFeatures(feature), which removes the feature from the features array.

We could maybe make this more intuitive by not calling layer.removeFeatures() in feature.destroy(), because we clean up something here that we did not create. Instead, layer.destroyFeatures() should do this, as proposed in this ticket. Also, Layer.Vector.removeFeatures could just call

            this.eraseFeatures([features]);

directly instead of

            if (feature.geometry) {
                this.renderer.eraseGeometry(feature.geometry);
            }

around line 305 to make it more intuitive.

I packed those suggestions into the attached patch. All tests pass in FF2 and IE6.

[crschmidt]

Hm, I actually like the fact that feature.destroy() removes itself from the layer if it still has it. I think Erik wants to go this way for markers as well (there's an open ticket on that), so I think this is acceptable. The general OL philosophy is "You don't destroy something you didn't create": this doesn't apply to the feature.vector since we're not destroying anything, just disconnecting from the layer (which is important, since destroyed features will throw errors if you have them on the layer still).

So, I like the changes to Layer, but I'd like to keep the Feature.Vector as is. Does that make sense?

[elemoine]

ahocevar, my bad: I'm the one who first said that destroyFeatures() wasn't removing the features from the array. Good catch.

crschmidt, now that we know feature.destroy() removes the feature from the feature array, do you still think destroyFeatures() should call removeFeatures() to make things clearer to the person reading the code?

[crschmidt]

I think that's a fine change, and the other catch that ahocever made is fine too -- this patch is essentially good other than the feature.vector bit, imho.

[elemoine]

I'm also in favor of this change.

[elemoine]

Hold on there are a couple of problems with this patch. I'm currently tracking them down...

[elemoine]

Replying to elemoine:

Hold on there are a couple of problems with this patch. I'm currently tracking them down...

The main issue: in destroyFeatures, if we remove the features from the layer before we loop over the feature to destroy each one of them we'll actually never get a chance to destroy them because the local variable features references an empty array of features. See?

[elemoine]

So I'd just say: let's forget about this ticket.

[ahocevar]

Replying to elemoine:

So I'd just say: let's forget about this ticket.

I'm fine with that. Obviously there was a reason for the code being a little unintuitive. And, as we agree, there is no bug.