RFR: 8361366: Allow sorting of member details in lexicographical order [v4]

Hannes Wallnöfer hannesw at openjdk.org
Thu Sep 11 10:51:13 UTC 2025


On Tue, 9 Sep 2025 09:19:41 GMT, Nizar Benalla <nbenalla at openjdk.org> wrote:

>> Please review this patch to add a toggle to order the member details in the table of contents in lexical order. The selected choice is stored and preserved.
>> 
>> Here is a preview of the new toggle.
>> 
>> 
>> https://github.com/user-attachments/assets/55c81e4b-5fc0-416e-8946-53aede419640
>
> Nizar Benalla has updated the pull request incrementally with one additional commit since the last revision:
> 
>   member details should also be sorted

This looks very good. I have added a few minor comments and questions inline. 

One issue I noticed is that it also sorts section headings in the main description of classes in the TOC, which it shouldn't do. One class where you can see this is `java.lang.Thread`. So I guess sorting needs to be limited to details sections. 

I still need a bit more time to review the JS sorting code, but I thought I shouldn't let you wait for the other feedback.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TableOfContents.java line 118:

> 116:             header.add(Entity.NO_BREAK_SPACE)
> 117:                     .add(HtmlTree.BUTTON(HtmlStyles.tocSortToggle)
> 118:                             .put(HtmlAttr.ID, HtmlIds.TOC_ORDER_TOGGLE)

Should use `setId(HtmlId)`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template line 26:

> 24: 
> 25: const sortLexicalLabel = "Sort lexicographically";
> 26: const sortSourceLabel  = "Sort by source order";

These should be localized messages as the ones in the preceding lines.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template line 653:

> 651:     function textForLi(li) {
> 652:         var a = li.querySelector(":scope > a");
> 653:         return (a ? a.textContent : li.textContent).trim();

Why is this needed? Isn't there always a link?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template line 766:

> 764:             var img = btn.querySelector("img");
> 765:             if (img) img.alt = next;
> 766:         });

It would be nice to have a visual clue for the state of the button. For example, if lexicographic order is applied, the button could have a darker (or lighter) background color.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/toggle.svg line 1:

> 1: <?xml version="1.0" encoding="UTF-8"?>

I think the icon works well, but the black is a bit striking. It should use a dark gray color for the light theme (via stroke attribute). For example, the copy-to-clipboard and link icons use `#505050` and `#404040`. This should result in a more off-white for the dark theme.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocPaths.java line 119:

> 117: 
> 118:     /** The name of the table of contents toggle icon file. */
> 119:     public static final DocPath TOGGLE_SVG = DocPath.create("toggle.svg");

I think "toggle" is not a good description for the icon. Something "sort" or "a-z" would describe it better.

-------------

PR Review: https://git.openjdk.org/jdk/pull/26322#pullrequestreview-3210375366
PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2339893967
PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2339984402
PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2340083835
PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2340023404
PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2340036014
PR Review Comment: https://git.openjdk.org/jdk/pull/26322#discussion_r2339948382


More information about the javadoc-dev mailing list