Layer.Vector fires featuresadded event with shapes vetoed by beforefeatureadded

[timbonicus]

In OpenLayers.Layer.Vector#addFeatures, the beforefeatureadded event is fired allowing listeners to return false and veto adding the feature to the layer. The feature is still added to this.features in the class before this opportunity and fired with the featuresadded event at the end of the function.

The attached patch corrects this behavior and never adds the feature to this.features or fires it with the featuresadded event if the shape was vetoed.

[elemoine]

Moving this.features.push(feature) after beforefeatureadded is triggered makes perfect sense to me. But I disagree with sending this.features instead of features with the featuresadded event.

[timbonicus]

The features array will contain all features, this.features will contain only the features that were actually added after firing the beforefeatureadded events. Without the second change, featuresadded will fire with potentially extraneous features that were not added.

[timbonicus]

I see what you mean that this.features may contain other features that were added at an earlier time. It seems like features that were not added after firing the beforefeatureadded event need to be spliced from the features array.

[timbonicus]

patch using splice instead of this.features

[elemoine]

Comments on beforefeatureadded2.patch :

to my knowledge the slice method does not modify the array it's applied to. So I think we should introduce a local variable addedFeatures, initially referencing this.features, and set to features.slice(i, 1) when triggerEvent("beforefeatureadded") returns false. What do you think?

[timbonicus]

slice does not modify the original array; splice is a different function that does modify the original array. There is a problem with the second patch in that splice will concurrently modify the array we are iterating over. In this sense, your solution of having a copy of the array is much safer/preferable. I am attaching a new patch with this modification. See what you think.

[timbonicus]

new patch cloning features and splicing safely

[elemoine]

timbonicus, sorry for mixing up slice and splice, and thanks for beforefeatureadded3.patch , it looks good to me. It'd be great if you could check that the tests pass (at least in FF3), and add a test for this new behavior/correction.

Also, do we have a ICLA or CCLA from you already? I don't see you in the list at:  http://trac.openlayers.org/wiki/CLA. See:  http://trac.openlayers.org/wiki/HowToContribute for more details.

Thanks,

[timbonicus]

The tests run without issue, I'm adding another patch with a test for this functionality included. I switched the behavior to just use Array.push instead of the more confusing splice.

Thanks for pointing out the CLA, our CTO faxed in a CCLA this morning.

[timbonicus]

patch with included unit test

[elemoine]

Replying to timbonicus:

The tests run without issue, I'm adding another patch with a test for this functionality included. I switched the behavior to just use Array.push instead of the more confusing splice. Thanks for pointing out the CLA, our CTO faxed in a CCLA this morning.

Is your company now listed in < http://trac.openlayers.org/wiki/CLA>? Which one is it? Chris, have you received this CCLA and added to the list?

[timbonicus]

Replying to elemoine:

Is your company now listed in < http://trac.openlayers.org/wiki/CLA>? Which one is it? Chris, have you received this CCLA and added to the list?

The company is RealGo, it does not appear on that list as of yet.

[elemoine]

Replying to timbonicus:

Replying to elemoine:

Is your company now listed in < http://trac.openlayers.org/wiki/CLA>? Which one is it? Chris, have you received this CCLA and added to the list?

The company is RealGo, it does not appear on that list as of yet.

Chris, have we received a fax from RealGo? Thanks.

[elemoine]

Code looks good. Tests pass in FF3, Chromium5 and IE7. And we have the CCLA from RealGo. Thanks for patch timbonicus.

[tschaub]

Closed with r2549.