Ticket #47 (closed enhancement: fixed)

Opened 11 months ago

Last modified 11 months ago

LayerStore configured with "fields" instead of "recordType"

Reported by: elemoine Assigned to:
Priority: major Milestone: 0.5
Component: GeoExt.data.LayerStore Version:
Keywords: Cc:
State: Commit

Description

Shouldn't we follow Ext (see Ext.data.JsonStore) and configure LayerStore instances with "fields" instead of "recordType"?

Moreover, we have an issue with today's LayerStoreMixin. Configuring a LayerStore with a "recordType" option results in an error in Ext. Use test.diff to reproduce that.

Attachments

test.diff (1.0 kB) - added by elemoine on 04/23/09 10:21:01.
patch-47-r421-A0.diff (3.3 kB) - added by elemoine on 04/23/09 10:22:10.
patch-47-r450-A1.diff (3.2 kB) - added by bbinet on 04/23/09 11:51:23.

Change History

04/23/09 10:21:01 changed by elemoine

  • attachment test.diff added.

04/23/09 10:22:10 changed by elemoine

  • attachment patch-47-r421-A0.diff added.

04/23/09 10:22:57 changed by elemoine

  • state changed from None to Review.

patch-47-r421-A0.diff provides a fix. Tests pass on FF3. Please review.

04/23/09 11:35:28 changed by ahocevar

  • state changed from Review to Commit.

Eric, I agree that providing a fields array is the way to go. I have one concern though. In the ND comment for the fields option, it says: "{Array} ... The value of this option is either a field definition objects as passed to the GeoExt.data.LayerRecord.create function or a {<GeoExt.data.LayerRecord>} constructor created using GeoExt.data.LayerRecord.create.". Is that true? I thought that this *has* to be an array of additional fields, not a Record constructor. Please correct that comment before committing.

04/23/09 11:51:23 changed by bbinet

  • attachment patch-47-r450-A1.diff added.

04/23/09 11:56:46 changed by bbinet

Eric, I also notice that you added "fields" as an APIProperty for LayerStoreMixin class. I think it's not useful since this option is only needed for LayerReader configuration.

I updated your patch (patch-47-r450-A1.diff) so that fields options is deleted after being used in the LayerReader instantiation.

04/23/09 13:28:52 changed by elemoine

  • status changed from new to closed.
  • resolution set to fixed.

(In [451]) LayerStore configured with "fields" instead of "recordType", r=ahocevar,bbinet, p=bbinet,me (closes #47)