Thanks for the review, Jon. I uploaded a new webrev.
http://cr.openjdk.java.net/~hannesw/8223378/webrev.03/
Here’s an overview of changes compared to the previous webrev:
- BodyContent has been renamed to BodyContents, and
- Now uses „fluent“ BodyContents API wherever it makes sense. Instead of
local variable declaration most MainContents are now created, built, and
consumed in-place where they are needed.
- „top" content is added to a ContentBuilder before Navigation content instead of
putting it inside the <nav>. There is also a new test to check presence of „top“
content with -nonavbar option (more details below)
- Removed unused contentTree fields and parameters
- There were a few cases where I passed a <main> tree to BodyContents.addMainContent.
This resulted in duplicate <main> elements because BodyContents.toContent generates its own
<main>. These are fixed now.
- Added the bug-id for this change to the changed tests
More details on some of these changes inlined below.
Am 22.11.2019 um 02:20 schrieb Jonathan Gibbons <[email protected]>:
I think its wrong to move the "top" content into the Navigation class. It doesn't help
that the Navigation class is not well designed in the first place and needs to be overhauled
anyway. Instead of moving "top" into Navigation, I think there are a couple of
possibilities.
1. Add a new Header class for the header of the page, containing the top text
and the content of the Navigation object.
2. #1 may be overkill, in which case you could extend/generalize your new BodyContent(s) class so that instead of providind
"setHeader"/"setFooter", you provide "addHeaderContent", "addFooterContent". Then the
code to "addTop" into the Navigation object becomes a pair of calls to add the top into the BodyContents header,
followed by the contents of the Navigation object.
3. Create a ContentBuilder. Call "addTop" to put the top content into that
content builder, then add Navigation.toContent() into the content builder, and finally
set the content builder as the header of the bodyContents. Of these 3 suggestions, I
think I recommend this one.
I followed suggestion 3 as described above.
c. There is silliness in the code. Look at the code-flow in Navigation.toContent.
Content contentTree = new
ContentBuilder();
if (!configuration.nonavbar
) {
// compute tree
return
tree;
}
return
contentTree;
This could be simplified/improved.
I fixed just this particular issue by immediately returning a new
ContentBuilder if the -nonavbar option is enabled.
d. By moving the "top" text into Navigation, you have made it subject to the
`-nonavbar` option, embodied by `configuration.nonavbar`. That's an inappropriate change
in behavior. The top text is typically used for legal info and should not be hidden if
`-nonavbar` is specified.
Actually „top“ content was already dependent on the navbar in the current code,
because HtmlDocletWriter.addTop ignored its htmlTree argument and added the top
text to fixedNavDiv, which was in turn used by/embedded in the navbar. This is
now fixed as described above, and I also added a test
TestTopOption.testNoNavbar for it (fails with current code).
Should the various declarations of bodyContent in the various subtypes of
HtmlDocletWriter be replaced by a single protected declaration in
HtmlDocletWriter itself?
Not all HtmlDocletWriter subclasses need/use a field for BodyContents. Having
just one declaration might make the code simpler and more uniform. For now I’ve
left it as it was because I don’t want this change to become too big, but it’s
something we should consider for future cleanup.
I noticed the following on PackageSummaryWriter/PackageWriterImpl, but there
may be equivalent instances on other pages as well. Look at the changes
towards the end of the file, line 295, 307.
The parameter "contentTree" is now unused. You've changed the semantics of the method from
"add the 2nd argument to the 1st argument" to "add the 2nd argument to a class member".
That in itself is not wrong, but it is not the semantics documented in the overriden method. One way or
another, you should align the spec/impl of the super method and the impl method. That being said, I think
this ties into some similar related issues, with respect to other add... methods and the use of the
page-global bodyContent member.
There were a few cases of this passing around unused arguments, and actually it
was quite a bit worse than that: Those classes had a contentTree field that was
passed uninitialised to the main build* method, where the return value of the
get*Header method was assigned to the parameter, which was then passed on as
contentTree. Basically the field was never assigned or used.
I fixed these classes by removing the field and parameter declarations, and
only passing contentTree to the methods that actually use it, which is mostly
printDocument.
I noticed this on SerializedFormWriterImpl.java; it may apply to other pages as
well. You've changed the impl of printDocument, which is supposed to (just)
print the tree that is provided as an argument. It seems wrong to be adding in
bodyContent at this point, and related to the previous comment about adding
nodes into page-global variables instead of the given arguments.
It’s not ideal to add the BodyContents in printDocument, but it’s the safest
thing to do with the current code layout. If we add the BodyContents before it
is fully filled some part will be missing in the resulting document. By adding
it just before generating the document we are sure to have the full contents.
Not great, but think it’s acceptable for now.
OK, now I see a root cause of the problems and why you have done some of what
you have done. I see that the issue is the historical split between the
format-neutral toolkit world and the formats.html world ... and that
`BodyContent` is a builder but not a subtype of Content. The problem is
exacerbated somewhat by the inconsistent asymmetry between the use of `get`
methods that return Content and `add` methods that take a Content as an
argument.
Temporary Summary:
I don't have a good solution/suggestion at this point. I need to think about
this. I'm sure we don't want to get into an big refactoring of the
Builder/Writer/Impl APIs, but equally, it seems difficult to conform to those
APIs as specified.
That being said, I'm beginning to see a way forward in which the API is biased more towards to
imperative `void addXyx(...)` style, at least at the outer levels of the page. Taking
PackageWriterImpl as an example, look at getPackageHeader. It too does not really conform to
its declared specification. It creates a mostly empty `bodyTree`, then stashes most everything
in member variables for `navBar` and `bodyContent` and finally returns the empty `bodyTree` ...
which is not the package header implied by the name of the method ... and then eventually,
there's a second sleight-of-hand when the `bodyContent` is surreptiously slipped into the
mostly empty body at the last minute in `printDocument`. Perhaps we should really go
"all-in" on side-effect methodology and have *WriterImpl just build up
<body>/bodyContent/navBar in member variables. In other words, use one instance of (a
subtype of) HtmlDocletWriter per page, which we already do, building up member variables for
the page, resulting in a final parameter-less call of `printDocument()` when the content of the
page has finally been prepared.
I agree we should do a cleanup of the Builder/Writer code structure, but that’s
not for 14.
This change doesn’t really fix all things that should be fixed, but at think it
slightly improves some parts of it and doesn’t make anything worse.
Hannes
