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
<[email protected]>:
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
<[email protected]>:
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
<[email protected]>:
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
<[email protected]>:
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
<[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
