+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.gibb...@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/ >
