JDK-8262992: Improve `@see` output #2894

hns · 2021-03-09T15:04:35Z

This changes the output for @see tags to a <ul> structure. A different CSS style is used if any of the @see tag labels are longer than 30 characters or contain a comma.

The layout for the default CSS style is similar to the one we had so far with multiple links displayed inline and separated by commas. The CSS style for lists containing longer labels displays the list in block mode, but still separated by commas and without list markers. Note that the commas are generated via CSS in both cases.

As expected, there's a lot of test churn with this change. I converted some tests to text blocks that were still using old style string concatenation. In TestSingletonLists.java I had to exclude @see lists from the singleton check as javadoc generates a "See also" item for constant field values.

Progress

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

Issue

JDK-8262992: Improve @see output

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/2894
$ git pull https://git.openjdk.java.net/jdk pull/2894/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2894

View PR using the GUI difftool:
$ git pr show -t 2894

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2894.diff

bridgekeeper · 2021-03-09T15:05:14Z

👋 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 · 2021-03-09T15:06:52Z

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

javadoc
net

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

mlbridge · 2021-03-09T15:10:19Z

Webrevs

dfuch

Changes to java.net.URI look good. Don't count me as reviewer for the rest.

best regards,
-- daniel

jonathan-gibbons

Nice! I like the basic change to TagletWriterImpl.

Close to being approved, but there are some minor comments inline for disciussion or to be addressed.

jonathan-gibbons · 2021-03-11T15:00:33Z

src/java.base/share/classes/java/security/cert/PKIXRevocationChecker.java

 * @see <a href="http://www.ietf.org/rfc/rfc2560.txt"><i>RFC&nbsp;2560: X.509
 * Internet Public Key Infrastructure Online Certificate Status Protocol -
- * OCSP</i></a>, <br><a
- * href="http://www.ietf.org/rfc/rfc5280.txt"><i>RFC&nbsp;5280: Internet X.509
- * Public Key Infrastructure Certificate and Certificate Revocation List (CRL)
- * Profile</i></a>
+ * OCSP</i></a>
+ * @see <a href="http://www.ietf.org/rfc/rfc5280.txt"><i>RFC&nbsp;5280:
+ * Internet X.509 Public Key Infrastructure Certificate and Certificate
+ * Revocation List (CRL) Profile</i></a>


It's not wrong, but the use of <i>...</i> is non-standard. But, while we should split the malformed entry, it's arguably out of scope to tweak the style.

jonathan-gibbons · 2021-03-11T15:01:44Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java

+    // Threshold for length of @see tag label for switching from inline to block layout.
+    private static final int SEE_TAG_MAX_INLINE_LENGTH = 30;
+


For future consideration, we should look at encoding numbers like this in a config file.

jonathan-gibbons · 2021-03-11T15:04:11Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlStyle.java

+    /**
+     * The class for the list containing the {@code @see} tags of an element.
+     */
+    seeList,
+
+    /**
+     * The class for the list containing the {@code @see} tags of an element
+     * when some of the tags have longer labels.
+     */
+    seeListLong,
+


The list is alpha-sorted in each group, suggesting that these new entries should go before serializedClassDetails.

jonathan-gibbons · 2021-03-11T15:06:57Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/stylesheet.css

+ul.see-list li:not(:last-child):after,
+ul.see-list-long li:not(:last-child):after {
+    content: ", ";
+    white-space: pre-wrap;
+}


Cool! I didn't know you could do that. Is this widely supported?

According to MDN documentation all used pseudo-classes/elements are supported in all browsers including Internet Explorer 9.

https://developer.mozilla.org/en-US/docs/Web/CSS/:not
https://developer.mozilla.org/en-US/docs/Web/CSS/:last-child
https://developer.mozilla.org/en-US/docs/Web/CSS/::after

I have tested the pages on my old laptop using IE 11 and Edge 44. These are the oldest browsers I have access to, they both rendered both list styles correctly.

jonathan-gibbons · 2021-03-11T15:07:53Z

test/langtools/jdk/javadoc/doclet/testConstructors/TestConstructors.java

                    <dt>See Also:</dt>
