From: Justin D. <jde...@op...> - 2009-02-27 16:42:51
|
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 <jde...@op... > <mailto:jde...@op...>> 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. |