RFR: JDK-8190295: Introduce a new Table builder class

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: JDK-8190295: Introduce a new Table builder class

Jonathan Gibbons
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

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190295: Introduce a new Table builder class

Kumar Srinivasan

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
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190295: Introduce a new Table builder class

Jonathan Gibbons
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
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190295: Introduce a new Table builder class

Bhavesh Patel

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




Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190295: Introduce a new Table builder class

Jonathan Gibbons
Bhavesh,

Responses inline.



On 11/15/2017 10:21 PM, Bhavesh Patel wrote:

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.

Links is a singleton factory class, so although we could set a prefix string in the class, we wouldn't want it to apply to all anchors and links, meaning we would have to add extra overloads to say whether the prefix should be used or not. Moreover, this is very specific to the index world, so I'm inclined to leave it in the AbstractIndexWriter, at least for now.



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

I may have a go at trying to simplify this, (again?)  I seem to recall trying this before and there were some places where the method was being used from multiple places, but that may have been the table header (getTableHeader()) and not the summary. If I can get a build/test/compare to work, I'll do this; otherwise I will leave this for later



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.

Will fix


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".

Will fix


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?

No, these are left in for phase 2, which will be to make all tables more consistent.
Right now, I'm placing greater emphasis on reducing change to generated output.


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.

OK.


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





Reply | Threaded
Open this post in threaded view
|

Re: RFR: JDK-8190295: Introduce a new Table builder class

Bhavesh Patel


Ok. Thanks Jon. Good to be pushed.


On 11/16/2017 12:00 PM, Jonathan Gibbons wrote:
Bhavesh,

Responses inline.



On 11/15/2017 10:21 PM, Bhavesh Patel wrote:

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.

Links is a singleton factory class, so although we could set a prefix string in the class, we wouldn't want it to apply to all anchors and links, meaning we would have to add extra overloads to say whether the prefix should be used or not. Moreover, this is very specific to the index world, so I'm inclined to leave it in the AbstractIndexWriter, at least for now.



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

I may have a go at trying to simplify this, (again?)  I seem to recall trying this before and there were some places where the method was being used from multiple places, but that may have been the table header (getTableHeader()) and not the summary. If I can get a build/test/compare to work, I'll do this; otherwise I will leave this for later



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.

Will fix


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".

Will fix


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?

No, these are left in for phase 2, which will be to make all tables more consistent.
Right now, I'm placing greater emphasis on reducing change to generated output.


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.

OK.


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