RFR: JDK-8263507: Improve structure of package summary pages [v3]
Hannes Wallnoefer
hannes.wallnoefer at oracle.com
Wed May 5 13:14:38 UTC 2021
Thanks for the reivew, Jon. Comments inline below.
> Am 30.04.2021 um 06:33 schrieb Jonathan Gibbons <jjg at openjdk.java.net>:
>
> 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.
>
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
More information about the javadoc-dev
mailing list