Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8260388: Listing (sub)packages at package level of API documentation #2917

Closed
wants to merge 5 commits into from

Conversation

hns
Copy link
Member

@hns hns commented Mar 10, 2021

This adds a "Related Packages" table to package summary pages with a list of neighboring tables. The rules for including packages is as follows:

  1. The super package of the current package is included if it exists.
  2. Direct subpackages of the current package are included, but only if their number does not exceed 20, in which case no subpackages are included.
  3. If steps 1 and 2 did not produce more than 5 packages, sibling packages of the current package are included, but only if their number is not greater than 5.

In steps 2. and 3. above, either all or no candidate packages are included. In other words, if the threshold is exceeded none of the candidate packages are included in the list. The cut-off values are arbitrary, but have shown to produce reasonably good results with various codebases.

A few example of API documentation generated with this feature:

http://cr.openjdk.java.net/~hannesw/8260388/api.01/
http://cr.openjdk.java.net/~hannesw/8260388/javafx.01/
http://cr.openjdk.java.net/~hannesw/8260388/tomcat.02/
http://cr.openjdk.java.net/~hannesw/8260388/servlet.01/

This is a first step to introduce the new feature. There may be future enhancements to fine-tune the feature or make it more customizable.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8260388: Listing (sub)packages at package level of API documentation

Reviewers

Download

To checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2917/head:pull/2917
$ git checkout pull/2917

To update a local copy of the PR:
$ git checkout pull/2917
$ git pull https://git.openjdk.java.net/jdk pull/2917/head

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 10, 2021

👋 Welcome back hannesw! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Mar 10, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 10, 2021

@hns The following label will be automatically applied to this pull request:

  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the javadoc label Mar 10, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 10, 2021

Webrevs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 10, 2021

Mailing list message from Daniel Fuchs on javadoc-dev:

Hi Hannes,

I wonder if it's a good idea (or not) to show "related" packages
that come from other modules.

For instance - if you click on java.base, then on java.net, you see
2 related packages:

- java.net.spi (which is in java.base)
- java.net.http - which is in another module (java.net.http) - but
this information is absent from the table.

I wonder if this is going to cause confusion.

Similarly - jf clicking on module java.net.http, and then on
package java.net.http, you see java.net and java.net.spi as
related package. But the spi from java.net.spi is not used
in any way by java.net.http. So this is a bit more questionable
whether this reference is useful.

Otherwise - I agree that for other packages - like java.lang - it
does look good.

Maybe the list should limit itself to subpackages in the same module?

best regards,

-- daniel

On 10/03/2021 16:49, Hannes Walln?fer wrote:

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 11, 2021

Mailing list message from Hannes Wallnoefer on javadoc-dev:

Hi Daniel,

Thanks for the feedback (and the review of the java.net.URI change).

I agree that the value of this feature varies depending on the code base, since it is all based on package naming.

However, I think the module spanning aspect is actually a good thing in most cases, as it complements the exising hierarchichal structure of documentation. For example, in the case of the java.net and java.net.http packages, it could be quite useful for somebody looking for the java.net.URL* classes to be reminded that there is actually a fully featured HTTP client as well, should they need one.

There are other cross-module links that could be useful, such as the inclusion of java.util.logging and java.util.prefs in the java.util package summary.

I admit the link between java.net.http and java.net.spi is useless, but I don?t think it?s that confusing either. IMO the feature has a positive net benefit. But of course it can and should be improved further based on feedback we get.

Hannes

Am 10.03.2021 um 18:18 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I wonder if it's a good idea (or not) to show "related" packages
that come from other modules.

For instance - if you click on java.base, then on java.net, you see
2 related packages:

- java.net.spi (which is in java.base)
- java.net.http - which is in another module (java.net.http) - but
this information is absent from the table.

I wonder if this is going to cause confusion.

Similarly - jf clicking on module java.net.http, and then on
package java.net.http, you see java.net and java.net.spi as
related package. But the spi from java.net.spi is not used
in any way by java.net.http. So this is a bit more questionable
whether this reference is useful.

