From 0741cd3289ccc89777449711ab20d6c32711f494 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hannes=20Walln=C3=B6fer?= Date: Thu, 6 Jul 2023 07:08:20 +0000 Subject: [PATCH] 8311264: JavaDoc index comparator is not transitive Reviewed-by: jjg --- .../doclets/toolkit/util/Comparators.java | 27 +++++++----- .../doclets/toolkit/util/IndexBuilder.java | 41 ++++++++++++------ .../javadoc/doclet/testIndex/TestIndex.java | 43 ++++++++++++++----- .../jdk/javadoc/doclet/testIndex/pkg/C.java | 42 ++++++++++++++++-- 4 files changed, 116 insertions(+), 37 deletions(-) diff --git a/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Comparators.java b/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Comparators.java index 02f16009391..c715dda9e31 100644 --- a/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Comparators.java +++ b/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Comparators.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -238,17 +238,8 @@ public class Comparators { */ @Override public int compare(Element e1, Element e2) { - int result; // first, compare names as appropriate - if ((utils.isModule(e1) || utils.isPackage(e1)) && (utils.isModule(e2) || utils.isPackage(e2))) { - result = compareFullyQualifiedNames(e1, e2); - } else if (utils.isModule(e1) || utils.isPackage(e1)) { - result = utils.compareStrings(utils.getFullyQualifiedName(e1), utils.getSimpleName(e2)); - } else if (utils.isModule(e2) || utils.isPackage(e2)) { - result = utils.compareStrings(utils.getSimpleName(e1), utils.getFullyQualifiedName(e2)); - } else { - result = compareNames(e1, e2); - } + int result = utils.compareStrings(getIndexElementKey(e1), getIndexElementKey(e2)); if (result != 0) { return result; } @@ -274,6 +265,20 @@ public class Comparators { return indexUseComparator; } + /** + * {@return the element's primary key for use in the index comparator} + * This method can be used by other comparators which need to produce results + * that are consistent with the index comparator. + * + * @param element an element + */ + public String getIndexElementKey(Element element) { + return switch (element.getKind()) { + case MODULE, PACKAGE -> utils.getFullyQualifiedName(element); + default -> utils.getSimpleName(element); + }; + } + private Comparator typeMirrorClassUseComparator = null; /** diff --git a/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/IndexBuilder.java b/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/IndexBuilder.java index ea7ffe61aaa..e3c88641b3b 100644 --- a/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/IndexBuilder.java +++ b/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/IndexBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 1998, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -119,7 +119,7 @@ public class IndexBuilder { itemsByFirstChar = new TreeMap<>(); itemsByCategory = new EnumMap<>(IndexItem.Category.class); - mainComparator = makeIndexComparator(classesOnly); + mainComparator = classesOnly ? makeClassComparator() : makeIndexComparator(); } /** @@ -310,6 +310,13 @@ public class IndexBuilder { return '*'; } + /** + * Returns a comparator for the all-classes list. + * @return a comparator for class element items + */ + private Comparator makeClassComparator() { + return Comparator.comparing(IndexItem::getElement, utils.comparators.makeAllClassesComparator()); + } /** * Returns a comparator for the {@code IndexItem}s in the index page. @@ -318,15 +325,17 @@ public class IndexBuilder { * * @return a comparator for index page items */ - private Comparator makeIndexComparator(boolean classesOnly) { - Comparator elementComparator = classesOnly - ? utils.comparators.makeAllClassesComparator() - : utils.comparators.makeIndexElementComparator(); - - Comparator labelComparator = - (ii1, ii2) -> utils.compareStrings(ii1.getLabel(), ii2.getLabel()); + private Comparator makeIndexComparator() { + // We create comparators specific to element and search tag items, and a + // base comparator used to compare between the two kinds of items. + // In order to produce consistent results, it is important that the base comparator + // uses the same primary sort keys as both the element and search tag comparators + // (see JDK-8311264). + Comparator elementComparator = utils.comparators.makeIndexElementComparator(); + Comparator baseComparator = + (ii1, ii2) -> utils.compareStrings(getIndexItemKey(ii1), getIndexItemKey(ii2)); Comparator searchTagComparator = - labelComparator + baseComparator .thenComparing(IndexItem::getHolder) .thenComparing(IndexItem::getDescription) .thenComparing(IndexItem::getUrl); @@ -350,9 +359,9 @@ public class IndexBuilder { return d; } - // If one is an element item, compare labels; if equal, put element item last + // If one is an element item, compare item keys; if equal, put element item last if (ii1.isElementItem() || ii2.isElementItem()) { - int d = labelComparator.compare(ii1, ii2); + int d = baseComparator.compare(ii1, ii2); return d != 0 ? d : ii1.isElementItem() ? 1 : -1; } @@ -361,6 +370,14 @@ public class IndexBuilder { }; } + private String getIndexItemKey(IndexItem ii) { + // For element items return the key used by the element comparator; + // for search tag items return the item's label. + return ii.isElementItem() + ? utils.comparators.getIndexElementKey(ii.getElement()) + : ii.getLabel(); + } + /** * Returns a Comparator for IndexItems in the types category of the search index. * Items are compared by short name, falling back to the main comparator if names are equal. diff --git a/test/langtools/jdk/javadoc/doclet/testIndex/TestIndex.java b/test/langtools/jdk/javadoc/doclet/testIndex/TestIndex.java index 5bc94fa63b4..cd3fd43d666 100644 --- a/test/langtools/jdk/javadoc/doclet/testIndex/TestIndex.java +++ b/test/langtools/jdk/javadoc/doclet/testIndex/TestIndex.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2003, 2022, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2003, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -24,6 +24,7 @@ /* * @test * @bug 4852280 4517115 4973608 4994589 8026567 8071982 8196202 8234746 + * 8311264 * @summary Perform tests on index.html file. * Also test that index-all.html has the appropriate output. * Test for unnamed package in index. @@ -50,22 +51,44 @@ public class TestIndex extends JavadocTester { checkExit(Exit.OK); //Test index-all.html - checkOutput("index-all.html", true, - """ - C - Class i\ - n pkg""", - """ - Int\ - erface - Interface in pkg""", + checkOrder("index-all.html", """ AnnotationType - Annotation Interface in pkg""", + """ + c() - Method in class pkg.\ + C""", + """ + C - Search tag in cla\ + ss pkg.C""", + """ + C - Class i\ + n pkg""", + """ + C() - Constructor\ + for class pkg.C""", + """ + C(int) - Const\ + ructor for class pkg.C""", + """ + c_() - Method in class pk\ + g.C""", + """ + c/d - Search tag in class p\ + kg.C""", + """ + C/D - Search tag in\ + class pkg.C""", """ Coin - Enum Class in pkg""", """ - Class in Unnamed Package""", +
Enum - Search tag i\ + n enum class pkg.Coin
""", + """ + Int\ + erface - Interface in pkg""", """
Java - Static variabl\ @@ -76,6 +99,6 @@ public class TestIndex extends JavadocTester {
 
""", """ -
Enum - Search tag in enum class pkg.Coin
"""); + Class in Unnamed Package"""); } } diff --git a/test/langtools/jdk/javadoc/doclet/testIndex/pkg/C.java b/test/langtools/jdk/javadoc/doclet/testIndex/pkg/C.java index de02c8b23e8..6d6cd5c3d50 100644 --- a/test/langtools/jdk/javadoc/doclet/testIndex/pkg/C.java +++ b/test/langtools/jdk/javadoc/doclet/testIndex/pkg/C.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2004, 2023, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -23,11 +23,45 @@ package pkg; +/** + * A class to test sorting of index items. + * + *

C

+ * + * Section "C" should appear right before language elements with the same name. + * + *

C/D

+ * + * Section "C/D" should appear after items named "C" in the index. + * + * {@index c/d should appear before the section above} + */ public class C { - //Test that Java appears before JDK in the index. The fact - //that 'D' is uppercase and 'a' is lowercase should make no difference - //in ordering. + /** + * Empty constructor. + */ + public C() {} + + /** + * Constructor with a parameter. + * @param i an int + */ + public C(int i) {} + + /** + * Lower case "c" method should appear before upper case "C" elements and sections in index. + */ + public void c() {} + + /** + * Should appear after all items named "c" or "C". + */ + public void c_() {} + + // Test that Java appears before JDK in the index. The fact + // that 'D' is uppercase and 'a' is lowercase should make no difference + // in ordering. public static final String JDK = "1.5"; public static final String Java = "1.5";