Integrated: 8327824: Type annotation placed on incorrect array nesting levels
On Tue, 12 Mar 2024 03:33:38 GMT, Chen Liang wrote: > Please review this patch that fixes placement of type annotations on array > types in Javadoc output. This oversight seems to have existed since JDK 8 but > was never noticed or reported. This pull request has now been integrated. Changeset: 35b00e64 Author:Chen Liang Committer: Pavel Rappo URL: https://git.openjdk.org/jdk/commit/35b00e640b7b3f29796433aee86838ae46167419 Stats: 128 lines in 3 files changed: 103 ins; 1 del; 24 mod 8327824: Type annotation placed on incorrect array nesting levels Reviewed-by: prappo - PR: https://git.openjdk.org/jdk/pull/18211
Re: RFR: 8327824: Type annotation placed on incorrect array nesting levels [v2]
On Tue, 12 Mar 2024 22:06:35 GMT, Chen Liang wrote:
>> Please review this patch that fixes placement of type annotations on array
>> types in Javadoc output. This oversight seems to have existed since JDK 8
>> but was never noticed or reported.
>
> Chen Liang has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains seven commits:
>
> - Check output from body instead of summary, summary changes css class when
> new members are added
> - Add test case for JLS example 10.2-2
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> fix/jd-array-anno-order
> - Fix whitespace
> - Make sure varargs work too
> - Fix bugs, add tests, fix wrong anticipated test results
> - 8327824: Type annotation placed on incorrect array nesting levels
This PR passes our CI and looks good; thanks.
test/langtools/jdk/javadoc/doclet/testTypeAnnotations/typeannos/DeepArrays.java
line 34:
> 32: @ArrA ArrParameterized<@ArrC String @ArrA [] @ArrB []> @ArrC [] @ArrD
> [] manyNested() { return null; }
> 33: void varargs(@ArrA String @ArrB [] @ArrC [] @ArrD ... arg) {}
> 34: int @ArrA [] mixedStyles(int @ArrB [] @ArrA [] arg) @ArrB [] { return
> null; } // JLS example 10.2-2
Took me a second or so to remind myself that this is a valid method declaration
with the return type `int[][]`:
int[] mixedStyles(int[][] arg) []
-
Marked as reviewed by prappo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18211#pullrequestreview-1933913639
PR Review Comment: https://git.openjdk.org/jdk/pull/18211#discussion_r1523088118
Integrated: JDK-8325874: Improve checkbox-based interface in summary pages
On Mon, 11 Mar 2024 13:37:54 GMT, Hannes Wallnöfer wrote: > Please review a change to improve the user experience and implementation for > the checkbox-based interface to selectively display content on API summary > pages: > > - Add a checkbox to toggle (select/unselect) all checkboxes in the page. > - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is > redundant and can be replaced by `!renderTabs`. > - Consolidate the code to generate checkboxes into a new protected > `SummaryListWriter.getCheckbox` method. This pull request has now been integrated. Changeset: 03532450 Author:Hannes Wallnöfer URL: https://git.openjdk.org/jdk/commit/035324503f5e04b53d99573a664fd1367b7ccf30 Stats: 142 lines in 9 files changed: 55 ins; 52 del; 35 mod 8325874: Improve checkbox-based interface in summary pages Reviewed-by: prappo - PR: https://git.openjdk.org/jdk/pull/18194
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages [v2]
> Please review a change to improve the user experience and implementation for > the checkbox-based interface to selectively display content on API summary > pages: > > - Add a checkbox to toggle (select/unselect) all checkboxes in the page. > - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is > redundant and can be replaced by `!renderTabs`. > - Consolidate the code to generate checkboxes into a new protected > `SummaryListWriter.getCheckbox` method. Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision: Review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18194/files - new: https://git.openjdk.org/jdk/pull/18194/files/cb37764d..d7460e8d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18194&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18194&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18194.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18194/head:pull/18194 PR: https://git.openjdk.org/jdk/pull/18194
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Wed, 13 Mar 2024 14:30:01 GMT, Hannes Wallnöfer wrote: > > if you check "all" and then uncheck something else, then "all" will remain > > selected. That is a confusing state. > > I agree, but I think the behavior of the checkbox is as it should be, and > it's the label that is slightly misleading. The intended behavior of the > checkbox is set all other checkboxes to a unified state which can be either > "on" or "off", not to reflect the state of other checkboxes. > > So maybe the correct label would be "toggle all" rather than just "all". > Admittedly I have used the shorter "all" label to limit the length of the > already long line of checkboxes. Do you think using "toggle all" as label > would make things better? No, I don't think that "toggle all" would be better. It's the checkbox that slightly bothers me, not that checkbox's label. One minimalistic, consistent approach that comes to mind is a stateless button that inverts selection. But admittedly, your suggestion is already implemented and is _less nerdy_. Don't bother changing it; it is a nit. - PR Comment: https://git.openjdk.org/jdk/pull/18194#issuecomment-1994584561
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Mon, 11 Mar 2024 18:09:41 GMT, Pavel Rappo wrote: > if you check "all" and then uncheck something else, then "all" will remain > selected. That is a confusing state. I agree, but I think the behavior of the checkbox is as it should be, and it's the label that is slightly misleading. The intended behavior of the checkbox is set all other checkboxes to a unified state which can be either "on" or "off", not to reflect the state of other checkboxes. So maybe the correct label would be "toggle all" rather than just "all". Admittedly I have used the shorter "all" label to limit the length of the already long line of checkboxes. Do you think using "toggle all" as label would make things better? - PR Comment: https://git.openjdk.org/jdk/pull/18194#issuecomment-1994536241
Re: RFR: JDK-8325874: Improve checkbox-based interface in summary pages
On Mon, 11 Mar 2024 13:37:54 GMT, Hannes Wallnöfer wrote:
> Please review a change to improve the user experience and implementation for
> the checkbox-based interface to selectively display content on API summary
> pages:
>
> - Add a checkbox to toggle (select/unselect) all checkboxes in the page.
> - Remove the `Table.setAlwaysShowDefaultTab(boolean)` setter as it is
> redundant and can be replaced by `!renderTabs`.
> - Consolidate the code to generate checkboxes into a new protected
> `SummaryListWriter.getCheckbox` method.
Looks okay.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/NewAPIListWriter.java
line 86:
> 84: Content tabs = HtmlTree.DIV(HtmlStyle.checkboxes,
> 85: contents.getContent("doclet.New_API_Checkbox_Label"));
> 86: // Table column ids are 1-based
On line 82, which I cannot add a comment to on GitHub, there's this:
List releases = configuration.newAPIPageBuilder.releases;
It might be better to change it to this:
List releases = builder.releases;
-
Marked as reviewed by prappo (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18194#pullrequestreview-1934169258
PR Review Comment: https://git.openjdk.org/jdk/pull/18194#discussion_r1523251733
Re: RFR: JDK-8328006: refactor large anonymous inner class in HtmlDocletWriter
On Tue, 12 Mar 2024 20:09:37 GMT, Jonathan Gibbons wrote: > Please review a conceptually simple change, to move a large anonymous inner > class to be a named inner class. > > While most of the code is simply moved, two simple changes were necessary. > In `visitEscape` and `visitText` the variable `result` from the enclosing > lexical scope was accessed directly. The change is simply to use the > equivalent value `content` which is passed to each `visit...` call. > > No tests needed updating. All javadoc tests pass. Looks good except for a duplicate semicolon. src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1295: > 1293: } > 1294: > 1295: var docTreeVisitor = new InlineVisitor(element, tag, > isLastNode, context, ch, trees);; Duplicate semicolon - Marked as reviewed by hannesw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18252#pullrequestreview-1933364693 PR Review Comment: https://git.openjdk.org/jdk/pull/18252#discussion_r1522689134
