RFR: JDK-8259283: use new HtmlId and HtmlIds classes

[Jonathan Gibbons-2]

Please review this change to centralize the management of HTML ids used by the standard doclet in a single new factory class, `HtmlIds`, which utilizes a new type-safe wrapper class around `String` called `HtmlId`.

The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and when referencing them (e.g. `Links.createLink`).  The enum `SectionName`, which declared a set of standard ids, goes away, as do methods in `Links` and other places to create "anchors".

This is a simple refactoring: there is no intentional change of functionality, and no tests are affected or need to be updated. However, the changes do highlight the inconsistency of the use of ids: it would be good to rationalize these in separate, targeted follow-up work.

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

Commit messages:
 - fix whitespace; add doc comments
 - JDK-8259283: use new HtmlId and HtmlIds classes

Changes: https://git.openjdk.java.net/jdk/pull/1951/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1951&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259283
  Stats: 949 lines in 35 files changed: 545 ins; 248 del; 156 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1951.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1951/head:pull/1951

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

[Jonathan Gibbons-2]

> Please review this change to centralize the management of HTML ids used by the standard doclet in a single new factory class, `HtmlIds`, which utilizes a new type-safe wrapper class around `String` called `HtmlId`.
>
> The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and when referencing them (e.g. `Links.createLink`).  The enum `SectionName`, which declared a set of standard ids, goes away, as do methods in `Links` and other places to create "anchors".
>
> This is a simple refactoring: there is no intentional change of functionality, and no tests are affected or need to be updated. However, the changes do highlight the inconsistency of the use of ids: it would be good to rationalize these in separate, targeted follow-up work.

Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:

 - Merge remote-tracking branch 'upstream/master' into htmlids, and resolve conflicts
   
   # Conflicts:
   # src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/DeprecatedListWriter.java
   # src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
   # src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java
 - fix whitespace; add doc comments
 - JDK-8259283: use new HtmlId and HtmlIds classes

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

Changes: https://git.openjdk.java.net/jdk/pull/1951/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1951&range=01
  Stats: 1002 lines in 38 files changed: 552 ins; 275 del; 175 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1951.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1951/head:pull/1951

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

[Jonathan Gibbons-2]

> Please review this change to centralize the management of HTML ids used by the standard doclet in a single new factory class, `HtmlIds`, which utilizes a new type-safe wrapper class around `String` called `HtmlId`.
>
> The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and when referencing them (e.g. `Links.createLink`).  The enum `SectionName`, which declared a set of standard ids, goes away, as do methods in `Links` and other places to create "anchors".
>
> This is a simple refactoring: there is no intentional change of functionality, and no tests are affected or need to be updated. However, the changes do highlight the inconsistency of the use of ids: it would be good to rationalize these in separate, targeted follow-up work.

Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:

  tidy up merge

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1951/files
  - new: https://git.openjdk.java.net/jdk/pull/1951/files/5d32ab5e..2ddcf132

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

  Stats: 9 lines in 2 files changed: 2 ins; 3 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1951.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1951/head:pull/1951

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

[Hannes Wallnöfer]

On Mon, 11 Jan 2021 23:55:16 GMT, Jonathan Gibbons <[hidden email]> wrote:

>> Please review this change to centralize the management of HTML ids used by the standard doclet in a single new factory class, `HtmlIds`, which utilizes a new type-safe wrapper class around `String` called `HtmlId`.
>>
>> The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and when referencing them (e.g. `Links.createLink`).  The enum `SectionName`, which declared a set of standard ids, goes away, as do methods in `Links` and other places to create "anchors".
>>
>> This is a simple refactoring: there is no intentional change of functionality, and no tests are affected or need to be updated. However, the changes do highlight the inconsistency of the use of ids: it would be good to rationalize these in separate, targeted follow-up work.
>
> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>
>   tidy up merge

Very nice work, Jon! +1

A few inline comments for an unused field and method and a couple of questions.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Links.java line 234:

