On Mon, 19 Apr 2021 09:45:27 GMT, Hannes Wallnöfer <[email protected]> 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
