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