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 via email to