JDK-8253735: Cleanup SearchIndexItem API

[jonathan-gibbons]

This pull-request is for a substantial refactoring and cleanup of the code
to build the static index pages and the files for the interactive search

There is no significant change in functionality, and all tests continue to
pass without change.

Improvements

• SearchIndexItem is merged into IndexItem
• SearchIndexItems (the collection of SearchIndexItems indexed by
  Category) is merged into IndexBuilder, which now maintains
  both maps: index items grouped by first character, and grouped by category
• In Category, the members INDEX and SYSTEM_PROPERTY are merged into
  single new entry TAGS, such that the members of Category now directly
  correspond with the JavaScript files generated for interactive search
• IndexItem now provides access to the DocTree node for those items
  that previously were SearchIndexItem. This can be used to differentiate
  between items for {@index...} and {@systemProperty}.
  This specific change was a primary motivation for all this work, in order
  to facilitate supporting additional new tags that can contribute items
  for the index.
• All index items (i.e. including those that previously were SearchIndexItem)
  are now created by static factory methods instead of directly calling
  constructors. This allows many values to be precomputed and stored in
  final fields or made available by overriding accessor methods in
  anonymous subtypes.
• The comparators for index items have been cleaned up. Previously, one
  of the comparators was "unstable", meaning that it could fall back on
  the value of mutable fields, and the toString output (which was used
  to generate the content for the JavaScript files.)
• Work to generate the JavaScript files has been moved out of
  AbstractIndexBuilder and its subtypes into a new class, HtmlIndexBuilder,
  that subclasses the main IndexBuilder.
  To facilitate that, some methods were moved from HtmlDocletWriter to
  Links. This is not ideal, because it introduces a dependence on Utils
  in Links that was not there before, but the choice was pragmatic and the
  least bad of the alternatives. Long term, we might want to move most
  of the formats/html/markup package into a new more standalone package that
  does not rely on other javadoc internals, and at that time, the factory
  objects like Links, TableHead and Table would just move up to the
  formats/html package.

The Changes

The work is done in a series of steps/commits, each with a specific focus
of the work involved. It may be instructive to review the changes in each
commit, to follow the overall evolution of the work. From the Git log,
the changes are as follows (oldest first)

• Move SearchIndexItem, SearchIndexItems to toolkit.utils
• Cleanup SearchIndexItem
• fix bug
• Simplify SearchIndexItems
• simplify statement
• Merge SearchIndexItems into IndexBuilder
• Cleanup IndexItem prior to merge with SearchIndexItem
• Cleanup SearchIndexItem prior to merge with IndexItem
• Merge SearchIndexItem into IndexItem
  (without changing how items are added to the index collections)
• simplify adding index items to the index builder
• move comparators for index items into IndexBuilder
• improve init for some index items
  improve comments in IndexItem
• Bug fix: obsolete call to add an item to the index
  Improve comparators used to build index
• Move getAnchor methods from HtmlDocletWriter to Links.
  This is slightly undesirable because it means that Links requires access to Utils, but it is less bad than the alternatives.
• Move code to complete initialization of index items and to write JavaScript index files from AbstractIndexWriter and subtypes to new subtype of IndexBuilder: HtmlIndexBuilder

At each stage, the repo should build and all javadoc tests should pass (and there is no reason to believe that any other tests may fail.)

Future work

Instead of maintaining collections of SortedSets in IndexBuilder, it might
be more effective to use List instead, and just sort the list as needed.

There is little need to eagerly build both maps in IndexBuilder. As long as
there is at least one collection, such as the itemsByFirstCharacter, we could
defer generating itemsByCategory until we need to write out the JavaScript
files. There is one place in the code, in SystemPropertyWriter, where we
look at itemsByCategory to determine whether there were any
{@systemProperty...} tags, but at the time we need that information, it
would not be significantly more expensive to scan indexByFirstCharacter,
because the items for elements need not have been added at this time.

---

Progress

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

Issue

• JDK-8253735: Cleanup SearchIndexItem API

Reviewers

• Hannes Wallnöfer (@hns - Reviewer) ⚠️ Review applies to 5343e74

Download

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

[bridgekeeper]

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

@jonathan-gibbons this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8253735-searchindexitem
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

[openjdk]

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

[jonathan-gibbons]

/reviewers 2

[openjdk]

@jonathan-gibbons
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

