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