Discussion, cleanup; was Re: RFR: JDK-8241625 use new "member-list" CSS class instead of general "block-list" for list of members

[Jonathan Gibbons]

Hannes,

Thanks for the review.

There are various cleanup opportunities coming to mind.

  * Rename "factory" methods from `getSomething` to `newSomething`,
    `makeSomething` or `buildSomething`. The specific name TBD. Maybe we
    use `new...` for lower-level factory methods, and `build...` for
    higher-level methods that build something complex.

  * Many "build" methods build something and `add` it into a parameter.
    This is both stylistically weird and cause a lack-of-focus in some
    of the abstraction boundaries that I am seeing.  A better paradigm
    would be to just return the item that is built, and leave it to the
    caller to decide what to do with the item.

  * Maybe merge Builder and Writer classes, or at least the factories.
    This is *just* an idea at this point; I have not investigated the
    practicality. It's also unusual for me to consider merging
    classes/abstractions as compared to factoring them into separate
    abstractions, so I view this idea with a healthy amount of scepticism.

  * Introduce abstract subtypes of Content in the toolkit world that
    have impls in the formats.html world. Examples might be MemberList,
    DetailList, SummaryList, SummaryTable etc.  While this would
    increase the type-safety of many builder APIs, and reduce the
    prevalence of Content everywhere, it does mean another set of
    toolkit/formats.html interfaces/impls.  This idea is not unrelated
    to merging Builder and Writer, in the sense that we would be moving
    to different/better abstractions.

  * Whenever we rename of modify a method for any of the reasons given
    above, we should work to update the doc comment to be accurate and
    useful at the same time.

My sense is to do them individually, and separately from any changes 
that modify the generated code. In other words, I think it is better to 
go for smaller well-defined changes, and to note ideas that arise for 
making the code even better.

-- Jon

On 3/31/20 12:10 PM, Hannes Wallnoefer wrote:
> +1
>
> I just realised how much of an improvement this is by understanding how entailed the existing code is. The use of the getMemberTree method with hard-coded CSS class for very different purposes and the resulting HTML were pretty confusing. The new MemberWriter interface and reduction of per-writer methods is a nice improvement as well.
>
> Regarding naming of the new methods in MemberWriter: I think newMemberList and newMemberListItem might be slightly more fitting. But I think we have plenty of other get* methods that create new Content, so maybe we should address that in a separate changes? I’m fine with leaving it as is in the current webrev.
>
> Hannes
>
>> Am 28.03.2020 um 01:44 schrieb Jonathan Gibbons <jonathan.gibbons at oracle.com>:
>>
>> Please review the first in a series of steps to simplify the (re)use of the CSS class "block-list".
>>
>> There are two parts:
>>
>> 1. The first part is simple but moderately pervasive, but conceptually easy to review. The use of `class="block-list"` is removed from all <li> elements. There is a corresponding small change in the stylesheet so that
>>      ul.block-list li.block-list
>> is replaced by
>>      ul.block-list > li
>> The use of '>' ensure that the corresponding styles only apply to immediate children of `ul.block-list`.
>>
>>
>> 2. The second part is more complicated but less pervasive. Within each list of details sections (Field Details, Method Details, etc) there is a list of members. Currently, both the enclosing list and the member lists use `block-list`.  With the proposed change, the inner, nested lists are changed to use a new CSS class called `member-list`.  Part of the complication is that the same Java methods were being used to construct the <ul> and <li> elements for the two different types of list. (Obviously, different code was used to create the contents of the <li> elements.) So, the problem was to track down where the list and list items for the member lists were being created and to create and use new method calls instead of the shared method calls.
>>
>> The code sequences starts off in the builder for each of the different kinds of member elements: fields, constructor, method, etc), each of which calls code on the corresponding writer class. Somewhat surprising, there was no common supertype for these writers, so I've created and used a new interface MemberWriter that is a supertype of the different types of member writer. The code in the member WriterImpl leverages a shared impl in SubWriterHolderWriter that creates the new element nodes.  There's more than a little code-smell with SubWriterHolderWriter, including a side-ways cast, that I now understand, and will look to fix soon, but not here in this changeset.
>>
>> The new MemberWriter interface only has the two new methods in it for now, but I foresee the possibility of renaming and pulling up method definitions from the immediate subtypes.
>>
>> The new coding pattern should be the same for each of the kinds of member.
>>
>> I removed an unnecessary call in each writer that caused the builder to go through two steps instead of one.  These are methods with names like `get...Doc`
>>
>> The names of new methods deliberately emphasis their conceptual role:  getMemberList and getMemberListItem. Writing this description, I'd be open to starting to use a different (but otherwise standard) convention of newMemberList, newMemberListItem.
>>
>> The new member-list class uses the exact same definitions as block-list in the stylesheet.  Thus, the intent is that there should be no change in the direct visual appearance of each type declaration page.  We're just changing the new of the CSS class that is used.
>>
>> There are trivial changes to HelpWriter that are necessary until the parallel review to improve HelpWriter is pushed.
>>
>> There are some minor cleanups, most notably fixing @link references to BaseOptions.noComment() which were not updated when we automatically encapsulated the fields in BaseOptions.
>>
>> I've added a new test that for now tests the new member-list code.  As we improve other uses of block-list, I envisage this test being updated for additional new kinds of lists.
>>
>> --Jon
>>
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8241625
>> Webrev: http://cr.openjdk.java.net/~jjg/8241625/webrev.00/
>> API: http://cr.openjdk.java.net/~jjg/8241625/api.00/
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200331/eb2a882b/attachment-0001.htm>