Opened 9 years ago

Closed 9 years ago

#18 closed enhancement (fixed)

add FeatureStore, a helper class for creating stores pre-configured with a FeatureReader

Reported by: elemoine Owned by:
Priority: major Milestone: 0.5
Component: GeoExt.data Version:
Keywords: Cc:
State: Review

Description

This ticket suggests adding a helper class to ease creating stores of features. An instance of this class is pre-configured with a feature reader.

Attachments (7)

patch-18-r171-A0.diff (10.4 KB) - added by elemoine 9 years ago.
patch-18-r371-B0.diff (40.3 KB) - added by bbinet 9 years ago.
patch-18-r371-B1.diff (40.4 KB) - added by bbinet 9 years ago.
patch-18-r371-C0.diff (45.8 KB) - added by elemoine 9 years ago.
patch-18-r371-D0.diff (48.0 KB) - added by elemoine 9 years ago.
patch-18-r371-D1.diff (48.7 KB) - added by elemoine 9 years ago.
patch-18-r371-D2.diff (49.0 KB) - added by elemoine 9 years ago.

Download all attachments as: .zip

Change History (19)

Changed 9 years ago by elemoine

comment:1 Changed 9 years ago by ahocevar

This is some duplicate of #12. sbenthall, elemoine: please agree on which ticket to close.

comment:2 Changed 9 years ago by elemoine

  • State changed from Review to Needs more work

Changed 9 years ago by bbinet

comment:3 Changed 9 years ago by bbinet

  • State changed from Needs more work to Review

The new patch patch-18-r371-B0.diff attached to this ticket provides the following:

  • Add FeatureStoreMixin class and FeatureStore helper class, the same way it is done for LayerStoreMixin/LayerStore. Events "load", "clear", "add", "remove" and "featureadded", "featureremoved" are supported by FeatureStoreMixin.
  • Option "layers" is an alias of option "data".
  • Add test cases for FeatureStore.
  • Remove LayerStoreMediator and FeatureStoreMediator classes which are no longer needed.

comment:4 Changed 9 years ago by elemoine

Thanks Bruno for the patch, this is looking really good to me. It'd be great if Andreas and/or Sebastian, who participated to the discussions on the FeatureStore on the mailing list, could look at it.

One feature we had in the mediator that is not in your patch is filtering. Ext.data.Store doesn't provide a beforeadd event, so I'd like that we add this to our mixin. If you have no time for this, I'll arrange time to work on it.

comment:5 Changed 9 years ago by bbinet

Pierre Giraud also points out to me that this FeatureStore had some performance issues:

This was due to FeatureStoreMixin that was listening to "featureadded" and "featureremoved" events from the layer vector, and for each feature added, the method onFeatureAdded was called.
You can imagine that the processing time was growing as much as the number of features added.

I have fixed the previous patch so that FeatureStoreMixin will listen to "featuresadded" and "featuresremoved". So the method onFeaturesAdded will be called only once after all features have been added to layer.
I will soon post my new patch here, but I discovered a bug in openlayers when creating unit tests for FeatureStore, so I couldn't propose the patch yet.

Do you think I should keep the possibility to listen to "featureadded" and "featureremoved" as an optional config parameter? Does event "featuresadded" be always called after "featureadded" or does it remain some cases that only "featureadded" event is called?

Eric, I agree for filtering feature which is missing here. I think this patch could be committed as it is, since it does not depend on filtering.
We could then add the filtering functionnality when we would have coded it. (I'd prefer many small commits than fewer big ones!) ;)

Changed 9 years ago by bbinet

comment:6 Changed 9 years ago by bbinet

Here is the new patch for better performance: patch-18-r371-B1.diff

Changed 9 years ago by elemoine

comment:7 Changed 9 years ago by elemoine

patch-18-r371-C0.diff handles "featuremodified" (from the layer) and "update" (from the store) events. New tests added. All tests pass on FF3.

Changed 9 years ago by elemoine

comment:8 Changed 9 years ago by elemoine

patch-18-r371-D0.diff adds recordFilter and featureFilter properties. New tests added. All tests pass on FF3.

Changed 9 years ago by elemoine

comment:9 Changed 9 years ago by elemoine

patch-18-r371-D1.diff adds comments, and reorganizes the code a bit to place the this._adding = true and delete this._adding and friends around the relevant statements. Tests continue to pass.

Bruno, tell me if this patch is ok to you.

Changed 9 years ago by elemoine

comment:10 Changed 9 years ago by elemoine

patch-18-r371-D2.diff adds the following:

  • featureFilter and recordFilter renamed addFeatureFilter and addRecordFilter (should we introduce removeFeatureFilter and removeRecordFilter in the future)
  • avoid looping over the features/records if no filter is specified
  • move code common to onAdd and onLoad in its own method (addFeaturesToLayer)

comment:11 Changed 9 years ago by bbinet

Good job Eric!

I've just read your last patch. Seems ok for me.

comment:12 Changed 9 years ago by elemoine

  • Resolution set to fixed
  • Status changed from new to closed

(In [378]) add FeatureStore, a helper class for creating stores pre-configured with a FeatureReader, p=sbenthall,bbinet,me, r=me (closes #18)

Note: See TracTickets for help on using tickets.