On Fri, 5 Feb 2021 04:44:07 GMT, Jonathan Gibbons <j...@openjdk.org> wrote:
>> Please review an update to improve the support, where appropriate, for
>> nested inline tags.
>>
>> It has always been the case that certain inline tags allow text and HTML to
>> appear within them. With tags like `{@return}` and `{@summary}` it becomes
>> desirable to also generally allow nested inline tags to appear in those
>> places as well. The work for this was started with the support for
>> `{@return}` [JDK-8075778](https://bugs.openjdk.java.net/browse/JDK-8075778),
>> but applying the work more generally was out of scope at the time. This
>> change completes the work that was started then.
>>
>> The work can be grouped into 4 parts, in 3 commits.
>>
>> ## Commit 1
>>
>> * Update `DocCommentParser` to syntactically allow nested inline tags in
>> situations that permit text and HTML
>> * Update the downstream code to semantically limit nestg where it does not
>> make sense, and provide new tests to verify the behavior.
>>
>> A family of new tests are added, each testing the ability to put an inline
>> tag within another of the same kind, with and without doclint being enabled.
>> In addition, test cases are added placing a simple instance `{@value}` in an
>> enclosing tag: this is a useful case just because the expansion is plain
>> text and therefore valid in all situations. Additional tests and test cases
>> can be added as needed.
>>
>> This commit left the `{@index}` tag generating "bad" code when it was
>> nested. The error was "reference to an undeclared ID". The (temporary)
>> solution was to disable the automatic link checking for this specific
>> subtest.
>>
>> ## Commit 2
>>
>> * `HtmlDocletWriter` and `TagletWriterImpl ` pass around a pair of booleans
>> `isFirstSentence` and `inSummary` to help determine the output to be
>> generated. Conceptually, a third value is added to that group: a set
>> containing the set of nested tag kinds, so that it is possible to determine
>> the enclosing tags for a tag. But, rather than add a third parameter to be
>> passed around, the 3 are grouped into a new class `TagletWriterImpl.Context`
>> which encapsulates the two booleans and the new set. The new class is added
>> in a way to minimize code churn. No tests are affected by this change: all
>> continue to pass.
>>
>> ## Commit 3
>>
>> * The new `Context#inTags` field is used to help improve the behavior of
>> nested `{@index}` tags even when used incorrectly, with warnings disabled.
>> As a result, the temporary change in the first commit to disable automatic
>> link checking in one of the test cases is reverted.
>>
>> <hr>
>>
>> The introduction of the new Context class is arguably more general than we
>> need at this time, but it clears up some erratic and inconsistent use of the
>> `isFirstSentence` and `inSummary` booleans. The new class also provides a
>> better framework for any complex new inline tags we may add in future. We
>> might want to change the `Set<DocTree.Kind>` to some other collection at
>> some point, if needs be (a stack, for example.) We might also want to move
>> more information into the `Context`, such as the related `Element` that is
>> otherwise ubiquitously passed around.
>>
>> The overall cleanup also revealed some latent bugs in the code, that were
>> hidden in some of the tests. Most notable was that there were still some
>> cases were `<` and `>` were not being correctly escaped as `<` and `>`
>> leading to output in some tests of the form `List<String>` ! This triggered
>> a minor cleanup/rewrite of the beginning of
>> `HtmlDocletWriter.seeTagsToContent` which was previously a bit too liberal
>> with the use of `new RawHtml`! The other minor cleanup was more consistent
>> handling of whitespace at the end of the first sentence, as will be seen in
>> a couple of places in one of the tests that was updated.
>
> Jonathan Gibbons has updated the pull request incrementally with one
> additional commit since the last revision:
>
> improve handling of nested links
That's a very nice improvement! I have added a few comments and suggestions,
but nothing of major importance.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 1082:
> 1080: default -> {
> 1081: assert false;
> 1082: return HtmlTree.EMPTY;
What is the reason to `assert false` here and in other places instead of, say,
always throw a RuntimeException?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 1615:
> 1613: if (label.isEmpty()) {
> 1614: label = new
> StringContent(node.getReference().getSignature());
> 1615: }
That's quite a bit of work for something probably very few people should try...
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
line 1637:
> 1635: public Boolean visitSee(SeeTree node, Content c) {
> 1636: // we need to pass the DocTreeImpl here, so ignore
> node
> 1637: result.add(seeTagToContent(element, tag, context));
Is above comment still correct? Aren't `tag` and `node` the same object?
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/TagletWriterImpl.java
line 165:
> 163: */
> 164: public TagletWriterImpl(HtmlDocletWriter htmlWriter, boolean
> isFirstSentence, boolean inSummary) {
> 165: super(isFirstSentence);
To avoid almost identical constructors this one could be implemented as
this(htmlWriter, new Context(isFirstSentence, inSummary));
test/langtools/jdk/javadoc/doclet/testNestedInlineTags/TestNestedLinkTag.java
line 108:
> 106: "ABC <a href=\"#m2()\"");
> 107: checkOutput("p/C.html", true,
> 108: "ABC DEF GHI");
Since you are running this with and without doclint, should this or any test in
this file check for doclint warnings?
-------------
Marked as reviewed by hannesw (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2369
