Thanks for the reivew, Jon. Comments inline below.
> Am 30.04.2021 um 06:33 schrieb Jonathan Gibbons <j...@openjdk.java.net>:
>
> On Mon, 19 Apr 2021 09:45:27 GMT, Hannes Wallnöfer <hann...@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.
>
I pushed a follow-up commit to change all the String constants in Contents.java
to Content objects. These constants are used for tab contents in Table.java
which take String arguments. For now I just added a #toString() conversion for
these usages, but I can file a JBS issue to convert these parameter types to
`Content`, which is what they are converted to anyway.
> 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.
Instead of the removed messages we are now using the pre-existing messages in
plural form without the „Summary“ suffix (e.g. „Enum Classes“ instead of „Enum
Class Summary". The test that checks for correct terminology
(TestTerminology.java) only covers the singular form of these messages (e.g.
„Enum Class“) so it only makes sure the mechanism is working, not that each and
every use in the docs is correct. I manually checked to make sure the correct
terminology is used in the affected summary tables.
> 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.
Done.
> 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?
>
Will do.
> 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?)
>
Quite possible. Different issue though.
> 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?
>
I looked into it, but ran into sublte issues (many of these methods are
implemented by exclusion, which makes their behaviour depend on its parameter
type. So I’d prefer to handle this as a separate issue, if we think it’s worth
changing.
> 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.
I changed the name to `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?
>
Not possible here because these are used for the sub-navigation links, so if a
table for some kind is empty the link label is still rendered as plain text. So
removing this logic from here means we have to filter out member kinds
somewhere else (this was done in the many lines of code in `Navigation` that I
was able to remove). I prefer to have it sorted at the source.
> Case in point for an issue: why does `enumSummarySet` have `INNER_CLASSES`
> but `annotationSummarySet` does not.
>
That is a bug that was present before. It is even baked into a test we have.
Obviously it is very uncommon to have nested classes in annotation interfaces,
or else somebody would have filed a bug. I’ll do that now.
> -------------
>
> Changes requested by jjg (Reviewer).
>
> PR: https://git.openjdk.java.net/jdk/pull/3413
