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 |
> 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 |
In reply to this post by 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 |
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 |
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 |
In reply to this post by Hannes Wallnöfer
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 |
In reply to this post by 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 |
In reply to this post by 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 |
Free forum by Nabble | Edit this page |