RFR: JDK-8257925 enable more support for nested inline tags

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8257925 enable more support for nested inline tags

Jonathan Gibbons-2
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 `&lt;` and `&gt;` 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.

-------------

Commit messages:
 - fix trailing whitespace
 - neaten alignment in test description
 - use TagletWriterImpl.Context#inTags to modify behavior on a nested {@index} tag
 - initial introduction of TagletWriterImpl.Context
 - JDK-8257925: enable more support for nested inline tags

Changes: https://git.openjdk.java.net/jdk/pull/2369/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2369&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8257925
  Stats: 708 lines in 14 files changed: 624 ins; 21 del; 63 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2369.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2369/head:pull/2369

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags

Pavel Rappo-3
On Wed, 3 Feb 2021 04:28:21 GMT, Jonathan Gibbons <[hidden email]> 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 `&lt;` and `&gt;` 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.

Although this patch generally looks reasonable, I'm surprised to see some of the behavior it introduces. For example, consider composition of `@link` tags from the included tests:

    /** First sentence. {@link #m1() ABC {@link #m2() DEF} GHI} */

Before the patch that doc comment resulted in the following HTML:

    First sentence. <a href="#m1()"><code>ABC {@link #m2() DEF} GHI</code></a>

After the patch that same doc comment results in HTML that looks like this:

    First sentence. <a href="#m1()"><code>ABC <a href="#m2()"><code>DEF</code></a> GHI</code></a>

I wouldn't expect that latter behavior. Not only nested `<a>` are invalid HTML, but they are also rendered strangely in browsers. In this particular case the trailing `GHI` appears as normal text, not as a hyperlink.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags

Jonathan Gibbons-2
On Wed, 3 Feb 2021 12:28:06 GMT, Pavel Rappo <[hidden email]> wrote:

> I wouldn't expect that latter behavior. Not only nested `<a>` are invalid HTML, but they are also rendered strangely in browsers. In this particular case the trailing `GHI` appears as normal text, not as a hyperlink.

Thanks; it was on my list to check whether nested links were allowed. I'm pleased they're not, and this will be another situation where the `inTags` info is useful.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v2]

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
> 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 `&lt;` and `&gt;` 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

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2369/files
  - new: https://git.openjdk.java.net/jdk/pull/2369/files/bd84b5dd..96bfeb59

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2369&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2369&range=00-01

  Stats: 17 lines in 3 files changed: 13 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2369.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2369/head:pull/2369

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v2]

Pavel Rappo-3
On Fri, 5 Feb 2021 04:44:07 GMT, Jonathan Gibbons <[hidden email]> 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 `&lt;` and `&gt;` 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

Thanks for fixing the nested `<a>` issue. With your last commit (96bfeb5) the output from

    /** First sentence. {@link #m1() ABC {@link #m2() DEF} GHI} */

looks like this

    First sentence. <a href="#m1()"><code>ABC DEF GHI</code></a>

and if doclint is enabled, javadoc issues a warning

    warning: nested tag: @link
    /** First sentence. {@link #m1() ABC {@link #m2() DEF} GHI} */
                                         ^

I would recommend comparing JDK API documentation before and after, if only to bring any uncovered surprises to maintainers' attention early.

-------------

Marked as reviewed by prappo (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v3]

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
> 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 `&lt;` and `&gt;` 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 with a new target base due to a merge or a rebase. The pull request now contains seven commits:

 - Merge with upstream/master
 - improve handling of nested links
 - fix trailing whitespace
 - neaten alignment in test description
 - use TagletWriterImpl.Context#inTags to modify behavior on a nested {@index} tag
 - initial introduction of TagletWriterImpl.Context
 - JDK-8257925: enable more support for nested inline tags

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2369/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2369&range=02
  Stats: 723 lines in 15 files changed: 637 ins; 21 del; 65 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2369.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2369/head:pull/2369

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v2]

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
On Fri, 12 Feb 2021 13:31:47 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   improve handling of nested links
>
> 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?

This is the general discussion about the use of `assert`, and whether it is better to throw an exception in production code, or "carry on regardless". I wish Java had a should-not-happen statement or at least a ShoudNotHappen exception.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v2]

Jonathan Gibbons-2
On Thu, 18 Feb 2021 18:47:16 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> 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?
>
> This is the general discussion about the use of `assert`, and whether it is better to throw an exception in production code, or "carry on regardless". I wish Java had a should-not-happen statement or at least a ShoudNotHappen exception.

In this case, `IllegalStateException` is a reasonable alternative

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v2]

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
On Fri, 12 Feb 2021 14:14:42 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   improve handling of nested links
>
> 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?

Yes, updated

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v2]

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
On Fri, 12 Feb 2021 14:25:23 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   improve handling of nested links
>
> 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));

Good catch

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v4]

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
> 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 `&lt;` and `&gt;` 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:

  Address review comments

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2369/files
  - new: https://git.openjdk.java.net/jdk/pull/2369/files/a6f3eb32..bbb98e6b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2369&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2369&range=02-03

  Stats: 39 lines in 5 files changed: 15 ins; 13 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2369.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2369/head:pull/2369

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8257925 enable more support for nested inline tags [v4]

Hannes Wallnöfer
On Thu, 18 Feb 2021 19:44:00 GMT, Jonathan Gibbons <[hidden email]> 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 `&lt;` and `&gt;` 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:
>
>   Address review comments

Looks good!

-------------

Marked as reviewed by hannesw (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2369
Reply | Threaded
Open this post in threaded view
|

Integrated: JDK-8257925 enable more support for nested inline tags

Jonathan Gibbons-2
In reply to this post by Jonathan Gibbons-2
On Wed, 3 Feb 2021 04:28:21 GMT, Jonathan Gibbons <[hidden email]> 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 `&lt;` and `&gt;` 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.

This pull request has now been integrated.

Changeset: c4f17a3e
Author:    Jonathan Gibbons <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/c4f17a3e
Stats:     733 lines in 15 files changed: 641 ins; 23 del; 69 mod

8257925: enable more support for nested inline tags

Reviewed-by: prappo, hannesw

-------------

PR: https://git.openjdk.java.net/jdk/pull/2369