RFR [15] 8239876: Improve SearchIndexItem

[Jonathan Gibbons]

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.

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.

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