RFR: JDK-8253735: Cleanup SearchIndexItem API

[Jonathan Gibbons]

On Tue, 6 Oct 2020 11:05:35 GMT, Hannes Wallnöfer <hannesw 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/AbstractIndexWriter.java line 70:
> 
>> 68:
>> 69:     /**
>> 70:      * Initializes the common data for a writers that can generate index files
> 
> Typo: should be either "a writer" (singular) or "writers" (plural)

Yes, I'd already caught that one myself as well.

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Links.java line 289:
> 
>> 287:         String a = isProperty
>> 288:                 ? executableElement.getSimpleName().toString()
>> 289:                 : executableElement.getSimpleName()
> 
> I guess `getSimpleName()` returns `"<init>"` for constructor elements? The old code handled that case explicitly.

Yes, this is explicitly defined in `Element.getSimpleName`
https://docs.oracle.com/en/java/javase/14/docs/api/java.compiler/javax/lang/model/element/Element.html#getSimpleName()

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/IndexBuilder.java line 192:
> 
>> 190:     }
>> 191:
>> 192:     public SortedSet<IndexItem> getItems(IndexItem.Category cat) {
> 
> Public method should have javadoc comment.

Oops

-------------

PR: https://git.openjdk.java.net/jdk/pull/499