Jon, > On 25 Feb 2020, at 21:59, Jonathan Gibbons <[email protected]> > 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. >> >