> 232:      * @return a content tree for the link
> 233:      */
> 234:     public Content createExternalLink(DocLink link, Content label) {

The `createLink(DocLink, Content, boolan)` method above (line #221) that is replaced by this new method is not used anymore (and within it, the boolean parameter is not used).

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Table.java line 301:

> 299:             rowStyle = stripedStyles.get(rowIndex % 2);
> 300:         }
> 301:         Set<String> tabClasses = new HashSet<>(); // !! would be better as a List

I assume no bug has been filed for this?

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java line 75:

> 73:         super(configuration, configuration.getOptions().noDeprecated());
> 74:         this.configuration = configuration;
> 75:         links = new Links(DocPath.empty);

It looks like `links` isn't used anywhere else in `HtmlIndexBuilder` and can be removed.

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlId.java line 34:

> 32:  * @see HtmlTree#setId(HtmlId)
> 33:  */
> 34: public interface HtmlId {

Is there a reason for making this an interface instead of a plain class?

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

Marked as reviewed by hannesw (Reviewer).

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

[Jonathan Gibbons-2]

On Wed, 13 Jan 2021 10:53:30 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   tidy up merge
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Links.java line 234:
>
>> 232:      * @return a content tree for the link
>> 233:      */
>> 234:     public Content createExternalLink(DocLink link, Content label) {
>
> The `createLink(DocLink, Content, boolan)` method above (line #221) that is replaced by this new method is not used anymore (and within it, the boolean parameter is not used).

Ah, good catch!  Will fix.

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Table.java line 301:
>
>> 299:             rowStyle = stripedStyles.get(rowIndex % 2);
>> 300:         }
>> 301:         Set<String> tabClasses = new HashSet<>(); // !! would be better as a List
>
> I assume no bug has been filed for this?

Not yet. I have a list of a number of minor cleanups to do.

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlIndexBuilder.java line 75:
>
>> 73:         super(configuration, configuration.getOptions().noDeprecated());
>> 74:         this.configuration = configuration;
>> 75:         links = new Links(DocPath.empty);
>
> It looks like `links` isn't used anywhere else in `HtmlIndexBuilder` and can be removed.

OK, will check.

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

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

[Jonathan Gibbons-2]

On Wed, 13 Jan 2021 15:03:17 GMT, Hannes Wallnöfer <[hidden email]> wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   tidy up merge
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/HtmlId.java line 34:
>
>> 32:  * @see HtmlTree#setId(HtmlId)
>> 33:  */
>> 34: public interface HtmlId {
>
> Is there a reason for making this an interface instead of a plain class?

At one point, I had in mind using an enum class for the set of fixed ids, and so wanted an interface that I could mix in to the declaration of that class.

Eventually, when we have value types, `HtmlId` could be a good candidate for that feature.

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

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

[Jonathan Gibbons-2]

> Please review this change to centralize the management of HTML ids used by the standard doclet in a single new factory class, `HtmlIds`, which utilizes a new type-safe wrapper class around `String` called `HtmlId`.
>
> The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and when referencing them (e.g. `Links.createLink`).  The enum `SectionName`, which declared a set of standard ids, goes away, as do methods in `Links` and other places to create "anchors".
>
> This is a simple refactoring: there is no intentional change of functionality, and no tests are affected or need to be updated. However, the changes do highlight the inconsistency of the use of ids: it would be good to rationalize these in separate, targeted follow-up work.

Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits:

 - Address review comments
 - Merge remote-tracking branch 'upstream/master' into htmlids
 - tidy up merge
 - Merge remote-tracking branch 'upstream/master' into htmlids, and resolve conflicts
   
   # Conflicts:
   # src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/DeprecatedListWriter.java
   # src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
   # src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Navigation.java
 - fix whitespace; add doc comments
 - JDK-8259283: use new HtmlId and HtmlIds classes

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

Changes: https://git.openjdk.java.net/jdk/pull/1951/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1951&range=03
  Stats: 1022 lines in 39 files changed: 553 ins; 293 del; 176 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1951.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1951/head:pull/1951

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

[Jonathan Gibbons-2]

On Wed, 6 Jan 2021 00:12:04 GMT, Jonathan Gibbons <[hidden email]> wrote:

> Please review this change to centralize the management of HTML ids used by the standard doclet in a single new factory class, `HtmlIds`, which utilizes a new type-safe wrapper class around `String` called `HtmlId`.
>
> The new classes are used both when declaring ids (e.g. `HtmlTree.setId`) and when referencing them (e.g. `Links.createLink`).  The enum `SectionName`, which declared a set of standard ids, goes away, as do methods in `Links` and other places to create "anchors".
>
> This is a simple refactoring: there is no intentional change of functionality, and no tests are affected or need to be updated. However, the changes do highlight the inconsistency of the use of ids: it would be good to rationalize these in separate, targeted follow-up work.

This pull request has now been integrated.

Changeset: 916ab4e7
Author:    Jonathan Gibbons <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/916ab4e7
Stats:     1022 lines in 39 files changed: 553 ins; 293 del; 176 mod

8259283: use new HtmlId and HtmlIds classes

Reviewed-by: hannesw

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

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