On Mon, 11 Jan 2021 23:55:16 GMT, Jonathan Gibbons <[email protected]> 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
