RFR: JDK-8190295: Introduce a new Table builder class
Bhavesh Patel
bhavesh.x.patel at oracle.com
Thu Nov 16 06:21:56 UTC 2017
Very good refactoring. Ok to push. Just a few minor suggestions. Also
there could be some merge conflicts as I pushed the multiple stylesheet
fix and I had done a few minor updated that were suggested. The patch
that is applied to this fix does not have those changes e.g. In
standard.properties <path> has been updated to <file>.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractIndexWriter.java*
Line 434: Should we move the prefix "I:" to Links.java and have a set
method there? Then we can check in the getName() if the prefix needs to
be generated. We have a similar prefix in Table.java so it would be
consistent to handle it this way.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AbstractMemberWriter.java*
Line 106: Do we need this anymore? Similar to getCaption() thats removed
here. See comment on AnnotationTypeFieldWriterImpl.java
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/AnnotationTypeFieldWriterImpl.java
*Lines 207 - 210: Seems to be redundant/dead code. If you look at lines
224 - 226, we set the summary there. Or else we can remove lines 224
-226 and update line 233 to .setSummary(getTableSummary); as we have
done in various other WriterImpl classes e.g.
AnnotationTypeRequiredMemberWriterImpl.java, ConstructorWriterImpl.java,
FieldWriterImpl.java etc. Same case with the caption as well.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/EnumConstantWriterImpl.java
*See comment on AnnotationTypeFieldWriterImpl.java. Inconsistency across
WriterImpl classes.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlConfiguration.java
*Live 250: Typo. 2 periods.*
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ModuleWriterImpl.java
*Lines 466 & 485: param name "text" does not match the parameter in the
method i.e. "caption".
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/PropertyWriterImpl.java*
See comment on AnnotationTypeFieldWriterImpl.java. Inconsistency across
WriterImpl classes*.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/SourceToHTMLConverter.java
*Lines 215 & 217: Commented out. Should it be removed?*
*******
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Table.java*
- The copyright year should just be 2017 as its a new file.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Head.java**
*
- The copyright year should just be 2017 as its a new file.
- Need a newline after line 78 to separate documentation comments from tags.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Links.java*
- The copyright year should just be 2017 as its a new file.
*src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/markup/Script.java
*- The copyright year should just be 2017 as its a new file*.
*Regards,
Bhavesh.*
**
*
On 11/6/2017 6:17 PM, Jonathan Gibbons wrote:
> Kumar,
>
> Thank you for your feedback. I've made the changes you suggested, and
> included the proposal
> as part of a series of related changes to generally cleanup more of
> the same.
>
> The overall webrev is now at
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/
>
> which contains a series of webrevs as follows:
>
> https://bugs.openjdk.java.net/browse/JDK-8190295
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.tables/index.html
>
> changeset: 47482:d222b617b9d2
> tag: tables
> user: jjg
> date: Mon Nov 06 17:17:07 2017 -0800
> summary: 8190295: Introduce a new Table builder class
>
> https://bugs.openjdk.java.net/browse/JDK-8190818
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.scripts/index.html
>
> changeset: 47483:875dd666843f
> tag: scripts
> user: jjg
> date: Mon Nov 06 17:18:34 2017 -0800
> summary: 8190818: Introduce a new Script builder class
>
> https://bugs.openjdk.java.net/browse/JDK-8190819
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.htmlDocument/index.html
>
> changeset: 47484:aaf2675c4f4b
> tag: htmlDocument
> user: jjg
> date: Mon Nov 06 17:19:06 2017 -0800
> summary: 8190819: Merge HtmlWriter into HtmlDocument
>
> https://bugs.openjdk.java.net/browse/JDK-8190820
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.head/index.html
>
> changeset: 47485:ca0c08593f7a
> tag: head
> user: jjg
> date: Mon Nov 06 17:19:28 2017 -0800
> summary: 8190820: Introduce a new Head builder class
>
> https://bugs.openjdk.java.net/browse/JDK-8190821
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.links/index.html
>
> changeset: 47486:3e139d148884
> tag: links
> user: jjg
> date: Mon Nov 06 17:19:47 2017 -0800
> summary: 8190821: Introduce a new Links builder class
>
> https://bugs.openjdk.java.net/browse/JDK-8190822
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.linkInfoImplStylename/index.html
>
> changeset: 47487:762be115f89a
> tag: linkInfoImplStylename
> user: jjg
> date: Mon Nov 06 17:20:49 2017 -0800
> summary: 8190822: Remove dead code that could lead to invalid HTML
>
> https://bugs.openjdk.java.net/browse/JDK-8190824
> http://cr.openjdk.java.net/~jjg/8190295-etc/webrev.00/webrev.htmldocwriter/index.html
>
> changeset: 47488:c09c658d94b3
> tag: htmldocwriter
> tag: qtip
> tag: tip
> user: jjg
> date: Mon Nov 06 17:21:00 2017 -0800
> summary: 8190824: Eliminate HtmlDocWriter
>
> The first webrev (for Table.java) is the biggie. The others are all
> much simpler.
>
> -- Jon
>
> On 11/06/2017 01:48 PM, Kumar Srinivasan wrote:
>>
>> Hi Jon,
>>
>> This is an enormous improvement and simplification!. Thanks!
>>
>> Table.java:
>>
>> 304 * <li>The prefix should probably be a value such that the
>> genenrated ids cannot
>>
>> s/genenrated/generated/
>>
>>
>> The previous block is ternary operation, would make it nicer if this
>> is also so.
>>
>> HtmlTree cell;
>> if ((colIndex) == rowScopeColumnIndex) {
>> cell = HtmlTree.TH(cellStyle, "row", c);
>> } else {
>> cell = HtmlTree.TD(cellStyle, c);
>> }
>>
>> final HtmlTree cell = HtmlTree.TH(cellStyle, "row", c)
>> ? HtmlTree.TH(cellStyle, "row", c)
>> : HtmlTree.TD(cellStyle, c);
>>
>>
>>
>> AbstractMemberWriter.java
>>
>> + TypeElement te =
>> utils.getEnclosingTypeElement(element); // is this different from
>> typeElement?
>>
>> I ran a quick test, and encountered several test failures, the reason
>> is ClassUseWriter uses this method,
>> and it creates AbstractMemberWriter with typeElement = null, via
>> public AbstractMemberWriter(SubWriterHolderWriter writer) {
>> this(writer, null);
>> }
>> and thus typeElement could be a null, the following might save some
>> cpu cycles,
>> I verified all tests pass and the comparison tests also pass.
>>
>> + TypeElement te = typeElement == null
>> + ? utils.getEnclosingTypeElement(element)
>> + : typeElement;
>>
>>
>> There were other details we discussed offlist, and that clarifies the
>> questions I had,
>> regarding Table.java. Most of this is in Bhavesh's expertise, I will
>> leave it to him.
>>
>> Kumar
>>
>>
>>> Please review the following change, to replace the code to create
>>> HTML tables with a new
>>> builder-style Table class. With the exception of some minor
>>> differences (bug fixes) on
>>> the deprecated-list page, the generated output should be unchanged
>>> for now, although
>>> we may want to remove some backwards-compatibility hacks at some
>>> point in the future
>>> to make all tables be more uniform when appropriate.
>>>
>>> Before reading the detailed changes in the modified files, you might
>>> want to start by
>>> reviewing the new Table class.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8190295
>>> Webrev: http://cr.openjdk.java.net/~jjg/8190295/webrev.00/
>>>
>>>
>>> Notes:
>>>
>>> (Various files)
>>> Stuff that has been deleted has generally moved in some form or
>>> other into the new Table class
>>> or otherwise made redundant.
>>>
>>> AbstractMemberWriter:118-138
>>> Previously, the state defining the table was indirectly managed in
>>> the MemberSummaryBuilder
>>> class via tableContents and counter (uugh.) Now, the state is kept
>>> in the AbstractMemberWriter
>>> and lazily created by getSummaryTable, line 120.
>>>
>>> AbstractMemberWriter:433-437
>>> Here's a relatively simple example of creating a Table, new-style.
>>> The table is created (433-437), rows are added (463) and the HTML
>>> generated (465)
>>> Other Tables are created in *WriterImpl classes.
>>>
>>> AbstractMemberWriter: old lines 535-553
>>> Anything to do with TableTabs is now mostly hidden inside the Table
>>> class, driven by
>>> addTab calls when the Table is initialized.
>>>
>>> AbstractMemberWriter:583-592
>>> The lines are called by the MemberSummaryBuilder to conclude its
>>> work on the table.
>>>
>>> DeprecatedListWriter:
>>> There is a small, deliberate (bug fix?) change in behavior here.
>>> Previously, modules
>>> in the deprecated list used the colFirst style, and other types used
>>> colDeprecatedItemName.
>>> Now, everything uses the same style, colDeprecatedItemName. It
>>> didn't seem worth
>>> fixing for the old bad behavior. Also note that some cells were
>>> omitted from the table,
>>> causing inconsistent visual appear of the striping. That is also fixed.
>>>
>>> HtmlDocletWriter:2548
>>> This is made available, for now. The script item itself is in
>>> HtmlWriter (uugh). Long term,
>>> this should move into a new Head class (builder for <head> element)
>>>
>>> MethodWriterImpl.java:258-278
>>> This is definitely the most exciting part of this work. This is the
>>> client-side replacement for
>>> all the previous code to manage table tabs. Now, each tab is just a
>>> method call with a
>>> name and a predicate.
>>>
>>> ModuleIndexWriter.java:135-141
>>> Following on from the previous comment, here in ModuleIndexWriter,
>>> and corresponding code
>>> in PackageIndexWriter, we have a dynamically added set of tabs,
>>> based on command line options.
>>>
>>> ModuleWriterImpl.java:635-648
>>> For now, for compatibility the rows are added in the same order as
>>> before.
>>> Going forward, we might want to add the rows in overall sorted order.
>>>
>>> PropertyWriterImpl.java:234-245
>>> Because we create differently configures tables for properties and
>>> methods, one without tabs, one with,
>>> it falls out of the design that the rows in the properties table
>>> don't get "ids", thus avoiding the earlier
>>> "duplicate id" problem.
>>>
>>> Table.java:
>>> The new class that does all the work. It's reasonably well
>>> commented, so nothing to add here.
>>>
>>> -- Jon
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20171115/f658718f/attachment-0001.html>
More information about the javadoc-dev
mailing list