Ok, I propose applying Ben's patch for the moment since simple feature type is pretty clearly broken. And make the clean break from the super class a separate issue.
Jody Garnett wrote: > Hi Ben sorry to be slow to respond to this one; I am busy prepping for a > meeting with Simone. I do have your patch applied but have not been able > to review it carefully yet. > > Justin you are correct about the duplication being an issue; I am still > keen to make a SimpleFeatureType / SimpleFeature implementation that is > lean and does not extend another class. > > Jody > > On Fri, Feb 27, 2009 at 5:05 PM, Justin Deoliveira <[email protected] > <mailto:[email protected]>> wrote: > > Hi Ben, > > > Ben Caradoc-Davies wrote: > > Justin, I have no objection. It should do no harm. However, note > that: > > (1) order should never be important, except for special case > subclasses such as SimpleFeatureTypeImpl (see below), and > > I guess the contract does not specify any order... but it just seems > like a good thing to do implementation wise. Unless there is a > strong case for changing the order... since types are immutable i > can't think of one. > > > (2) LinkedHashMap conforms to the Map contract, which guarantees > that order is *not* significant for equals/hashCode. Fixing > iteration order does not fix this. > > Good point, something i missed when reading the javadoc of > LinkedHashMap. > > > I experimented with using LinkedHashMap in ComplexTypeImpl to > fix SimpleFeatureTypeImpl iteration order, but when I realised > it did not fix equals/hashCode, I made all my changes to > SimpleFeatureTypeImpl. If you support my position, please join > me in nagging Jody to get my patch accepted. Please take a look > at it; the patch comes with unit tests for iteration order > consistency: > http://jira.codehaus.org/browse/GEOT-2338 > > More importantly, why do you want to change the order of > properties? Do you have a non-simple use case in which they > matter? If not, please consider my patch. > > Indeed this is the exact case addressed by the patch, calling > getDescriptors() and it returning descriptors in a different order. > So I am +1 on fixing this at the simple feature type level if the > current behavior of complex feature type is deemed necessary. The > patch looks good however the duplication of storage of descriptors > for simple feature type seems wrong... perhaps SimpleFetaureTypeImpl > should not extend ComplexFeatureTypeImpl. > > > Kind regards, > Ben. > > > Justin Deoliveira wrote: > > Sorry, i did not mean tree map, just a map with predictable > iteration order. LinkedHashMap should do the trick. > > Justin Deoliveira wrote: > > Hi all, > > I think Ben may have brought this up recently, but > looking at ComplexTypeImpl it seems property values are > stored in a hash map, which totally throws away the > order in which property descriptors are added to it. > > Any objection to making this a tree map as to maintain > order? > > > > > > -- > Justin Deoliveira > OpenGeo - http://opengeo.org > Enterprise support for open source geospatial. > > -- Justin Deoliveira OpenGeo - http://opengeo.org Enterprise support for open source geospatial. ------------------------------------------------------------------------------ Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA -OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise -Strategies to boost innovation and cut costs with open source participation -Receive a $600 discount off the registration fee with the source code: SFAD http://p.sf.net/sfu/XcvMzF8H _______________________________________________ Geotools-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geotools-devel