Otherwise - I agree that for other packages - like java.lang - it
does look good.

Maybe the list should limit itself to subpackages in the same module?

best regards,

-- daniel

On 10/03/2021 16:49, Hannes Walln?fer wrote:

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 11, 2021

Mailing list message from Daniel Fuchs on javadoc-dev:

Hi Hannes,

I am a bit conflicted about this.

I agree with you that the link from java.net to java.net.http can
be useful.

I guess my main gripe is the fact that when the related package is
not in the same module this is not apparent.

How about changing that?
I mean - when the related package is in an other module could
this information be recorded in the table?

Something like:

Package java.net
----------------

Related Packages
----------------

+ java.net.spi

+ java.net.http in module java.net.http

best regards,

-- daniel

On 11/03/2021 09:14, Hannes Wallnoefer wrote:

Hi Daniel,

Thanks for the feedback (and the review of the java.net.URI change).

I agree that the value of this feature varies depending on the code base, since it is all based on package naming.

However, I think the module spanning aspect is actually a good thing in most cases, as it complements the exising hierarchichal structure of documentation. For example, in the case of the java.net and java.net.http packages, it could be quite useful for somebody looking for the java.net.URL* classes to be reminded that there is actually a fully featured HTTP client as well, should they need one.

There are other cross-module links that could be useful, such as the inclusion of java.util.logging and java.util.prefs in the java.util package summary.

I admit the link between java.net.http and java.net.spi is useless, but I don?t think it?s that confusing either. IMO the feature has a positive net benefit. But of course it can and should be improved further based on feedback we get.

Hannes

Am 10.03.2021 um 18:18 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I wonder if it's a good idea (or not) to show "related" packages
that come from other modules.

For instance - if you click on java.base, then on java.net, you see
2 related packages:

- java.net.spi (which is in java.base)
- java.net.http - which is in another module (java.net.http) - but
this information is absent from the table.

I wonder if this is going to cause confusion.

Similarly - jf clicking on module java.net.http, and then on
package java.net.http, you see java.net and java.net.spi as
related package. But the spi from java.net.spi is not used
in any way by java.net.http. So this is a bit more questionable
whether this reference is useful.

Otherwise - I agree that for other packages - like java.lang - it
does look good.

Maybe the list should limit itself to subpackages in the same module?

best regards,

-- daniel

On 10/03/2021 16:49, Hannes Walln?fer wrote:

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 11, 2021

Mailing list message from Hannes Wallnoefer on javadoc-dev:

That is a very good idea to make users aware of the cross-module link. I?ll give it a try and see how it works.

Thanks Daniel!

Hannes

Am 11.03.2021 um 10:50 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I am a bit conflicted about this.

I agree with you that the link from java.net to java.net.http can
be useful.

I guess my main gripe is the fact that when the related package is
not in the same module this is not apparent.

How about changing that?
I mean - when the related package is in an other module could
this information be recorded in the table?

Something like:

Package java.net
----------------

Related Packages
----------------

+ java.net.spi

+ java.net.http in module java.net.http

best regards,

-- daniel

On 11/03/2021 09:14, Hannes Wallnoefer wrote:

Hi Daniel,
Thanks for the feedback (and the review of the java.net.URI change).
I agree that the value of this feature varies depending on the code base, since it is all based on package naming.
However, I think the module spanning aspect is actually a good thing in most cases, as it complements the exising hierarchichal structure of documentation. For example, in the case of the java.net and java.net.http packages, it could be quite useful for somebody looking for the java.net.URL* classes to be reminded that there is actually a fully featured HTTP client as well, should they need one.
There are other cross-module links that could be useful, such as the inclusion of java.util.logging and java.util.prefs in the java.util package summary.
I admit the link between java.net.http and java.net.spi is useless, but I don?t think it?s that confusing either. IMO the feature has a positive net benefit. But of course it can and should be improved further based on feedback we get.
Hannes

Am 10.03.2021 um 18:18 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I wonder if it's a good idea (or not) to show "related" packages
that come from other modules.