[mlbridge]

Webrevs

• 01: Full - Incremental (670afb1)
• 00: Full (5343e74)

[jonathan-gibbons]

Possible future RFE: Search tag is not a great term here, and doesn't really apply to tags like {@systemProperty ...} that exist for other reasons (e.g. to generate the System Properties page, and for which the entry in the interactive search is something of a secondary side-effect. Given that the IndexItem contains the original DocTree, we could vary the description based on the DocTree.Kind.
That is not done here in this work, because it would likely require corresponding changes to tests.

[jonathan-gibbons]

Also, what is the relationship between this content and the value in IndexItem.getDescription() ?

[jonathan-gibbons]

Logically, it would make more sense to write the System Properties page before writing the index files, in case the index files depend in any way on any for the individual pages being written.

[hns]

Excellent work, Jon! It's great to see index generating code going from pretty bad to quite nice in one major effort. I suggested some minor changes in the comments but nothing substantial, so reviewing as "approved".

[mlbridge]

Mailing list message from Jonathan Gibbons on javadoc-dev:

On 10/6/20 5:30 AM, Hannes Walln?fer wrote:

On Mon, 5 Oct 2020 18:48:37 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

This pull-request is for a substantial refactoring and cleanup of the code
to build the static index pages and the files for the interactive search

There is no significant change in functionality, and all tests continue to
pass without change.

## Improvements

* `SearchIndexItem` is merged into `IndexItem`
* `SearchIndexItems` (the collection of `SearchIndexItem`s indexed by
`Category`) is merged into `IndexBuilder`, which now maintains
both maps: index items grouped by first character, and grouped by category
* In `Category`, the members `INDEX` and `SYSTEM_PROPERTY` are merged into
single new entry `TAGS`, such that the members of `Category` now directly
correspond with the JavaScript files generated for interactive search
* `IndexItem` now provides access to the `DocTree` node for those items
that previously were `SearchIndexItem`. This can be used to differentiate
between items for `{@Index...}` and `{@systemProperty}`.
This specific change was a primary motivation for all this work, in order
to facilitate supporting additional new tags that can contribute items
for the index.
* All index items (i.e. including those that previously were `SearchIndexItem`)
are now created by static factory methods instead of directly calling
constructors. This allows many values to be precomputed and stored in
final fields or made available by overriding accessor methods in
anonymous subtypes.
* The comparators for index items have been cleaned up. Previously, one
of the comparators was "unstable", meaning that it could fall back on
the value of mutable fields, and the `toString` output (which was used
to generate the content for the JavaScript files.)
* Work to generate the JavaScript files has been moved out of
`AbstractIndexBuilder` and its subtypes into a new class, `HtmlIndexBuilder`,
that subclasses the main `IndexBuilder`.
To facilitate that, some methods were moved from `HtmlDocletWriter` to
`Links`. This is not ideal, because it introduces a dependence on `Utils`
in `Links` that was not there before, but the choice was pragmatic and the
least bad of the alternatives. Long term, we might want to move most
of the `formats/html/markup` package into a new more standalone package that
does not rely on other javadoc internals, and at that time, the factory
objects like `Links`, `TableHead` and `Table` would just move up to the
`formats/html` package.

## The Changes

The work is done in a series of steps/commits, each with a specific focus
of the work involved. It may be instructive to review the changes in each
commit, to follow the overall evolution of the work. From the Git log,
the changes are as follows (oldest first)

* Move `SearchIndexItem`, `SearchIndexItems` to `toolkit.utils`
* Cleanup `SearchIndexItem`
* fix bug
* Simplify `SearchIndexItems`
* simplify statement
* Merge `SearchIndexItems` into `IndexBuilder`
* Cleanup `IndexItem` prior to merge with `SearchIndexItem`
* Cleanup `SearchIndexItem` prior to merge with `IndexItem`
* Merge `SearchIndexItem` into `IndexItem`
(without changing how items are added to the index collections)
* simplify adding index items to the index builder
* move comparators for index items into IndexBuilder
* improve init for some index items
improve comments in IndexItem
* Bug fix: obsolete call to add an item to the index
Improve comparators used to build index
* Move `getAnchor` methods from `HtmlDocletWriter` to `Links`.
This is slightly undesirable because it means that `Links` requires access to `Utils`, but it is less bad than the
alternatives.
* Move code to complete initialization of index items and to write JavaScript index files from `AbstractIndexWriter` and
subtypes to new subtype of `IndexBuilder`: `HtmlIndexBuilder`

