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

Fri Apr 30 04:33:58 UTC 2021

On Mon, 19 Apr 2021 09:45:27 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: Update JBS summary

I've read the source changes, but only skimmed through the tests at this point.

Various suggestions for your consideration or feedback.

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

> 123:                 .addTab(contents.exceptionsString, e -> utils.isException((TypeElement)e))
> 124:                 .addTab(contents.errorsString, e -> utils.isError((TypeElement)e))
> 125:                 .addTab(contents.annotationTypesString, utils::isAnnotationType);

I can't say I'm a big fan of the new-style `String` suffix, but  I guess it is OK for now; we need a cleanup on the names of members in `Contents` anyway.  More comments in `Contents`.

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

> 192:     public final String interfacesString;
> 193:     public final String packageSummary;
> 194:     public final String recordsString;

The new-style `String` suffix looks weird, and iut's unexpected to see `String` constants here, but, I see the constants were there before, and (worse) I added them!
I guess we generally need to separate these names from equivalent `Content` objects. OK for now, but worth re-examining in a future cleanup.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDoclet.java line 143:

> 141:                 // in doclets.properties
> 142:                 { "doclet.Annotation_Types_Summary", "doclet.Annotation_Interfaces_Summary" },
> 143:                 { "doclet.Enum_Summary", "doclet.Enum_Class_Summary" },

given you're changing this table, you should at least check manually if not in a test that you get old-style terminology with `--release 16` or older, and new-style terminology with `--release 17` or later.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleWriterImpl.java line 209:

> 207:                         HtmlTree.LI(links.createLink(HtmlIds.SERVICES, contents.navServices,
> 208:                             displayServices(uses, usesTrees) || displayServices(provides.keySet(), providesTrees)))
> 209:                 ));

In general, I like the new `.setSubNavLinks` method. :-)
Do you need the `HtmlTree.LI` wrappers, or is it enough to pass in a list of `links.createLink` calls?
The `HtmlTree.LI` are really internal detail of the subnavbar implementation ... i.e. a `ul` of `li` elements.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java line 107:

> 105:          * {@return a list of links to display in the sub-navigation area}
> 106:          * Links should be wrapped in {@code HtmlTree.LI} elements as they are
> 107:          * displayed within an unordered list.

As noted above, this class could do the wrapping with `LI` elements.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java line 144:

> 142:     }
> 143: 
> 144:     public Navigation setMemberSummaryBuilder(MemberSummaryBuilder memberSummaryBuilder) {

General comment for this and all the following red lines: yay!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java line 343:

> 341:             case PACKAGE:
> 342:             case CLASS:
> 343:             case HELP:

style comment ... since you're using new-style switch, do you want to do so here as well?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java line 344:

> 342:             case CLASS:
> 343:             case HELP:
> 344:                 List<Content> listContents = subNavLinks.getSubNavLinks();

I think this looks like where you could stream the list, wrap the entries in `LI` and convert back to a list.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageWriterImpl.java line 186:

> 184:                 packages.addAll(siblings);
> 185:             }
> 186:         }

Hmmm, I foresee downstream JBS issues ... "javadoc bug: I added a package to my code and the table disappeared".  Should there be a warning and a way to change the limit? (Eventually?)

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PackageWriterImpl.java line 260:

> 258:                 .addTab(contents.exceptionsString, e -> utils.isException((TypeElement)e))
> 259:                 .addTab(contents.errorsString, e -> utils.isError((TypeElement)e))
> 260:                 .addTab(contents.annotationTypesString, utils::isAnnotationType);

should we normalize the `utils.is...` methods so that we do not need the casts?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/PackageSummaryWriter.java line 130:

> 128:      * @param summaryContentTree the content tree to which the summaries will be added
> 129:      */
> 130:     void addAnnotationTypeSummary(SortedSet<TypeElement> annoTypes, Content summaryContentTree);

Nice cleanup!

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java line 111:

> 109: 
> 110:         private static final EnumSet<Kind> defaultSummarySet = EnumSet.of(
> 111:                 INNER_CLASSES, FIELDS, CONSTRUCTORS, METHODS);

Picky, maybe for another time: the correct term is probably `NESTED_CLASSES`.  Inner classes are a subset of nested classes that have an `outer` instance. (i.e. they're not `static` nested classes.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/VisibleMemberTable.java line 119:

> 117:                 FIELDS, CONSTRUCTORS, METHODS);
> 118:         private static final EnumSet<Kind> enumDetailSet = EnumSet.of(
> 119:                 ENUM_CONSTANTS, FIELDS, METHODS);

I'm a bit surprised by this, if only because we've had problems with tables like this in the past.

Is it possible to have a single combined list where some of the entries will give effectively empty tables which can be suppressed?

Case in point for an issue: why does `enumSummarySet` have `INNER_CLASSES` but `annotationSummarySet` does not.

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

Changes requested by jjg (Reviewer).

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