RFR: JDK-8263507: Improve structure of package summary pages [v5]

[Jonathan Gibbons]

On Wed, 5 May 2021 13:21:16 GMT, Hannes Wallnöfer <hannesw at openjdk.org> wrote:

>> This adds a feature to add sub-navigation links to the package summary page, but ended up more like a refactoring of the code to generate sub-navigation links. The reason for this is that generation of these links was highly idiosyncratic. Every writer class that wanted to create sub-nav links deposited something of itself in the `Navigation` instance which was then responsible for generating these links. The new code introduces a `Navigation.SubNavLinks` interface that allows writers to provide a list of links into their page content.
>> 
>> As for the new feature in the package summary page itself, I chose an approach that is a bit different from the one we use for other types of pages. Instead of displaying the inactive label instead of the link when a section is not present on the page, only the active links are displayed. The reason for this is that the package summary page contains so many potential summary tables that the sub-nav area gets quite crowded if they are all shown. Just showing the actually present pieces looked better to me.
>> 
>> Like in other sub-nav sections, the link labels sometimes use abbreviated terms such as "RELATED" instead of "RELATED PACKAGES" and "ENUMS" instead of "ENUM CLASSES". The full list of potential package sub-nav links is as follows:
>> 
>>     Package: Description | Related | Interfaces | Classes | Enums | Records | Exceptions | Errors | Annotations
>> 
>> An important implementation note is that I moved the code to compute package summary contents from `PackageSummaryBuilder` to `PackageWriterImpl`. The reason for this is that the contents are required to determine which links to create, and I didn't want to re-compute this information that was previously computed on the fly in the builder class. The various summary items are now stored in collection fields in the writer class. 
>> 
>> I have tried to add all the new properties and constants in a sensible place, which usually means alphabetic order within the sub-group of related entries.
>> 
>> I chose to keep the markup structure of the package summary page mostly unchanged, adding only `id` attributes to the existing `<li>` elements for each summary table. I decided against adding `class` attributes as well as it seems very unlikely to me that somebody would want to apply different styles to the various summary tables. Even without them, it could be done using the `id`s.
>
> Hannes Wallnöfer has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8263507: More suggested code cleanup

Generally good.
There are some minor suggestions for additional cleanup, but we could file those for later and declare victory.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AllClassesIndexWriter.java line 125:

> 123:                 .addTab(contents.exceptions.toString(), e -> utils.isException((TypeElement)e))
> 124:                 .addTab(contents.errors.toString(), e -> utils.isError((TypeElement)e))
> 125:                 .addTab(contents.annotationTypes.toString(), utils::isAnnotationType);

Maybe the `*Tab` methods could be overloaded to accept `Content` ?  Maybe later cleanup?
What does `.toString()` do in the malformed case of providing an `HtmlTree` ... should there be a method on `Content`? Maybe later cleanup?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AllPackagesIndexWriter.java line 98:

> 96:     protected void addPackages(Content content) {
> 97:         Table table = new Table(HtmlStyle.summaryTable)
> 98:                 .setCaption(Text.of(contents.packageSummaryLabel.toString()))

This seems a weird round trip: `Text.of(contents.packageSummaryLabel.toString()`

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Contents.java line 193:

> 191:     public final String packageSummary;
> 192:     public final String recordSummary;
> 193: 

��

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 606:

> 604: 
> 605:     public boolean isOrdinaryClass(TypeElement te) {
> 606:         if (isEnum(te) || isInterface(te) || isAnnotationType(te) || isRecord(te)) {

For your consideration ... thinking aloud, this could be done in a way that is better protected against future changes by

if (elementUtils.isClass(te) && te.getKind() != ElementKind.CLASS
        || elementUtils.isInterface(te) && te.getKind() != ElementKind.INTERFACE)
    return false;
}

-------------

Marked as reviewed by jjg (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/3413