Hello Andrea,
Sorry for my late reponses.
On 01/19/2013 06:54 PM, Andrea Aime wrote:
Here is some more I've found during the review.
There's a change in DefaultCatalogFacade.java that I'm not sure about:
if (null != sortByList) {
for (int i = sortByList.length - 1; i >=0 ; i--) {
SortBy sortBy = sortByList[i];
Ordering<Object> ordering = Ordering.from(comparator(sortBy));
if (SortOrder.DESCENDING.equals(sortBy.getSortOrder())) {
ordering = ordering.reverse();
}
all = ordering.sortedCopy(all);
}
}
What guarantees are there that sorting the same list multiple times
we'll get
to a result that is ordered on sortBy1, then sortBy2, and so on?
What I'm trying to say is, the second time we sort the algorithm might
as well
ruin the partial ordering established by the first sort... the Java 6
algorithm might
work, but Java 7 has a different sorting algorithm (TimSort), not sure
if the
multiple sorts are going to be preserved there.
I see your point. I remember I needed to include support for sorting
with more than one sortby, which wasn't yet supported previously.
But perhaps we have to revise that algorithm and see if there is a more
appropriate solution.
Anyways, no big deal, I guess it can be handled as a bug later, if it
turns
out to misbehave.
What's a bit more troublesome is changes in the CatalogStore interface.
Mind, I'm not against them per se, but in order to change a interface
that was
discussed openly on the ml I would have expected the same opennes, or at
least some explanation on why they happened, instead I'm looking at the
diff trying to understand them.
The changes propagate in other portions as well, it seems bits of the CSW
service code have been refactored, again, without providing any clue
as to why.
I'm not saying the changes are right or wrong (on the contraty, I'm
confident
they have been done for good reasons), but they should have been
discussed: since the patch is out already, can we have some rationale
about them?
It would make reviewing the patch a lot easier.
First my apologies for not explaining the changes in advance. I usually
try to make sure I am certain that particular changes are necessary or
useful before I propose them, perhaps that is why I didn't do it earlier.
Then I also have to apologise about something else, I did my best
cleaning up all the code and filling in the javadoc before I did my pull
request but I seemed to have missed this file. The javadoc for
translateProperty wasn't filled in, and on top of that one of the
changes had to be removed before committing. The extra parameter
elementSet was supposed to be removed again, this was a change I made at
some point but became unnecessary later. So please remove this change
all together, this can be done in the interface and all its
implementations without a problem. Again, sorry about that, that was not
my intention.
The other two changes however were for a good reasons and have both to
do with making CSW more generic.
A lot of the generic stuff relies on using the RecordDescriptor
interface. At points where the old code simply calls
CSWRecordDescriptor.SOME_CONSTANT the new code often uses an instance of
the RecordDescriptor interface and a method to get this information, for
obvious reasons. Another thing is that as a consequence of parsing .xsd
files the name of the record type is not any more in the FeatureType,
but in an AttributeDescriptor. For example, the name of the CSW DC type
is not "Record" but "RecordType", there is a descriptor with the name
"Record" that points to "RecordType". This is good because it
corresponds with how this is coded in the .xsd file. This is just one of
the reasons why I don't pass on FeatureType objects any more, but rather
RecordDescriptor objects. From here all sorts of information can be
retrieved, like a namespace set which is also frequently used.
Considering the translateProperty method, this method converts a Name to
PropertyName. This is used for property selection. This happens
differently with the two Catalog Stores. The Abstract Catalog Store
selects properties from a finished feature, so the PropertyName applies
to a finished feature. The Internal Catalog Store selects properties
earlier in the process, in the mapping, so it needs property names that
the mapping understands (so for example, with '/value' included).
I hope this helps, if there are any more questions, please ask.
Kind Regards
Niels Charlier
------------------------------------------------------------------------------
Master Visual Studio, SharePoint, SQL, ASP.NET, C# 2012, HTML5, CSS,
MVC, Windows 8 Apps, JavaScript and much more. Keep your skills current
with LearnDevNow - 3,200 step-by-step video tutorials by Microsoft
MVPs and experts. ON SALE this month only -- learn more at:
http://p.sf.net/sfu/learnnow-d2d
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
