On 10/6/20 5:30 AM, Hannes Wallnöfer wrote:
On Mon, 5 Oct 2020 18:48:37 GMT, Jonathan Gibbons <j...@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
-------------
PR: https://git.openjdk.java.net/jdk/pull/499
