On Wed, 5 May 2021 13:21:16 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: 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
