Opened 8 years ago

Last modified 6 years ago

#141 new enhancement

don't remove features from layer that have delete state

Reported by: tschaub Owned by:
Priority: major Milestone: 1.2
Component: GeoExt.data.FeatureStore Version: trunk
Keywords: Cc:
State: Review

Description

In Ext 3.0, when you call store.remove(record), the record gets stored in a store.removed array. This is somewhat analogous to us setting the feature state to delete (and typically not rendering it on a map).

When store.save is called, the proxy.doRequest method is called with the "delete" action with records in the store.removed array. If the feature store doesn't remove features from the layer with delete state, then the request callbacks can do the right thing: remove the feature from the layer and from the store.removed array.

An alternative would be to maintain a layer.deleted array of features (or something similar).

Attachments (2)

geoext-141.patch (945 bytes) - added by ahocevar 7 years ago.
no tests yet.
geoext-141.2.patch (4.2 KB) - added by ahocevar 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by sbenthall

  • State changed from None to Needs more work

comment:2 Changed 8 years ago by tschaub

  • Milestone changed from 0.6 to 0.7

comment:3 Changed 8 years ago by tschaub

  • Milestone changed from 0.7 to 1.0

comment:4 Changed 7 years ago by ahocevar

  • Milestone changed from 1.0 to 1.1

No patch. Bumping.

Changed 7 years ago by ahocevar

no tests yet.

comment:5 Changed 7 years ago by tschaub

I might be misreading, but with this patch, it still looks like properly deleting a feature would involve this:

    record.getFeature().state = OpenLayers.State.DELETE;
    store.remove(record);
    store.save();

Ideally, a test would demonstrate that this works:

    store.remove(record);
    store.save();

(without something else setting the feature state first)

comment:6 Changed 7 years ago by ahocevar

It seems the ticket summary is a bit misleading, and different from the description. My patch only addressed what's said in the summary - don't remove features with delete state from the bound vector layer.

comment:7 Changed 7 years ago by ahocevar

If you want a feature to be deleted in a transaction by saying {{{ store.remove(record); store.save(); }}} we should properly handle the store.removed array, for example by setting the delete state in the save() method, and re-adding the features we find there to the layer. Changing store.remove(record); so that it does not remove the feature from the layer feels like a significant behavioral change to me.

Changed 7 years ago by ahocevar

comment:8 Changed 7 years ago by ahocevar

  • State changed from Needs more work to Review

Ok, here is the strategy: the only time we need a feature with delete state is when the OpenLayers.Protocol creates the transaction. The attached patch does this by re-adding features from the store's removed array to the layer, with a delete state, and removing them again after the transaction is created.

Patch with tests, tests pass in Safari5. Thanks for any review.

comment:9 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.