Community
Participate
Working Groups
The API added in 3.2 to allow JFace viewers to compare strings using arbitrary Comparators, not just Java Collators, is very badly designed. A kludge had to be inserted somewhere between StructuredViewer and the initialization of the ViewerSorter with a Java collator, so ViewerSorter was copied and pasted as ViewerComparator and most of the members were pushed up. I think it would be much better to allow a ViewerSorter to have an arbitrary comparator, defaulting to the one from the JFace Policy (which in Eclipse is an ICU collator because they're better than Java ones). My reasoning: - ViewerComparator is a bad name as it does not suggest how that class differs from ViewerSorter (i.e., arbitrary comparator vs. Java collator). It would be much less confusing to have only one class. - All existing clients that call StructuredViewer.getSorter() will break on a viewer whose comparator was configured using the new API (i.e. ViewerComparator), even if the ViewerComparator actually contains a Java collator. If instead getSorter() returned a ViewerSorter whose getCollator() returns null, those clients would only break if they try to use the collator. Furthermore, it might be possible to implement getCollator() so that if the comparator happens to be an ICU collator, it is wrapped in a proxy that looks like a Java collator. Then clients would only break if they ask for a Java collator from a ViewerSorter that isn't using either a Java or an ICU collator, which probably will never happen. - A lot of existing code says "viewer.setSorter(new ViewerSorter());". Under the current API, that will result in a sort using the default Java collator. It would be much easier to make "new ViewerSorter()" return a sorter that uses the default ICU collator than to change all calls to "viewer.setSorter(new ViewerSorter())" into "viewer.setComparator(new ViewerComparator())". Furthermore, there are only two references to ViewerComparator outside JFace itself, one of which was introduced only yesterday in response to bug 153463. Thus, I don't think anyone will complain if we back out the API from 3.2 and do it a better way. If you like this proposal, I would be happy to implement it myself and post the patch.
see comment 8 in bug 153463 for design reasoning.
If it's essential that ViewerSorter.getCollator() never return null, it could use the same kludge as the following early adopters of ICU: org.eclipse.jdt.internal.ui.search .JavaSearchResultPage.DecoratorIgnoringViewerSorter org.eclipse.jdt.ui.JavaElementSorter org.eclipse.search.internal.ui.text.FileSearchPage Namely, return java.text.Collator.getInstance() even though that isn't actually the comparator being used.
(In reply to comment #0) > Furthermore, there are only two references to ViewerComparator outside JFace > itself, one of which was introduced only yesterday in response to bug 153463. > Thus, I don't think anyone will complain if we back out the API from 3.2 and do > it a better way. This is not an option. We cannot break binary API compatibility.
Boris of course has made the strongest point but to add: We considered that option in 3.2 but it didn't make sense for a series of reasons 1) Many sorters make no use of a Collator so having one that had no dependency was quite useful 2) Forcing the use of a Collator when it was never used was just bad style - especially if someone made the mistake of thinking the Collator for the sorter was used in the sorting and made assumptions about it 3) Creation of Collators can be slow in both icu and java.util. If an RCP app wants to avoid the potential of creating on on startup for instance having no reference to collators is a good way to do it.
(In reply to comment #0) > - All existing clients that call StructuredViewer.getSorter() will break on a > viewer whose comparator was configured using the new API (i.e. > ViewerComparator), even if the ViewerComparator actually contains a Java > collator. Yes. Clients can only move to the new story if they don't pass the viewer instance to existing code which is not aware of getComparator(). Where is this a problem?
I don't understand #1-3, but I see that nothing major can be done given an absolute requirement of binary compatibility. Nonetheless, StructuredViewer.getSorter() should probably be deprecated, and perhaps StructuredViewer.setSorter() and ViewerSorter should also be deprecated. In fact, the javadoc for ViewerSorter says "It is recommended to use ViewerComparator instead"; that could be turned into a real deprecation.
Is there anything that prevents us from deprecating in 3.3?
I think we decided not to deprecate in 3.2 because the changes were done post-API freeze (with approval, of course). So I can't think of a reason why they should not be deprecated.
Waiting for some action on bug 154884 before I deprecate.
I think TreePathViewerSorter should also be deprecated. A class named TreePathViewerComparator should be introduced that subclasses ViewerComparator. Also, Martin suggested that the ViewerSorter class not be deprecated as it will introduce alot of warnings that people likely aren't going to fix. Instead we deprecated the getCollator method of ViewerSorter. See bug 158118 for more details.
I think this bug should be marked WONTFIX in light of bug 158118 comment 6.