At each stage, the repo should build and all javadoc tests should pass (and there is no reason to believe that any
other tests may fail.)
## Future work

Instead of maintaining collections of `SortedSet`s in `IndexBuilder`, it might
be more effective to use `List` instead, and just sort the list as needed.

There is little need to eagerly build both maps in `IndexBuilder`. As long as
there is at least one collection, such as the `itemsByFirstCharacter`, we could
defer generating `itemsByCategory` until we need to write out the JavaScript
files. There is one place in the code, in `SystemPropertyWriter`, where we
look at `itemsByCategory` to determine whether there were any
`{@systemProperty...}` tags, but at the time we need that information, it
would not be significantly more expensive to scan `indexByFirstCharacter`,
because the items for elements need not have been added at this time.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SplitIndexWriter.java line 164:

162: contentTree.add(getVerticalSeparator());
163: contentTree.add(links.createLink(pathToRoot.resolve(DocPaths.SYSTEM_PROPERTIES),
164: contents.systemPropertiesLabel));
1. the `getItems` code could be simplified by providing `getItems(DocTree.Kind)`
2. This code is common to both `SingleIndexWriter` and `SplitIndexWriter` and so could be moved up into
`AbstractIndexWriter`
Since all four usages of `getItems(Category.TAGS)` then go on to look for `DocTree.Kind.SYSTEM_PROPERTY` and your
cleanup is almost perfect it would be a pity not to do item 1 in your list. Item 2 would be nice but a bit less
compelling IMO, maybe add a TODO comment?

I'll do Item 1. I think there's a possibility of merging all 3 of
AbstractIndexWriter and its two subtypes, which would
obviously supersede Item 2, but I'd prefer to do that as a separate change.

-- Jon

[jonathan-gibbons]

/reviewers 1

[openjdk]

@jonathan-gibbons
The number of required reviews for this PR is now set to 1.

[openjdk]

@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 more details.

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

8253735: Cleanup SearchIndexItem API

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 25 new commits pushed to the master branch:

• 54b340b: 8254081: java/security/cert/PolicyNode/GetPolicyQualifiers.java fails due to an expired certificate
• 51fdb4c: 8254075: Shenandoah: Remove ShenandoahCodeRootsStyle diagnostic flag and related test
• 77921b9: 8254080: fix for JDK-8204256 causes jlink test failures
• 57493c1: 8253694: Remove Thread::muxAcquire() from ThreadCrashProtection()
• d2b1dc6: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
• a34f48b: 8253832: CharsetDecoder : decode() mentioning CoderMalfunctionError behavior not as per spec
• f397b60: 8251123: doclint warnings about missing javadoc tags and comments
• c9d1dcc: 8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure
• 9199783: 8253565: PPC64: Fix duplicate if condition in vm_version_ppc.cpp
• 1728547: 8254010: GrowableArrayView::print fails to compile
• ... and 15 more: https://git.openjdk.java.net/jdk/compare/1c2754bfe3294fb3c80defe5479bdd85b0d07e29...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.

[jonathan-gibbons]

/integrate

[openjdk]

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

• 54b340b: 8254081: java/security/cert/PolicyNode/GetPolicyQualifiers.java fails due to an expired certificate
• 51fdb4c: 8254075: Shenandoah: Remove ShenandoahCodeRootsStyle diagnostic flag and related test
• 77921b9: 8254080: fix for JDK-8204256 causes jlink test failures
• 57493c1: 8253694: Remove Thread::muxAcquire() from ThreadCrashProtection()
• d2b1dc6: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
• a34f48b: 8253832: CharsetDecoder : decode() mentioning CoderMalfunctionError behavior not as per spec
• f397b60: 8251123: doclint warnings about missing javadoc tags and comments
• c9d1dcc: 8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure
• 9199783: 8253565: PPC64: Fix duplicate if condition in vm_version_ppc.cpp
• 1728547: 8254010: GrowableArrayView::print fails to compile
• ... and 15 more: https://git.openjdk.java.net/jdk/compare/1c2754bfe3294fb3c80defe5479bdd85b0d07e29...master

Your commit was automatically rebased without conflicts.

Pushed as commit bd50ccd.

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