Thanks for looking into this. > On 18 Feb 2020, at 22:57, Jonathan Gibbons <[email protected]> > wrote: > > In a number of places, you use a variable or field named "indexbuilder" that > would be better written in all situations as "indexBuilder". No need for > re-review to fix that.
That practice predates this fix, but I'll update the fields as requested. > The work is good, for what it does, but it triggers a discussion for a > potential followup. What follows is a discussion for future work, not > necessarily part of this review. That exact follow-up you are talking about has been in the works for some time now. Stay tuned. > "IndexBuilder" is "old" and now is only half the story. It's the A-Z index, > and is unrelated to the interactive search index. There's a bunch of fields > in HtmlConfiguration that could arguably be moved/merged into an imporved > abstraction of IndexBuilder that is able to manage the A-Z index as well as > the interactive index. Either that, or, IndexBuilder is renamed to > AZIndexBuilder, and we create a new sibling structure for the interactive > search. For the fields in question, look at this list from > HtmlConfiguration: > > protected SortedSet<SearchIndexItem> memberSearchIndex > ; > > protected SortedSet<SearchIndexItem> moduleSearchIndex > ; > > protected SortedSet<SearchIndexItem> packageSearchIndex > ; > > protected SortedSet<SearchIndexItem> tagSearchIndex > ; > > protected SortedSet<SearchIndexItem> typeSearchIndex > ; > > protected Map<Character,List<SearchIndexItem>> tagSearchIndexMap = new > HashMap<>(); > > protected Set<Character> tagSearchIndexKeys; > These fields, and probably much of the code to manage them should be moved > out of HtmlConfiguration into either IndexBuilder or a some new abstraction > to manage these index objects. This would be philosophically similar to the > recent cleanup to move the collections of fields for options out into > distinct abstractions. > > -- Jon > > On 02/18/2020 07:13 AM, Pavel Rappo wrote: >> Hi Hannes, >> >> I've updated the webrev: >> >> >> http://cr.openjdk.java.net/~prappo/8239243/webrev.01/ >> >> >> Thanks! >> >> >>> On 18 Feb 2020, at 09:52, Hannes Wallnöfer <[email protected]> >>> wrote: >>> >>> Hi Pavel, >>> >>> +1 for the conditional index building, and the IndexBuilder cleanup is a >>> major improvement in every respect. >>> >>> Nitpicking here, but two naming suggestions: >>> >>> - „index“ can be both verb and noun, so renaming the „index“ method to >>> „indexElements“ might make its meaning a bit clearer and puts it in line >>> with the other index* methods >>> >>> - since firstCharacter(String) doesn’t always return the first character >>> maybe something like „indexCharacter“ or „keyCharacter“ would be more >>> fitting? >>> >>> Hannes >>> >>> >>> >>>> Am 17.02.2020 um 16:35 schrieb Pavel Rappo <[email protected]> >>>> : >>>> >>>> Hello, >>>> >>>> Please review the change for >>>> https://bugs.openjdk.java.net/browse/JDK-8239243 >>>> : >>>> >>>> >>>> http://cr.openjdk.java.net/~prappo/8239243/webrev.00/ >>>> >>>> >>>> I noticed that the index structure, used in Index Page an others, is >>>> created >>>> unconditionally (i.e. regardless of the presence of the "-noindex" >>>> command-line >>>> option). Since populating that index doesn't seem to have any >>>> side-effects, this >>>> will unnecessarily consume resources of documentation generation process >>>> when no >>>> index is required. Thus, moving it under the relevant if-statement should >>>> be >>>> safe and the right thing to do. I've also cleaned up the IndexBuilder >>>> class a bit. >>>> >>>> In case you wonder, when generating the API docs for the JDK 15, it takes >>>> about >>>> 1 sec. (one second) to create that structure. The number of instances of >>>> Element >>>> in that Map<Character, SortedSet<Element>> is roughly 50,000 (fifty >>>> thousand). >>>> >>>> -Pavel >>>> >>>> >
