Opened 7 years ago

Last modified 6 years ago

#389 new enhancement

FeatureStore should not trigger a beforefeaturemodified event on the layer

Reported by: ahocevar Owned by:
Priority: major Milestone: 1.2
Component: GeoExt.data.FeatureStore Version: 1.0
Keywords: Cc:
State: None

Description

According to the OpenLayers API docs for Layer.Vector, the beforefeaturemodified event is "triggered when a feature is selected to be modified". So it is inappropriate to trigger this event in onUpdate.

Unless someone can provide a good reason why this is done, I'd be in favor of removing the code that does this, and checking if the feature is in the layer's selectedFeatures array instead (see attached patch).

Attachments (3)

geoext-389.patch (830 bytes) - added by ahocevar 7 years ago.
geoext-389.2.patch (2.2 KB) - added by ahocevar 7 years ago.
389.patch (1.5 KB) - added by tschaub 7 years ago.
fire beforefeaturemodified, featuremodified, and afterfeaturemodified

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by ahocevar

comment:1 follow-up: Changed 7 years ago by tschaub

What's the reason for coupling the FeatureStore to the layer's selectedFeatures array?

comment:2 in reply to: ↑ 1 Changed 7 years ago by ahocevar

Replying to tschaub:

What's the reason for coupling the FeatureStore to the layer's selectedFeatures array?

At first I thought it would be good to have a replacement for the case where feature modification is cancelled by returning false from a beforefeaturemodified event. But not that you ask, I'm inclined to say that there is no reason. New patch to come.

Changed 7 years ago by ahocevar

comment:3 Changed 7 years ago by tschaub

Ok, I see you're taking very literally the word "selected" in the OL docs. I'd consider it to mean that the beforefeaturemodified event is triggered when a feature is about to be modified.

If there are duplicate beforefeaturemodified events being triggered, we should consider the case where that comes up. Looking just at this code, I think it is consistent to fire this event before modifying the feature.

The patch above only redraws the feature if it was selected. I imagine the purpose for redrawing the feature is to apply a new symbolizer in cases where a new attribute would make a different rule apply. This isn't limited to selected features in any way.

It also looks like the patch above would make it so the features attributes aren't updated when the corresponding record's fields are modified - unless the feature is selected. This sounds like unnecessary coupling of record editing and feature selection to me (but I could be misreading).

comment:4 Changed 7 years ago by tschaub

My comments about "the patch above" were only relevant to the first patch.

What's the case where beforefeatureselected is fired twice? It looks like that's what you're trying to address here.

Changed 7 years ago by tschaub

fire beforefeaturemodified, featuremodified, and afterfeaturemodified

comment:5 Changed 7 years ago by tschaub

Perhaps not the issue you're trying to address, but it looks like we should be firing afterfeaturemodified as well.

comment:6 Changed 7 years ago by ahocevar

Let's say you have a vector layer with a ModifyFeature control. You select a feature with it, and beforefeaturemodified is triggered. Now you also have a grid where you edit feature attributes. As soon as a feature attribute is modified, beforefeaturemodified is fired again.

comment:7 Changed 7 years ago by tschaub

Yeah, I see that. There are two things that can modify the feature. Both are responsible for letting listeners know the feature is about to be modified. I think it's inconsistent to not have the FeatureStore fire the same events (and I think it should fire afterfeaturemodified). There are likely applications that depend on the current behavior (having the store fire beforefeaturemodified). Seems a bit brash just to remove it.

While it's clunky, if an application cannot tolerate having a beforefeaturemodified listener called twice, it should register an idempotent function instead. It shouldn't be hard on the application level to keep track of how many times the method has been called (e.g. if (!alreadyRegistered) {...}).

comment:8 Changed 6 years ago by ahocevar

  • Milestone changed from 1.1 to 1.2

Batch move of tickets to finish the 1.1 milestone.

Note: See TracTickets for help on using tickets.