JDK-8263507: Improve structure of package summary pages #3413
Conversation
|
👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Mailing list message from Jonathan Gibbons on javadoc-dev: Hannes, Reading your email, and the comments about the number of tables, reminds This is not meant to invalidate the general work you have done to -- Jon On 4/9/21 7:29 AM, Hannes Walln?fer wrote: |
|
Mailing list message from Hannes Wallnoefer on javadoc-dev: Am 09.04.2021 um 17:22 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
Excellent idea! I wonder how this didn?t occur to me while working on the code. I?m not worried about the work I?ve done, as it will only reduce the number of links/ids. Should this be done as part of this change, or filed as separate issue? Hannes
|
|
Mailing list message from Jonathan Gibbons on javadoc-dev: On 4/9/21 12:59 PM, Hannes Wallnoefer wrote:
I think that's your call. I think there are 2 JBS issues, but you could do a single PR for both, As two issues, maybe it would make sense to do the tabbed table first, -- Jon
|
|
Mailing list message from Jonathan Gibbons on javadoc-dev: On 4/9/21 12:59 PM, Hannes Wallnoefer wrote:
I see we already have a similar table in the All Classes page. Looking * Change the page heading to "All Classes and Interfaces" -- Jon
-------------- next part -------------- |
|
Mailing list message from Hannes Wallnoefer on javadoc-dev: I have imlemented the tabbed classes and interfaces table and playing around with the wording. It looks like dropping the word ?Summary? would give us enough space to actually expand the first ?All Classes? tab to ?All Classes and Interfaces?, which I find more meaningful and correct. That is even with adding a ?Record Classes? tab which is currently missing on the ?All Classes? index page. Below is.a screenshot of how that would look like: [cid:CAA503F9-C160-4CA0-9439-2A5258DDD406 at fritz.box] I would also suggest to use the same wording for the tabs on the ?All Classes? index page and the package summary pages, as I don?t see why they should be different. Hannes Am 10.04.2021 um 17:55 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com<mailto:jonathan.gibbons at oracle.com>>: I see we already have a similar table in the All Classes page. Looking at that page I would suggest the following: ? Change the page heading to "All Classes and Interfaces" -------------- next part -------------- |
|
Mailing list message from Jonathan Gibbons on javadoc-dev: Looks good. Agreed that the terminology and appearance should be the same on the If space becomes an issue, I think the first tab could be reduced to Agreed that "All Classes" is less than ideal. It should either be "All Just asking: does the "Classes" tab refer to "all classes, including -- jon On 4/12/21 8:59 AM, Hannes Wallnoefer wrote:
-------------- next part -------------- |
|
Mailing list message from Hannes Wallnoefer on javadoc-dev: Am 13.04.2021 um 07:22 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
It just includes ?ordinary? classes as identified by Utils.isOrdinaryClass(TypeElement), so interfaces, enum classes, exceptions, errors and annotation interfaces are excluded. But I just discovered record classes are not (matching the fact that there is currently no ?Record Classes? tab in the All Classes page). Hannes
|
hns
changed the title
|
I opted for doing the summary table merge within this same PR. I couldn't find an existing JBS issue for it, and splitting the work into two changesets and reverse their order seemed like a non-trivial amount of work. I have updated the JBS summary to "Improve structure of package summary pages" to reflect the broader scope. |
| .setDefaultTab(contents.allClassesAndInterfaces) | ||
| .addTab(contents.interfacesString, utils::isInterface) | ||
| .addTab(contents.classesString, e -> utils.isOrdinaryClass((TypeElement)e)) | ||
| .addTab(contents.enumsString, utils::isEnum) | ||
| .addTab(contents.recordsString, e -> utils.isRecord((TypeElement)e)) | ||
| .addTab(contents.exceptionsString, e -> utils.isException((TypeElement)e)) | ||
| .addTab(contents.errorsString, e -> utils.isError((TypeElement)e)) | ||
| .addTab(contents.annotationTypesString, utils::isAnnotationType); |
There was a problem hiding this comment.
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.
| public final String allClassesAndInterfaces; | ||
| public final String annotationTypesString; | ||
| public final String classesString; | ||
| public final String enumsString; | ||
| public final String errorsString; | ||
| public final String exceptionsString; | ||
| public final String interfacesString; | ||
| public final String packageSummary; | ||
| public final String recordSummary; | ||
| public final String recordsString; |
There was a problem hiding this comment.
The new-style String suffix looks weird, and it'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.
| { "doclet.Annotation_Types_Summary", "doclet.Annotation_Interfaces_Summary" }, | ||
| { "doclet.Enum_Summary", "doclet.Enum_Class_Summary" }, |
There was a problem hiding this comment.
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.
| .setSubNavLinks(() -> List.of( | ||
| HtmlTree.LI(links.createLink(HtmlIds.MODULE_DESCRIPTION, contents.navDescription, | ||
| !utils.getFullBody(mdle).isEmpty() && !options.noComment())), | ||
| HtmlTree.LI(links.createLink(HtmlIds.MODULES, contents.navModules, | ||
| display(requires) || display(indirectModules))), | ||
| HtmlTree.LI(links.createLink(HtmlIds.PACKAGES, contents.navPackages, | ||
| display(packages) || display(indirectPackages) || display(indirectOpenPackages))), | ||
| HtmlTree.LI(links.createLink(HtmlIds.SERVICES, contents.navServices, | ||
| displayServices(uses, usesTrees) || displayServices(provides.keySet(), providesTrees))) | ||
| )); |
There was a problem hiding this comment.
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.
| * Links should be wrapped in {@code HtmlTree.LI} elements as they are | ||
| * displayed within an unordered list. |
There was a problem hiding this comment.
As noted above, this class could do the wrapping with LI elements.
| // add subpackages unless there are very many of them | ||
| Pattern subPattern = Pattern.compile(pkgName.replace(".", "\\.") + "\\.\\w+"); | ||
| List<PackageElement> subpackages = filterPackages( | ||
| p -> subPattern.matcher(p.getQualifiedName().toString()).matches()); | ||
| if (subpackages.size() <= MAX_SUBPACKAGES) { | ||
| packages.addAll(subpackages); | ||
| } | ||
|
|
||
| // only add sibling packages if we are beneath threshold, and number of siblings is beneath threshold as well | ||
| if (pkgPrefix != null && packages.size() <= MAX_SIBLING_PACKAGES) { | ||
| Pattern siblingPattern = Pattern.compile(pkgPrefix.replace(".", "\\.") + "\\.\\w+"); | ||
|
|
||
| List<PackageElement> siblings = filterPackages( | ||
| p -> siblingPattern.matcher(p.getQualifiedName().toString()).matches()); | ||
| if (siblings.size() <= MAX_SIBLING_PACKAGES) { | ||
| packages.addAll(siblings); | ||
| } | ||
| } |
There was a problem hiding this comment.
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?)
| .addTab(contents.interfacesString, utils::isInterface) | ||
| .addTab(contents.classesString, e -> utils.isOrdinaryClass((TypeElement)e)) | ||
| .addTab(contents.enumsString, utils::isEnum) | ||
| .addTab(contents.recordsString, e -> utils.isRecord((TypeElement)e)) | ||
| .addTab(contents.exceptionsString, e -> utils.isException((TypeElement)e)) | ||
| .addTab(contents.errorsString, e -> utils.isError((TypeElement)e)) | ||
| .addTab(contents.annotationTypesString, utils::isAnnotationType); |
There was a problem hiding this comment.
should we normalize the utils.is... methods so that we do not need the casts?
| void addInterfaceSummary(SortedSet<TypeElement> interfaces, Content summaryContentTree); | ||
|
|
||
| /** | ||
| * Adds the table of classes to the documentation tree. | ||
| * | ||
| * @param classes the classes to document. | ||
| * @param summaryContentTree the content tree to which the summaries will be added | ||
| */ | ||
| void addClassSummary(SortedSet<TypeElement> classes, Content summaryContentTree); | ||
|
|
||
| /** | ||
| * Adds the table of enums to the documentation tree. | ||
| * | ||
| * @param enums the enums to document. | ||
| * @param summaryContentTree the content tree to which the summaries will be added | ||
| */ | ||
| void addEnumSummary(SortedSet<TypeElement> enums, Content summaryContentTree); | ||
|
|
||
| /** | ||
| * Adds the table of records to the documentation tree. | ||
| * | ||
| * @param records the records to document. | ||
| * @param summaryContentTree the content tree to which the summaries will be added | ||
| */ | ||
| void addRecordSummary(SortedSet<TypeElement> records, Content summaryContentTree); | ||
|
|
||
| /** | ||
| * Adds the table of exceptions to the documentation tree. | ||
| * | ||
| * @param exceptions the exceptions to document. | ||
| * @param summaryContentTree the content tree to which the summaries will be added | ||
| */ | ||
| void addExceptionSummary(SortedSet<TypeElement> exceptions, Content summaryContentTree); | ||
|
|
||
| /** | ||
| * Adds the table of errors to the documentation tree. | ||
| * | ||
| * @param errors the errors to document. | ||
| * @param summaryContentTree the content tree to which the summaries will be added | ||
| */ | ||
| void addErrorSummary(SortedSet<TypeElement> errors, Content summaryContentTree); | ||
|
|
||
| /** | ||
| * Adds the table of annotation types to the documentation tree. | ||
| * | ||
| * @param annoTypes the annotation types to document. | ||
| * @param summaryContentTree the content tree to which the summaries will be added | ||
| */ | ||
| void addAnnotationTypeSummary(SortedSet<TypeElement> annoTypes, Content summaryContentTree); |
| public static final EnumSet<Kind> summarySet = EnumSet.range(INNER_CLASSES, METHODS); | ||
| public static final EnumSet<Kind> detailSet = EnumSet.range(ENUM_CONSTANTS, METHODS); | ||
| private static final EnumSet<Kind> defaultSummarySet = EnumSet.of( | ||
| INNER_CLASSES, FIELDS, CONSTRUCTORS, METHODS); |
There was a problem hiding this comment.
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.
| private static final EnumSet<Kind> enumSummarySet = EnumSet.of( | ||
| INNER_CLASSES, ENUM_CONSTANTS, FIELDS, METHODS); | ||
| private static final EnumSet<Kind> annotationSummarySet = EnumSet.of( | ||
| FIELDS, ANNOTATION_TYPE_MEMBER_OPTIONAL, ANNOTATION_TYPE_MEMBER_REQUIRED); | ||
| private static final EnumSet<Kind> defaultDetailSet = EnumSet.of( | ||
| FIELDS, CONSTRUCTORS, METHODS); | ||
| private static final EnumSet<Kind> enumDetailSet = EnumSet.of( | ||
| ENUM_CONSTANTS, FIELDS, METHODS); |
There was a problem hiding this comment.
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.
… rename INNER_CLASSES to NESTED_CLASSES
| .setDefaultTab(contents.allClassesAndInterfacesLabel.toString()) | ||
| .addTab(contents.interfaces.toString(), utils::isInterface) | ||
| .addTab(contents.classes.toString(), e -> utils.isOrdinaryClass((TypeElement)e)) | ||
| .addTab(contents.enums.toString(), utils::isEnum) | ||
| .addTab(contents.records.toString(), e -> utils.isRecord((TypeElement)e)) | ||
| .addTab(contents.exceptions.toString(), e -> utils.isException((TypeElement)e)) | ||
| .addTab(contents.errors.toString(), e -> utils.isError((TypeElement)e)) | ||
| .addTab(contents.annotationTypes.toString(), utils::isAnnotationType); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
The *Tab methods could and should be converted to accept Content. I didn't want to pack too many unrelated changes into this issue so I have filed a separate one for it:
| @@ -95,7 +95,7 @@ protected void buildAllPackagesFile() throws DocFileIOException { | |||
| */ | |||
| protected void addPackages(Content content) { | |||
| Table table = new Table(HtmlStyle.summaryTable) | |||
| .setCaption(Text.of(contents.packageSummary)) | |||
| .setCaption(Text.of(contents.packageSummaryLabel.toString())) | |||
There was a problem hiding this comment.
This seems a weird round trip: Text.of(contents.packageSummaryLabel.toString()
| public final String annotationTypeSummary; | ||
| public final String classSummary; | ||
| public final String enumSummary; | ||
| public final String errorSummary; | ||
| public final String exceptionSummary; | ||
| public final String interfaceSummary; | ||
| public final String packageSummary; | ||
| public final String recordSummary; | ||
|
|
| @@ -603,7 +603,7 @@ public boolean isNoType(TypeMirror t) { | |||
| } | |||
|
|
|||
| public boolean isOrdinaryClass(TypeElement te) { | |||
| if (isEnum(te) || isInterface(te) || isAnnotationType(te)) { | |||
| if (isEnum(te) || isInterface(te) || isAnnotationType(te) || isRecord(te)) { | |||
There was a problem hiding this comment.
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;
}
|
@hns This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 31 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
/integrate |
openjdk
bot
removed
ready
|
@hns Since your change was applied there have been 44 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit d2b5350. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Mailing list message from Hannes Wallnoefer on javadoc-dev: Thanks for the reivew, Jon. Comments inline below.
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.
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.
Done.
Will do.
Quite possible. Different issue though.
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.
I changed the name to `NESTED_CLASSES`.
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.
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. |
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
Navigationinstance which was then responsible for generating these links. The new code introduces aNavigation.SubNavLinksinterface 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:
An important implementation note is that I moved the code to compute package summary contents from
PackageSummaryBuildertoPackageWriterImpl. 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
idattributes to the existing<li>elements for each summary table. I decided against addingclassattributes 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 theids.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3413/head:pull/3413$ git checkout pull/3413Update a local copy of the PR:
$ git checkout pull/3413$ git pull https://git.openjdk.java.net/jdk pull/3413/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3413View PR using the GUI difftool:
$ git pr show -t 3413Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3413.diff