Community
Participate
Working Groups
We should consider adding the ascending check box to sorting order menu in the repo view.
For consistency, we should consider adding the same option to the Change Set model in the Synchronize view.
(In reply to comment #1) > For consistency, we should consider adding the same option to the Change Set > model in the Synchronize view. I reported bug 197944 to address this.
Created attachment 74806 [details] Patch Szymon, could you take a look at the patch?
Created attachment 74807 [details] mylyn/context/zip
Szymon, please have a look at the attached patch.
Created attachment 74945 [details] Patch v2 I made some changes in the patch as we discussed: * renamed switchOrder to getReversedComparator in RepositoryComparator class * redid part of RepositoryComparator#compare method for RepositoryRoot -- the switch/case statement. It works the same way, but I hope that the code looks cleaner now
I have one question. What's that advantage of encoding the the ascending/descending flag and the order flag in the same int? Given that there is never more than a few comparators around at a time, I don't know if the space savings is worth the complication this introduces in the code (i.e. it always takes a while to digest the math involved when reading the code). If you feel strongly about encoding the order and direction in the same flag, I would like to see it encapsulated in getters and setters for the two flags and not in the method that uses the flags (i.e. not in the compare method). The readability of the code can also be increased by using bit flags and masks instead of the sign (although this is more of an issue if want to include additional flags in the future).
Created attachment 75028 [details] Patch v3 I've introduced a separate flag for the sorting order, as a consequence of that I hope the code gained on readability. I've also made some code formating, added getters and setters. Please let me know if you still think it's not enough or find any other kludges.
The only thing that can be improved is to use one instance of comparator instead three (or more). Instead creating new instance, just set new parameters on the existing one and send the event that parameter (order or ascending/descending) was changed.
Also, it looks like you used a method on boolean that is not in Java 1.4 (parseBoolean). Until we are given the OK to move to 1.5, we need to make sure we stick to valid 1.4 syntax and class libraries. I changed the code to use valid 1.4 methods and I have released the patch to HEAD. Please verify that the change I made still has the expected behavior (i.e. options preserved over restart). Also, I agree with Szymon that having three instances of the comparator around is a bit wasteful and unnecessary. However, that code pre-dates the patch so I didn;t feel we needed to wait before releasing this patch. Szymon, could you log a separate bug report for that?
Verified in build I20070807-0010