For instance - if you click on java.base, then on java.net, you see
2 related packages:

- java.net.spi (which is in java.base)
- java.net.http - which is in another module (java.net.http) - but
this information is absent from the table.

I wonder if this is going to cause confusion.

Similarly - jf clicking on module java.net.http, and then on
package java.net.http, you see java.net and java.net.spi as
related package. But the spi from java.net.spi is not used
in any way by java.net.http. So this is a bit more questionable
whether this reference is useful.

Otherwise - I agree that for other packages - like java.lang - it
does look good.

Maybe the list should limit itself to subpackages in the same module?

best regards,

-- daniel

On 10/03/2021 16:49, Hannes Walln?fer wrote:

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 11, 2021

Mailing list message from Hannes Wallnoefer on javadoc-dev:

Daniel,

I uploaded documentation with the cross-module link indication you suggested:

http://cr.openjdk.java.net/~hannesw/8260388/api.02/

I?m not qute convinced, curious to hear what you and others think.

Hannes

Am 11.03.2021 um 12:07 schrieb Hannes Wallnoefer <hannes.wallnoefer at oracle.com>:

That is a very good idea to make users aware of the cross-module link. I?ll give it a try and see how it works.

Thanks Daniel!

Hannes

Am 11.03.2021 um 10:50 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I am a bit conflicted about this.

I agree with you that the link from java.net to java.net.http can
be useful.

I guess my main gripe is the fact that when the related package is
not in the same module this is not apparent.

How about changing that?
I mean - when the related package is in an other module could
this information be recorded in the table?

Something like:

Package java.net
----------------

Related Packages
----------------

+ java.net.spi

+ java.net.http in module java.net.http

best regards,

-- daniel

On 11/03/2021 09:14, Hannes Wallnoefer wrote:

Hi Daniel,
Thanks for the feedback (and the review of the java.net.URI change).
I agree that the value of this feature varies depending on the code base, since it is all based on package naming.
However, I think the module spanning aspect is actually a good thing in most cases, as it complements the exising hierarchichal structure of documentation. For example, in the case of the java.net and java.net.http packages, it could be quite useful for somebody looking for the java.net.URL* classes to be reminded that there is actually a fully featured HTTP client as well, should they need one.
There are other cross-module links that could be useful, such as the inclusion of java.util.logging and java.util.prefs in the java.util package summary.
I admit the link between java.net.http and java.net.spi is useless, but I don?t think it?s that confusing either. IMO the feature has a positive net benefit. But of course it can and should be improved further based on feedback we get.
Hannes

Am 10.03.2021 um 18:18 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I wonder if it's a good idea (or not) to show "related" packages
that come from other modules.

For instance - if you click on java.base, then on java.net, you see
2 related packages:

- java.net.spi (which is in java.base)
- java.net.http - which is in another module (java.net.http) - but
this information is absent from the table.

I wonder if this is going to cause confusion.

Similarly - jf clicking on module java.net.http, and then on
package java.net.http, you see java.net and java.net.spi as
related package. But the spi from java.net.spi is not used
in any way by java.net.http. So this is a bit more questionable
whether this reference is useful.

Otherwise - I agree that for other packages - like java.lang - it
does look good.

Maybe the list should limit itself to subpackages in the same module?

best regards,

-- daniel

On 10/03/2021 16:49, Hannes Walln?fer wrote:

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 11, 2021

Mailing list message from Daniel Fuchs on javadoc-dev:

Hi Hannes,

I think I like it better - could be even more useful if the
module name was a link.

You're not planning to introduce any javadoc tag to
customize this - I mean - to customize that list - are you?

best regards,

-- daniel

On 11/03/2021 13:59, Hannes Wallnoefer wrote:

Daniel,

I uploaded documentation with the cross-module link indication you suggested:

http://cr.openjdk.java.net/~hannesw/8260388/api.02/

I?m not qute convinced, curious to hear what you and others think.

Hannes

