Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8288058: Broken links on constant-values page #9162

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Jun 15, 2022

JDK 19:

Please review a medium-simple fix for some broken links on the constant-values page, from the "contents" index at the top of the page to the summary tables lower down on the page.

There are two root causes:

  1. The packages are grouped into "groups" with headings based on abbreviated package names, meaning "at most two leading components of each package name". At one point there is a probable typo, when a full package name is used instead of the abbreviated name, thus causing the wrong "id" to be generated, thus causing broken links. The fix for this is obvious: use the correct name.

  2. More seriously, the abbreviated package name is modeled using a PackageElement. While seemingly a good idea, the construction of these elements inherits the module element of the originating package. Then, the package elements are used to build a Set of headings, based on these package elements. The problem is that when the subpackages are found in different modules, they give rise to distinct "abbreviated package elements" in different modules and thus cause repeated instances of like-named headers in the output. This is easily visible as repeated headers for com.sun.* in (for example) the JDK 18 API. The fix is to use a Set<String> to model the set of headers and associated links, instead of Set<PackageElement>.

Along with the two fixes described above, there is minor code cleanup, including the removal of some ill-considered code in both Workarounds and Utils. Apart from those classes, the changes are confined to the "constant summary" builder and writers. The use of a member called "first" and related shenanigans is a holdover from the days when the page was generated literally top-to-bottom with print statements. It is easily removed.

The only existing test for the constant values page was laughably insufficient, and was effectively just a regression test for a minor issue in JDK 1.4.2. The test is enhanced with a bunch of new test cases, that exercise different scenarios for the list of contents at the top of the page, and the links to the corresponding sections lower down. The new test cases leverage the implicit support for checking links in the generated output, although they do forcibly enable that support, just to make sure it is active.

When comparing the new JDK API docs against recent JDK API docs, the only changes are the expected changes in the links and headings on the constant-values.html page.

There is more cleanup that could be done to this page ... such as suppressing the "contents:" list when it is just a single entry ... but that is an enhancement that is out of scope for this bug fix.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9162/head:pull/9162
$ git checkout pull/9162

Update a local copy of the PR:
$ git checkout pull/9162
$ git pull https://git.openjdk.org/jdk pull/9162/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9162

View PR using the GUI difftool:
$ git pr show -t 9162

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9162.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 15, 2022

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

printedPackageHeaders.add(abbrevPkg);
String abbrevPkgName = getAbbrevPackageName(currentPackage);
if (!packageGroupHeadings.contains(abbrevPkgName)) {
writer.addPackageGroup(abbrevPkgName, target);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is the fix for the "typo" that I referred to in item 1 of the main description.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jun 15, 2022
*/
protected final Set<PackageElement> printedPackageHeaders;
protected final Set<String> packageGroupHeadings;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is the basis for the fix for item 2 that I referred to run the main description, about the inappropriate use of PackageElement to model the headings.

@openjdk
Copy link

openjdk bot commented Jun 15, 2022

@jonathan-gibbons The following label will be automatically applied to this pull request:

  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the javadoc javadoc-dev@openjdk.org label Jun 15, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 15, 2022

Webrevs

@jonathan-gibbons
Copy link
Contributor Author

Superseded by openjdk/jdk19#62

@jonathan-gibbons jonathan-gibbons deleted the jdk19.8288058.constant-value-links branch June 28, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javadoc javadoc-dev@openjdk.org rfr Pull request is ready for review
1 participant