Hi Jon, The changes look good to me. As I said in my previous review, the separation of method and CSS class use is a big improvement.
The html.*Writer classes still make be a bit uneasy. Browsing through the classes I found a number of unused methods, but that is not related to your changeset. I’ll file a new JBS issue for this. Hannes > Am 01.04.2020 um 02:27 schrieb Jonathan Gibbons <jonathan.gibb...@oracle.com>: > > Please review the next in the series to clean up the use of "block-list" CSS > class. > > The previous review (JDK-8241625) handled the nested lists for the details > for members of a given kind. This review is for the enclosing list ... the > list of nested lists for members of a given kind. The CSS class is changed > from "block-list" to "details-list" > > The mechanism and code flow is much the same as in the previous review. > Starting with the builder class for each kind of member, a new method is used > to create a new "details list". That is passed down into the writer and > writer-impl, where eventually a call of `getMemberTree` is replaced by a call > to another new method to create a new "details list item". I've taken the > opportunity to restructure the code a bit when creating the details list > item, but the content of the details list item should be exactly the same as > before. > > Notes: > > I don't like the fact that the call to create and add a "details list item" > is so far removed from the call to create a "details list". This makes me > want to have some of these builder methods return what they build instead of > add it into a container. Maybe soon... > > In WriterFactoryImpl, a cast has been changed from (SubWriterHolderWriter) to > an appropriate WriterImpl class. The old cast was a "sideways cast", which > was somewhat weird. The new cast is a cast down to the impl subtype, but is > then widened to SubWriterHolderWriter inside the constructor that is called. > The use of SubWriterHolderWriter is still a bit weird, but is now a bit less > weird. The IDE seems to be happier as well, and better able to locate impls > of some of the interface methods. > > In the Builder classes, I replaced a "skip if null" check from various > methods with a defensive Objects.requireNonNull call in the constructor. > > The names of the new CSS classes can be thought of as placeholders. The point > is that they are new unique names, that can be easily changed if and when we > decide to open up the naming bikeshed. > > I don't know if the benefits outweigh the "complexity", but one possibility > is for these lists (member list, detail list, etc) to use an abstract subtype > of Content such that adding items to the list would automatically wrap them > in <li> elements. That would cut down on some of the new methods being added, > since they would become instance methods on this new abstract type of content. > > Coming soon: > > Summary lists? > Tables? > > -- Jon > > JBS: https://bugs.openjdk.java.net/browse/JDK-8241895 > Webrev: http://cr.openjdk.java.net/~jjg/8241895/webrev.00/ > API: http://cr.openjdk.java.net/~jjg/8241895/api.00/ >