Am 11.03.2021 um 12:07 schrieb Hannes Wallnoefer <hannes.wallnoefer at oracle.com>:

That is a very good idea to make users aware of the cross-module link. I?ll give it a try and see how it works.

Thanks Daniel!

Hannes

Am 11.03.2021 um 10:50 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I am a bit conflicted about this.

I agree with you that the link from java.net to java.net.http can
be useful.

I guess my main gripe is the fact that when the related package is
not in the same module this is not apparent.

How about changing that?
I mean - when the related package is in an other module could
this information be recorded in the table?

Something like:

Package java.net
----------------

Related Packages
----------------

+ java.net.spi

+ java.net.http in module java.net.http

best regards,

-- daniel

On 11/03/2021 09:14, Hannes Wallnoefer wrote:

Hi Daniel,
Thanks for the feedback (and the review of the java.net.URI change).
I agree that the value of this feature varies depending on the code base, since it is all based on package naming.
However, I think the module spanning aspect is actually a good thing in most cases, as it complements the exising hierarchichal structure of documentation. For example, in the case of the java.net and java.net.http packages, it could be quite useful for somebody looking for the java.net.URL* classes to be reminded that there is actually a fully featured HTTP client as well, should they need one.
There are other cross-module links that could be useful, such as the inclusion of java.util.logging and java.util.prefs in the java.util package summary.
I admit the link between java.net.http and java.net.spi is useless, but I don?t think it?s that confusing either. IMO the feature has a positive net benefit. But of course it can and should be improved further based on feedback we get.
Hannes

Am 10.03.2021 um 18:18 schrieb Daniel Fuchs <daniel.fuchs at oracle.com>:

Hi Hannes,

I wonder if it's a good idea (or not) to show "related" packages
that come from other modules.

For instance - if you click on java.base, then on java.net, you see
2 related packages:

- java.net.spi (which is in java.base)
- java.net.http - which is in another module (java.net.http) - but
this information is absent from the table.

I wonder if this is going to cause confusion.

Similarly - jf clicking on module java.net.http, and then on
package java.net.http, you see java.net and java.net.spi as
related package. But the spi from java.net.spi is not used
in any way by java.net.http. So this is a bit more questionable
whether this reference is useful.

Otherwise - I agree that for other packages - like java.lang - it
does look good.

Maybe the list should limit itself to subpackages in the same module?

best regards,

-- daniel

On 10/03/2021 16:49, Hannes Walln?fer wrote:

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

All OK; some suggestions for future work/cleanup/improvement

// Maximum number of subpackages and sibling packages to list in related packages table
private final static int MAX_SUBPACKAGES = 20;
private final static int MAX_SIBLING_PACKAGES = 5;

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Mar 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future work, move these to a config file.

Short-term "workaround" suggestions still for a separate RFR: annotate these and similar declarations with an internal source-retention annotation of @Configuration

@openjdk
Copy link

@openjdk openjdk bot commented Mar 12, 2021

@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:

8260388: Listing (sub)packages at package level of API documentation

Reviewed-by: jjg

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 44 new commits pushed to the master branch:

  • 8120064: 8263781: C2: Cannot hoist independent load above arraycopy
  • 9689863: 8262295: C2: Out-of-Bounds Array Load from Clone Source
  • a678a38: 8263743: redundant lock in SSLSocketImpl
  • 3fcb499: 8263768: JFormattedTextField.AbstractFormatter.getDocumentFilter()/getNavigationFilter() spec doesn't mention what the default impls return and what does it mean
  • 4155533: 8258753: StartTlsResponse.close() hangs due to synchronization issues
  • 3e18330: 8264018: AArch64: NEON loadV2 and storeV2 addressing is wrong
  • 0ff8168: 8258957: DocLint: check for HTML start element at end of body
  • 3d7f912: 8220266: add support for additional metadata in add/remove programs
  • 37f494c: 8260619: Add final modifier to several DataFlavor static fields
  • cfc9aa3: 8264002: Delete outdated assumptions about ColorSpace initialization
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/d7268fa3a68c2356548651a27b307d7f7158e700...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 12, 2021
@hns
Copy link
Member Author

