Thanks for the review, Jon. I’ve uploaded a new webrev to address the issues you brought up:
http://cr.openjdk.java.net/~hannesw/8190876/webrev.01/ Moving the index item comparators up to Utils didn’t feel right, so I made them static and moved them to SearchIndexItem.java, which feels more appropriate to me. The generic one that is used multiple times is now a singleton. I also forgot to update SearchTest.java for the changes in search.js file, that is included in the new webrev. Hannes > Am 18.06.2018 um 20:23 schrieb Jonathan Gibbons <[email protected]>: > > Minor issues to address: > > HtmlConfiguration, 397, 406: > You're using .toUpperCase() which depends on the Locale. The convention in > javadoc is to use Utils.to{Lower,Upper}Case(String), which forces the en-US > locale (to avoid the Turkish-i problem). There is an equivalent convention in > javac as well. > > SearchIndexItem > If I understand the code correctly, "NestedName" is not the correct term to > be using. I think you're trying to get the "simple name". Nesting is a > different concept, as in, "nested classes". In a better/future world, > SearchIndexItem should contain an Element (not should not always be only > string based) and once you have an Element, you can easily get the simple > name. > > Style issue: > > Although not wrong, it seems less than ideal to have functions creating and > returning equal instances of the comparators, as compared to having singleton > instances stored as needed. But then, it's also weird to have these search > indexes stored in HtmlConfiguration, as compared to a search-related class. > In addition, Utils has many comparators, so you arguably should not be adding > more comparators here in HtmlConfiguration. (Not that I like the overuse of > the Utils bucket.) > > -- Jon > > > On 06/15/2018 12:43 AM, Hannes Wallnöfer wrote: >> This changes sorting order of packages and modules in the search box from >> last name segment to whole package or module name, respectively. Apart from >> fixing the observed issue that leads to more intuitive listings as package >> and module names are hierarchic by nature. >> >> The sorting order for types, members, and search tags is not changed. >> >> The patch also moves sorting from client side JavaScript to Java, speeding >> up rendering of search results by at over 2x. It also provides the benefit >> of secondary order, so members and types with the same name and signature >> are now ordered by package name, whereas their order was undefined before. >> >> Please review: >> >> Webrev: cr.openjdk.java.net/~hannesw/8190876/webrev.00/ >> Issue: https://bugs.openjdk.java.net/browse/JDK-8190876 >> >> Thanks, >> Hannes >
