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 <[email protected]>:
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/
