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.
