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-8253559: The INDEX page should link to Serialized Form and Constant Values pages #643

Conversation

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Oct 13, 2020

The mechanism to add links for summary pages in the main index is generalized, and now includes the Constant Values page, the Deprecated page and the Serialized Form page, provided these pages have non-empty content.

The only change to the generated output is very minor: the text of existing link to the "System Properties" page is changed to use a non-breaking space, in line with the links for "All Classes" and "All Packages".


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8253559: The INDEX page should link to Serialized Form and Constant Values pages

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/643/head:pull/643
$ git checkout pull/643

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 13, 2020

👋 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.

@openjdk openjdk bot added the rfr label Oct 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

@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 label Oct 13, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 13, 2020

(There is an equivalent field in `HtmlDocletWriter`)
Copy link
Member

@hns hns left a comment

I'm surprised the mechanism to add links to the top of the index page can be generalized with so few changes.

I think this change needs to be checked and possibly tuned for the changes it causes to the search index contents. Basically we need to make sure these summary pages show up in the search results in the correct places (categories).

@@ -2659,8 +2659,7 @@ public TreePath getTreePath(Element e) {
* A cache of doc comment info objects for elements.
* The entries may come from the AST and DocCommentParser, or may be autromatically
* generated comments for mandated elements and JavaFX properties.
*
* @see CommentUtils.dcInfoMap
** @see CommentUtils#dcInfoMap
Copy link
Member

@hns hns Oct 15, 2020

Choose a reason for hiding this comment

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

I think you unintentionally removed a line break while fixing the reference.

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Oct 15, 2020

Choose a reason for hiding this comment

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

Oops

if (item.isElementItem() || item.isTagItem()) {
// don't put summary-page items in the A-Z index:
// they are listed separately, at the top of the index page
itemsByFirstChar.computeIfAbsent(keyCharacter(item.getLabel()),
Copy link
Member

@hns hns Oct 15, 2020

Choose a reason for hiding this comment

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

You're preventing these summary pages from showing up in the alphabetic index, but not from the by-category index below. This means they are added to the javadoc search contents, which by itself is a good thing. However, this means we also need to think about the category under which the item is listed. For example, you are using IndexItem.Category.MEMBER for the Deprecated API list, but deprecation applies to all element categories. I guess we should use the TAGS category for summary pages unless there is a clear element category they fit in?

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Oct 15, 2020

Choose a reason for hiding this comment

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

Initially, I put the summary pages in the alphabetic index as well, but it disrupted test files too much. It was sort-of silly to see All Classes and All Packages showing up under A in the alphabetic index when previously there was no A entry. Hence the change to the current behavior.

See next comment about which category to use.

if (!deprapi.isEmpty() && configuration.mainIndex != null) {
configuration.mainIndex.add(IndexItem.of(IndexItem.Category.MEMBERS,
resources.getText("doclet.Deprecated_API"), path));
}
Copy link
Member

@hns hns Oct 15, 2020

Choose a reason for hiding this comment

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

IndexItem.Category.MEMBERS is probably not the correct category to use for the Deprecated API page, and we should choose a fitting category because these summary page items will end up in the search index (see my comment in IndexItem#add)

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Oct 15, 2020

Choose a reason for hiding this comment

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

Agreed. Now that you raise the issue, I realize that we should only use MODULE/PACKAGE/TYPES/MEMBERS for the appropriate kinds of elements, and should use TAGS for all "plain text" entries.

hns
hns approved these changes Oct 16, 2020
Copy link
Member

@hns hns left a comment

Jon, I actually liked "All Classes" and "All Packages" being in the Types and Packages categories, respectively. I think it made sense to show them there and they were easier to find and use.

However, I understand it is conceptually cleaner this way and I don't want to start a bikeshedding session over it, so I give an approving review, but I would be interested in your thoughts about this.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 16, 2020

@jonathan-gibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8253559: The INDEX page should link to Serialized Form and Constant Values pages

Reviewed-by: hannesw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 53 new commits pushed to the master branch:

  • e66c6bb: 8254349: The TestNoScreenMenuBar test should be updated
  • 402d01a: 8254795: Remove obsolete template files
  • 07ec35e: 8254623: gc/g1/TestHumongousConcurrentStartUndo.java still fails sometimes
  • 0570cc1: 8254855: Clean up and remove unused code in vmIntrinsics
  • 1742c44: 8254695: G1: Next mark bitmap clear not cancelled after marking abort
  • 34583eb: 8254161: Prevent instantiation of EnumSet subclasses through deserialization
  • 3d23bd8: 8254814: [Vector API] Fix an AVX512 crash after JDK-8223347
  • 7c0d417: 8251535: Partial peeling at unsigned test adds incorrect loop exit check
  • 5145bed: 8254125: Assertion in cppVtables.cpp during builds on 32bit Windows
  • bdda205: 8254369: Node::disconnect_inputs may skip precedences
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/3fb2e822be11a8fdfa708f88b7dcf7a5066a2335...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 16, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 16, 2020

Mailing list message from Jonathan Gibbons on javadoc-dev:

On 10/16/20 4:16 AM, Hannes Walln?fer wrote:

Jon, I actually liked "All Classes" and "All Packages" being in the Types and Packages categories, respectively. I
think it made sense to show them there and they were easier to find and use.

However, I understand it is conceptually cleaner this way and I don't want to start a bikeshedding session over it, so
I give an approving review, but I would be interested in your thoughts about this.

Hi Hannes,

Thanks for the feedback. I'll revert the changes related to "All
Classes" and "All Packages" for now.

However, as a follow-up, I think we should better document what kinds of
`IndexItem`s are acceptable in each of the files, and how they will appear.

-- Jon

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Oct 16, 2020

The only change in the latest commit is to back out the changes related to "All Classes" and "All Packages". Given that it builds and all tests still pass, I'll go with your earlier approval and integrate the change.

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Oct 16, 2020

/integrate

@openjdk openjdk bot closed this Oct 16, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 16, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 16, 2020

@jonathan-gibbons Since your change was applied there have been 53 commits pushed to the master branch:

  • e66c6bb: 8254349: The TestNoScreenMenuBar test should be updated
  • 402d01a: 8254795: Remove obsolete template files
  • 07ec35e: 8254623: gc/g1/TestHumongousConcurrentStartUndo.java still fails sometimes
  • 0570cc1: 8254855: Clean up and remove unused code in vmIntrinsics
  • 1742c44: 8254695: G1: Next mark bitmap clear not cancelled after marking abort
  • 34583eb: 8254161: Prevent instantiation of EnumSet subclasses through deserialization
  • 3d23bd8: 8254814: [Vector API] Fix an AVX512 crash after JDK-8223347
  • 7c0d417: 8251535: Partial peeling at unsigned test adds incorrect loop exit check
  • 5145bed: 8254125: Assertion in cppVtables.cpp during builds on 32bit Windows
  • bdda205: 8254369: Node::disconnect_inputs may skip precedences
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/3fb2e822be11a8fdfa708f88b7dcf7a5066a2335...master

Your commit was automatically rebased without conflicts.

Pushed as commit 83ea863.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@jonathan-gibbons jonathan-gibbons deleted the 8253559-index-summary-pages branch Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants