RFR: 8190876: javadoc search on "java.se" shows "java.se" the last one among other modules

Wed Jun 27 20:50:49 UTC 2018

If you just add a comment explaining the secondary comparison, you don't 
need another round of review.

-- Jon

On 6/27/18 1:49 PM, Jonathan Gibbons wrote:
> OK,
>
> Please add a comment to that effect in the code, so we don't lose that 
> info.
>
> -- Jon
>
>
> On 6/27/18 1:42 PM, Hannes Wallnöfer wrote:
>> Am 27.06.2018 um 22:12 schrieb Jonathan Gibbons 
>> <jonathan.gibbons at oracle.com>:
>>> Utils.java
>>>
>>> The secondary comparisons, on lines 2093, 2113 still use 
>>> String.compareTo instead of compareStrings.
>>>
>> The reason is that TreeSet requires the Comparator to be consistent 
>> with equals, i.e. if compare returns 0 for any two of them, TreeSet 
>> will consider them as equal and discard one of them.
>>
>> So the reason for the secondary comparison is not just to have 
>> consistent order, but (maybe more importantly) avoid losing items 
>> that happen to have the same label or name.
>>
>> Also, the toString representation is not what is displayed to the 
>> user, so using collation for ordering may not have the expected result.
>>
>> Hannes
>>
>>
>>> -- Jon
>>>
>>> On 06/27/2018 11:36 AM, Hannes Wallnöfer wrote:
>>>> Here’s a new webrev with collator applied to all search results 
>>>> (search tags now use generic comparator, specific search tag 
>>>> comparator is removed). The search tag test has been updated to 
>>>> expect the new order.
>>>>
>>>> http://cr.openjdk.java.net/~hannesw/8190876/webrev.03/
>>>>
>>>> Hannes
>>>>
>>>>> Am 27.06.2018 um 19:20 schrieb Jonathan Gibbons 
>>>>> <jonathan.gibbons at oracle.com>:
>>>>>
>>>>> This seems like flawed reasoning.  We should not have an 
>>>>> inconsistent policy just because it affects a particular use case 
>>>>> and/or test.
>>>>>
>>>>> Either we believe in using collation for sorting the index 
>>>>> entries, or we don't.
>>>>>
>>>>> -- Jon
>>>>>
>>>>>
>>>>> On 6/27/18 9:24 AM, Hannes Wallnöfer wrote:
>>>>>> When searching for „jav“, Search Tags contains „Java Collections 
>>>>>> Framework“ followed by various java* command line tools. When 
>>>>>> using collation, the first item is „javac“, followed by „Java 
>>>>>> Collections Framework“ and then the remaining java* tools.
>>>>>>
>>>>>> It just looks strange and it causes tests to fail, so I opted for 
>>>>>> keeping search tag order as it is.
>>>>>>
>>>>>> Hannes
>>>>>>
>>>>>>> Am 27.06.2018 um 18:05 schrieb Jonathan Gibbons 
>>>>>>> <jonathan.gibbons at oracle.com>:
>>>>>>>
>>>>>>> Hannes,
>>>>>>>
>>>>>>> Can you explain more what you mean by ...
>>>>>>>
>>>>>>> "except search tags, where collation causes frameworks and 
>>>>>>> commands to be mixed up and tests to fail."
>>>>>>>
>>>>>>> -- Jon
>>>>>>>
>>>>>>>
>>>>>>> On 6/27/18 8:57 AM, Hannes Wallnöfer wrote:
>>>>>>>> Updated webrev with comparators moved to Utils. All index items 
>>>>>>>> use collation for primary comparison except search tags, where 
>>>>>>>> collation causes frameworks and commands to be mixed up and 
>>>>>>>> tests to fail. Comparators for index collections are created in 
>>>>>>>> HtmlConfiguration.initConfiguration, I think Locale should be 
>>>>>>>> set by then.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~hannesw/8190876/webrev.02/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Hannes
>>>>>>>>
>>>>>>>>
>>>>>>>>> Am 22.06.2018 um 23:22 schrieb Jonathan Gibbons 
>>>>>>>>> <jonathan.gibbons at oracle.com>:
>>>>>>>>>
>>>>>>>>> While I agree that Utils is not a great place for the 
>>>>>>>>> comparators, I will note that the comparison routines there 
>>>>>>>>> are better than the "simple" ones you have placed in 
>>>>>>>>> SearchIndexItem. In particular, the existing comparison code 
>>>>>>>>> in Utils uses java.text.Collator to provide locale-sensitive 
>>>>>>>>> comparison, which is conceptually more correct than your 
>>>>>>>>> simple comparison of the upper-cased strings.
>>>>>>>>>
>>>>>>>>> -- Jon
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 06/19/2018 03:03 AM, Hannes Wallnöfer wrote:
>>>>>>>>>> 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 
>>>>>>>>>>> <jonathan.gibbons at oracle.com>:
>>>>>>>>>>>
>>>>>>>>>>> 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
>