RFR: JDK-8190295: Introduce a new Table builder class
Bhavesh Patel
bhavesh.x.patel at oracle.com
Thu Nov 16 20:02:45 UTC 2017
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
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20171116/e0d56fb0/attachment-0001.html>
More information about the javadoc-dev
mailing list