-                    <dd><a href="Outer.Inner.html#%3Cinit%3E()"><code>Inner()</code></a>,\s
-                    <a href="Outer.Inner.html#%3Cinit%3E(int)"><code>Inner(int)</code></a>,\s
-                    <a href="Outer.Inner.NestedInner.html#%3Cinit%3E()"><code>NestedInner()</code></a>,\s
-                    <a href="Outer.Inner.NestedInner.html#%3Cinit%3E(int)"><code>NestedInner(int)</code></a>,\s
-                    <a href="#%3Cinit%3E()"><code>Outer()</code></a>,\s
-                    <a href="#%3Cinit%3E(int)"><code>Outer(int)</code></a></dd>""",
+                    <dd>
+                    <ul class="see-list">
+                    <li><a href="Outer.Inner.html#%3Cinit%3E()"><code>Inner()</code></a></li>
+                    <li><a href="Outer.Inner.html#%3Cinit%3E(int)"><code>Inner(int)</code></a></li>
+                    <li><a href="Outer.Inner.NestedInner.html#%3Cinit%3E()"><code>NestedInner()</code></a></li>
+                    <li><a href="Outer.Inner.NestedInner.html#%3Cinit%3E(int)"><code>NestedInner(int)</code></a></li>
+                    <li><a href="#%3Cinit%3E()"><code>Outer()</code></a></li>
+                    <li><a href="#%3Cinit%3E(int)"><code>Outer(int)</code></a></li>
+                    </ul>
+                    </dd>""",
                """


Nice new rendering ... i.e. the general use of <ul>

jonathan-gibbons · 2021-03-11T15:10:55Z

test/langtools/jdk/javadoc/doclet/testLinkOption/pkg/B.java

    * A external links, complicated by whitespace
    * @see <a href="http://www.ietf.org/rfc/rfc2279.txt"><i>RFC&nbsp;2279: UTF-8, a
-    * transformation format of ISO 10646</i></a>, <br><a
-    * href="http://www.ietf.org/rfc/rfc2373.txt"><i>RFC&nbsp;2373: IPv6 Addressing
-    * Architecture</i></a>, <br><a
-    * href="http://www.ietf.org/rfc/rfc2396.txt"><i>RFC&nbsp;2396: Uniform
-    * Resource Identifiers (URI): Generic Syntax</i></a>, <br><a
-    * href="http://www.ietf.org/rfc/rfc2732.txt"><i>RFC&nbsp;2732: Format for
-    * Literal IPv6 Addresses in URLs</i></a>, <br><a
-    * href="C.html">A nearby file</a>
+    * transformation format of ISO 10646</i></a>
+    * @see <a href="http://www.ietf.org/rfc/rfc2373.txt"><i>RFC&nbsp;2373: IPv6 Addressing
+    * Architecture</i></a>
+    * @see <a href="http://www.ietf.org/rfc/rfc2396.txt"><i>RFC&nbsp;2396: Uniform
+    * Resource Identifiers (URI): Generic Syntax</i></a>
+    * @see <a href="http://www.ietf.org/rfc/rfc2732.txt"><i>RFC&nbsp;2732: Format for
+    * Literal IPv6 Addresses in URLs</i></a>
+    * @see <a href="C.html">A nearby file</a>
    */


Wow, I didn't know we had "bad" examples in the test code. If there isn't one already, can you file an issue for me to updated DocLint with a check for this?

jonathan-gibbons · 2021-03-11T15:14:55Z

test/langtools/jdk/javadoc/doclet/testSingletonLists/TestSingletonLists.java

+                    if ("see-list".equals(attrs.get("class"))) {
+                        inSeeList = true;
+                    }


This is OK for now, because the lists from @see tags cannot be nested. But, I had a similar problem in my work yesterday, and so going forward we might want a more general solution to track the class of each list on the stack.

jonathan-gibbons · 2021-03-11T15:16:17Z

test/langtools/jdk/javadoc/doclet/testSingletonLists/TestSingletonLists.java

@@ -290,10 +283,15 @@ public void endElement(String name) {
            switch (name) {
                case "ul": case "ol": {
                    Map<String,Integer> c = counts.pop();
-                    if (c.get("li") == 0) {
+                    if (inSeeList) {
+                        // Ignore "Constant Field Values" @see items for final fields created by javadoc


I think the comment is too specific. Won't the problem also be triggered for classes with a single @see and no constant values? It m might also be triggered on serializable classes, with the link to the Serialized Form.

Yes, the problem could be triggered by various circumstances, but in this particular test (which doesn't contain any explicit @see tags) it is just the automatically generated "see also" link to the constant field values page. The fields in question can't be made non-final because they are in annotation interfaces, btw.

jonathan-gibbons · 2021-03-25T01:10:11Z

Reviewed earlier feedback. Waiting for responses...

bridgekeeper · 2021-04-22T15:45:32Z

@hns This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

openjdk · 2021-05-06T20:11:01Z

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

8262992: Improve `@see` output

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

0ca86da: 8266002: vmTestbase/nsk/jvmti/ClassPrepare/classprep001 should skip events for unexpected classes
52f1db6: 8262002: java/lang/instrument/VerifyLocalVariableTableOnRetransformTest.sh failed with "TestCaseScaffoldException: DummyClassWithLVT did not match .class file"
04f7112: 8266293: Key protection using PBEWithMD5AndDES fails with "java.security.InvalidAlgorithmParameterException: Salt must be 8 bytes long"
a90b33a: 8266573: Make sure blackholes are tagged for all JVMCI paths
2dcbedf: 8266044: Nested class summary should show kind of class or interface
e840597: 8266460: java.io tests fail on null stream with upgraded jtreg/TestNG
fcedfc8: 8266579: Update test/jdk/java/lang/ProcessHandle/PermissionTest.java & test/jdk/java/sql/testng/util/TestPolicy.java
c665dba: 8266561: Remove Compile::_save_argument_registers
47d4438: 8266426: ZHeapIteratorOopClosure does not handle native access properly
2438498: 8252758: Lanai: Optimize index calculation while copying glyphs
... and 12 more: https://git.openjdk.java.net/jdk/compare/6018336fc5e93675482b92df76594712c238adda...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.

hns · 2021-05-07T10:44:42Z

/integrate

openjdk · 2021-05-07T10:46:11Z

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

d2b5350: 8263507: Improve structure of package summary pages
a65021e: 8266618: Remove broken -XX:-OptoRemoveUseless
94c6177: 8266536: Provide a variant of os::iso8601_time which works with arbitrary timestamps
71b8ad4: 8266609: AArch64: include FP/LR space in LIR_Assembler::initial_frame_size_in_bytes()
ebb68d2: 8049700: Deprecate obsolete classes and methods in javax/swing/plaf/basic
3a474d9: 8265612: revise the help info for jmap histo command
c97f56c: 8266172: -Wstringop-overflow happens in vmError.cpp
43ad24f: 8265465: jcmd VM.cds should keep already dumped archive when exception happens
66191ff: 8266193: BasicJMapTest does not include testHistoParallel methods
36e5ad6: 8263236: runtime/os/TestTracePageSizes.java fails on old kernels
... and 22 more: https://git.openjdk.java.net/jdk/compare/6018336fc5e93675482b92df76594712c238adda...master

Your commit was automatically rebased without conflicts.

Pushed as commit e0c8688.

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

JDK-8262992: Improve @see output

ba3d513

openjdk bot added the rfr Pull request is ready for review label Mar 9, 2021

openjdk bot added javadoc javadoc-dev@openjdk.org net net-dev@openjdk.org labels Mar 9, 2021

dfuch reviewed Mar 9, 2021

View reviewed changes

Update @see tag in class java.security.cert.PKIXRevocationChecker

401ed70

jonathan-gibbons reviewed Mar 11, 2021

View reviewed changes

hns added 3 commits March 25, 2021 11:51

Merge branch 'master' into JDK-8262992

8eea887

Adapt new test

9d2da2a

Put enum constants into alphabetical order.

e17025b

Merge branch 'master' into JDK-8262992

a0799ba

jonathan-gibbons approved these changes May 6, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 6, 2021

openjdk bot closed this May 7, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8262992: Improve `@see` output #2894

JDK-8262992: Improve `@see` output #2894

hns commented Mar 9, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 9, 2021

openjdk bot commented Mar 9, 2021

mlbridge bot commented Mar 9, 2021 •

edited

Loading

dfuch left a comment

jonathan-gibbons left a comment

jonathan-gibbons Mar 11, 2021

jonathan-gibbons Mar 11, 2021

jonathan-gibbons Mar 11, 2021

jonathan-gibbons Mar 11, 2021

hns Mar 25, 2021

jonathan-gibbons Mar 11, 2021

jonathan-gibbons Mar 11, 2021

jonathan-gibbons Mar 11, 2021

jonathan-gibbons Mar 11, 2021 •

edited

Loading

hns Mar 25, 2021

jonathan-gibbons commented Mar 25, 2021

bridgekeeper bot commented Apr 22, 2021

openjdk bot commented May 6, 2021 •

edited

Loading

hns commented May 7, 2021

openjdk bot commented May 7, 2021

		// Threshold for length of @see tag label for switching from inline to block layout.
		private static final int SEE_TAG_MAX_INLINE_LENGTH = 30;

JDK-8262992: Improve @see output #2894

JDK-8262992: Improve @see output #2894

Conversation

hns commented Mar 9, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 9, 2021

openjdk bot commented Mar 9, 2021

mlbridge bot commented Mar 9, 2021 • edited Loading

Webrevs

dfuch left a comment

Choose a reason for hiding this comment

jonathan-gibbons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons commented Mar 25, 2021

bridgekeeper bot commented Apr 22, 2021

openjdk bot commented May 6, 2021 • edited Loading

hns commented May 7, 2021

openjdk bot commented May 7, 2021

JDK-8262992: Improve `@see` output #2894

JDK-8262992: Improve `@see` output #2894

hns commented Mar 9, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 9, 2021 •

edited

Loading

jonathan-gibbons Mar 11, 2021 •

edited

Loading

openjdk bot commented May 6, 2021 •

edited

Loading