RFR [15] 8239876: Improve SearchIndexItem

[Pavel Rappo]

Jon,

> On 25 Feb 2020, at 21:59, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
> 
> Nice cleanup, and a good stepping stone to more cleanup sometime down the road, such as maybe combining the data for the search index and A-Z index.
> 
> Minor points:
> 
> Although it was nice to see the individual "phase" webrevs, it also made it a bit harder to envisage the cumulative effect in files that were affected in multiple phases. A cumulative webrev would be nice.

http://cr.openjdk.java.net/~prappo/8239876/webrev.00/full/

> There is something of a coding convention in the doclet that we create local aliases for doclet-wide data structures obtained from the configuration, using consistent names for each alias across the various components.

Done. Thanks.

> No need to re-review for those changes, although posting a cumulative webrev would be good for the record.
> 
> -- Jon
> 
> On 02/25/2020 05:11 AM, Pavel Rappo wrote:
>> Hello,
>> 
>> Please review the change for https://bugs.openjdk.java.net/browse/JDK-8239876:
>> 
>>   http://cr.openjdk.java.net/~prappo/8239876/webrev.00/
>> 
>> This is a cleanup change. For convenience I split it into 3 changesets, called
>> phase-1, phase-2, and phase-3, which are applied in this particular order.
>> 
>> phase-1 changes the way instances of SearchIndexItem are managed.
>> SearchIndexItem is a POJO whose instances make up the index for the interactive
>> search feature (JDK-8141492). SearchIndexItem has categories. Before the change
>> items were stored in the HtmlConfiguration class in 5 (five) different sets, a
>> set per category. This was both error-prone and not easily extensible. After the
>> change, there are no sets, but there is a container (SearchIndexItem*s*), which
>> groups items by category and adapts to new categories automatically.
>> 
>> phase-2 moves a couple of structures, an extra map and a set, used for
>> alphabetic indexing of instances of SearchIndexItem from HtmlConfiguration to
>> AbstractIndexWriter. This latter abstract class and its two subclasses,
>> SplitIndexWriter and SingleIndexWriter, are the only clients of those
>> structures. This declutters HtmlConfiguration, a weird global object that seems
>> to have evolved to hold references to everything but the kitchen sink.
>> 
>> phase-3 is a minor cleanup which concludes the change.
>> 
>>  ***
>> 
>> There are a lot more issues than this change addresses. Here are some of them:
>> 
>> 1. SearchIndexItem is mutable and prone to inconsistencies due to not enforcing
>> any structure and invariants.
>> 
>> 2. SearchIndexItem is defined in terms of java.lang.String, where other options
>> might be more appropriate.
>> 
>> 3. SearchIndexItem does not specify the equals and hashCode methods.
>> Currently, the container relies on comparators when storing items.
>> 
>> Speaking of comparators. There are hidden assumptions about the order of items
>> in the retrieved collections. A particular order is assumed when SearchIndexItem
>> instances are merged with elements from IndexBuilder when printing out HTML in
>> the {Split, Single}IndexWriter classes. That same order is also used when
>> displaying search results to the user.
>> 
>> 4. IndexBuilder and SearchIndexItems are still intertwined. It's not easy to
>> draw the line where each of them is fully initialized and thus can be used.
>> Index corresponding to the {@index} and {@systemProperty} tags is put into
>> SearchIndexItems while traversing classes. Index corresponding to elements is
>> put into IndexBuilder during its initialization. Then {Split, Single}IndexWriter
>> updates the SearchIndexItems from IndexBuilder [*] while simultaneously querying
>> SearchIndexItems to print out the index for the {@index} and {@systemProperty}
>> tags.
>> 
>> After the change has been applied, the end result will not be ideal.
>> But it will be definitely better.
>> 
>> -Pavel
>> 
>> -------------------------------------------------------------------------------
>> [*] This suggests that there's a bug where interactive search results will
>> contain only {@index} and {@systemProperty} entries if the "-noindex"
>> command-line option is specified. Surprisingly, the interactive search is
>> disabled if that option is specified. I guess it's a separate bug. This implicit
>> dependency between index pages and interactive search has to be straightened
>> out.
>> 
>> This discovery refers to my recent cleanup
>> 
>>   https://mail.openjdk.java.net/pipermail/javadoc-dev/2020-February/001414.html
>> 
>> Namely, "populating that index doesn't seem to have any side-effects". This is
>> still true. It's just that those side-effects happen to reside in {Split,
>> Single}IndexWriter, which opportunistically updates the SearchIndexItems
>> container.
>> 
>