JDK-8259283: use new HtmlId and HtmlIds classes #1951

jonathan-gibbons · 2021-01-06T00:12:04Z

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.

Progress

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

Issue

JDK-8259283: use new HtmlId and HtmlIds classes

Reviewers

Hannes Wallnöfer (@hns - Reviewer) ⚠️ Review applies to 2ddcf13

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1951/head:pull/1951
$ git checkout pull/1951

bridgekeeper · 2021-01-06T00:12:50Z

👋 Welcome back jjg! 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-01-06T00:13:52Z

@jonathan-gibbons The following label will be automatically applied to this pull request:

javadoc

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

mlbridge · 2021-01-06T01:18:07Z

Webrevs

…lve 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

hns

Very nice work, Jon! +1

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

hns · 2021-01-13T10:53:30Z

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

     */
-    public String getName(String name) {
-        return name.replaceAll("\\s+", "");
+    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.

hns · 2021-01-13T10:57:52Z

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

@@ -299,16 +298,21 @@ public void addRow(Element element, List<Content> contents) {
            int rowIndex = bodyRows.size();
            rowStyle = stripedStyles.get(rowIndex % 2);
        }
-        Set<String> tabClasses = new HashSet<>();
+        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.

hns · 2021-01-13T15:00:49Z

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

@@ -71,9 +72,10 @@
    HtmlIndexBuilder(HtmlConfiguration configuration) {
        super(configuration, configuration.getOptions().noDeprecated());
        this.configuration = configuration;
-        links = new Links(DocPath.empty, configuration.utils);
+        links = new Links(DocPath.empty);


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

OK, will check.

hns · 2021-01-13T15:03:17Z

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

+ *
+ * @see HtmlTree#setId(HtmlId)
+ */
+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.

openjdk · 2021-01-13T16:05:24Z

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

8259283: use new HtmlId and HtmlIds classes

Reviewed-by: hannesw

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

jonathan-gibbons · 2021-01-13T17:01:15Z

/integrate

openjdk · 2021-01-13T17:02:04Z

@jonathan-gibbons Pushed as commit 916ab4e.

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

JDK-8259283: use new HtmlId and HtmlIds classes

25dd9a7

openjdk bot added the javadoc javadoc-dev@openjdk.org label Jan 6, 2021

fix whitespace; add doc comments

e8a1e07

openjdk bot added the rfr Pull request is ready for review label Jan 6, 2021

jonathan-gibbons added 2 commits January 11, 2021 14:31

tidy up merge

2ddcf13

hns approved these changes Jan 13, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 13, 2021

jonathan-gibbons added 2 commits January 13, 2021 08:36

Merge remote-tracking branch 'upstream/master' into htmlids

0ed24cd

Address review comments

24525ab

openjdk bot closed this Jan 13, 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 Jan 13, 2021

jonathan-gibbons deleted the htmlids branch January 13, 2021 17:01

JDK-8259283: use new HtmlId and HtmlIds classes #1951

JDK-8259283: use new HtmlId and HtmlIds classes #1951

jonathan-gibbons commented Jan 6, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jan 6, 2021

openjdk bot commented Jan 6, 2021

mlbridge bot commented Jan 6, 2021 •

edited

hns left a comment

hns Jan 13, 2021

jonathan-gibbons Jan 13, 2021

hns Jan 13, 2021

jonathan-gibbons Jan 13, 2021

hns Jan 13, 2021

jonathan-gibbons Jan 13, 2021

hns Jan 13, 2021

jonathan-gibbons Jan 13, 2021

openjdk bot commented Jan 13, 2021 •

edited

jonathan-gibbons commented Jan 13, 2021

openjdk bot commented Jan 13, 2021

JDK-8259283: use new HtmlId and HtmlIds classes #1951

JDK-8259283: use new HtmlId and HtmlIds classes #1951

Conversation

jonathan-gibbons commented Jan 6, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 6, 2021

openjdk bot commented Jan 6, 2021

mlbridge bot commented Jan 6, 2021 • edited

Webrevs

hns 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

openjdk bot commented Jan 13, 2021 • edited

jonathan-gibbons commented Jan 13, 2021

openjdk bot commented Jan 13, 2021

jonathan-gibbons commented Jan 6, 2021 •

edited by openjdk bot

mlbridge bot commented Jan 6, 2021 •

edited

openjdk bot commented Jan 13, 2021 •

edited