On Wed, 1 Jun 2022 14:55:57 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
>> The original purpose of this issue was to fix the problems of elements on >> the deprecated API list that can't be deselected because their release is >> not included in the list of checkboxes at the top of the page. This is fixed >> here by adding a checkbox for all elements/releases not included in the list >> if necessary. >> >> While working on it I discovered a few corner cases that produce undesirable >> output for the deprecated list. This includes the case of a single release >> (where a checkbox doesn't make any sense) and the case of some or all >> releases not having any occurrences in the `since` element of deprecated >> elements (in which case the checkbox is useless as well). A more detailed >> description of these cases is available in the [JBS >> issue](https://bugs.openjdk.java.net/browse/JDK-8287524). >> >> As examples for the output generated with this change I uploaded JDK API >> docs: >> >> http://cr.openjdk.java.net/~hannesw/8287524/api.00/deprecated-list.html >> >> To illustrate behaviour in corner cases mentioned above I also uploaded >> deprecated pages generated by the `TestNewAPIListWriter` test (which despite >> its name also tests the "extended" deprecated list). >> >> http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-multi/deprecated-list.html >> http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-none/deprecated-list.html >> http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-package/deprecated-list.html >> http://cr.openjdk.java.net/~hannesw/8287524/test.00/TestNewApiList/out-single/deprecated-list.html > > Hannes Wallnöfer has updated the pull request incrementally with one > additional commit since the last revision: > > Add comment Some questions inline. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/DeprecatedListWriter.java line 88: > 86: for (int i = 0; i < releases.size(); i++) { > 87: String release = releases.get(i); > 88: String releaseIndex = release.isEmpty() ? "" : > Integer.toString(i + 1); See following comments about the questionable? use of an empty string src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/DeprecatedListWriter.java line 92: > 90: ? > contents.getContent("doclet.Deprecated_API_Checkbox_Other_Releases") > 91: : Text.of(release); > 92: HtmlId htmlId = HtmlId.of("release-" + releaseIndex); Note that `releaseIndex` may be an empty string. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/DeprecatedListWriter.java line 97: > 95: .put(HtmlAttr.CHECKED, "") > 96: .put(HtmlAttr.ONCLICK, > 97: "toggleGlobal(this, '" + > releaseIndex + "', 3)")) Note that `releaseIndex` may be an empty string src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SummaryListWriter.java line 136: > 134: bodyContents.addMainContent(content); > 135: // The script below enables checkboxes in the page and invokes > their click handler > 136: // to restore any previous state, which unfortunately doesn't > work on all browsers. This comment looks worrying. It might be worth identifying good or bad browsers, either here or in public-facing documentation. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DeprecatedAPIListBuilder.java line 63: > 61: if (!releases.containsAll(foundReleases)) { > 62: // Empty string is added for other releases, including > the default value "" > 63: releases.add(""); Is this instance of an empty string related to preceding uses of empty string, noted earlier in these review comments? ------------- PR: https://git.openjdk.java.net/jdk/pull/8973