@hns hns commented Mar 24, 2021

I have added a change to this PR to add a "Module" column to the related packages table if any of the packages is in a different module. Example output for the JDK libraries is available here (java.lang, java.util and com.sun.tools.* packages have a module column):

http://cr.openjdk.java.net/~hannesw/8260388/api.03/

Implementation note: I added a HtmlStyle.colExtra CSS style because existing column classes either had unwanted rules (such as displaying links in bold font) or didn't fit for other reasons.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

Mailing list message from Daniel Fuchs on javadoc-dev:

On 24/03/2021 15:04, Hannes Walln?fer wrote:

I have added a change to this PR to add a "Module" column to the related packages table if any of the packages is in a different module. Example output for the JDK libraries is available here (`java.lang`, `java.util` and `com.sun.tools.*` packages have a module column):

http://cr.openjdk.java.net/~hannesw/8260388/api.03/

I like it! :-)

cheers,

-- daniel

@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 24, 2021

Mailing list message from Jonathan Gibbons on javadoc-dev:

I like it except that I was expecting the module name (column) to
precede the package name (column).

Is there a strong reason to have the module name column appear second?

-- Jon

On 3/24/21 8:04 AM, Hannes Walln?fer wrote:

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

Definitely better with the new column for the linked module name.

This is OK as is, but I think it would be better to have the module column first, before the package column.

public void addRelatedPackagesSummary(List<PackageElement> relatedPackages, Content summaryContentTree) {
boolean showModules = configuration.showModules && hasRelatedPackagesInOtherModules(relatedPackages);
TableHeader tableHeader= showModules
? new TableHeader(contents.packageLabel, contents.moduleLabel, contents.descriptionLabel)
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK, but I think it would be better if the module column came first.

Copy link
Member Author

@hns hns Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it out and was very surprised to find I agree with you. I'm changing the order in a new commit.

public void addPackageSummary(List<PackageElement> packages, Content label,
TableHeader tableHeader, Content summaryContentTree,
boolean showModules) {
if(!packages.isEmpty()) {
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace after if

addSummaryComment(pkg, description);
}
if (showModules) {
table.addRow(packageLink, moduleLink, description);
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like the companion row to the header entry, to put the module column first.

* The class of the cells in a table column used to display additional
* information without any particular style.
*/
colExtra,
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra in the same seems irrelevant. How about colPlain, to indicate it is a column with no additional style.

Copy link
Member Author

@hns hns Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering colPlain, but dismissed it because I thought we or somebody else might actually add styles to it at some point. But I'll take the risk of future cognitive dissonance and go with colPlain.

@hns
Copy link
Member Author

@hns hns commented Mar 25, 2021

/integrate

@openjdk openjdk bot closed this Mar 25, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 25, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@hns Since your change was applied there have been 44 commits pushed to the master branch:

  • 8120064: 8263781: C2: Cannot hoist independent load above arraycopy
  • 9689863: 8262295: C2: Out-of-Bounds Array Load from Clone Source
  • a678a38: 8263743: redundant lock in SSLSocketImpl
  • 3fcb499: 8263768: JFormattedTextField.AbstractFormatter.getDocumentFilter()/getNavigationFilter() spec doesn't mention what the default impls return and what does it mean
  • 4155533: 8258753: StartTlsResponse.close() hangs due to synchronization issues
  • 3e18330: 8264018: AArch64: NEON loadV2 and storeV2 addressing is wrong
  • 0ff8168: 8258957: DocLint: check for HTML start element at end of body
  • 3d7f912: 8220266: add support for additional metadata in add/remove programs
  • 37f494c: 8260619: Add final modifier to several DataFlavor static fields
  • cfc9aa3: 8264002: Delete outdated assumptions about ColorSpace initialization
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/d7268fa3a68c2356548651a27b307d7f7158e700...master

Your commit was automatically rebased without conflicts.

Pushed as commit a9d287a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated javadoc
2 participants