Before getting into the code details, I will say how much I like the
resulting UI result!
On 11/14/2019 09:04 AM, Hannes Wallnöfer wrote:
I agree, calling something *Content that doesn’t extend Content is not great.
I actually considered „Body“ and „BodyBuilder“, but the former doesn’t fit because
the generated content does not include the <body> tag (which is generated
elsewhere and dependent on page type), and the latter has that annoying connotation
of people in undies flexing their muscles.
Yeah, I had the same issue with "BodyBuilder" :-) As for "Body", my
initial reaction is, why isn't this a builder for the <body> tag?
What do you think about BodyContents (plural) or BodyContentBuilder?
For now, I guess I'd suggest to go with BodyContents, but I do like the
idea that maybe down the road, we evolve this into Body, and have it
take care of the complete overall high level structure of the page, as
represented by a <body> tag. There's not much else involved ... see
HtmlDocletWriter.getBody. And now that frames have gone, I'm not sure
we need scripts to set the window title, etc. Also, I note that unless
we separately choose to get rid of the script stuff, evolving
BodyContents into Body should just be a code refactoring, without
related changes to any tests, which would significantly reduce the cost
of the refactoring.
Regarding the unused fluency: yes, it’s superfluous and should probably be
removed.
I guess I like using fluency when it is reasonable to do so, so I would
tend to go the other way: retain it and use it.
-------
In addition,
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.
What's wrong with Navigation? IMO, it is broken in a number of ways.
(These are general criticisms, mostly not specific to your changes.)
a. It's too high level to be a mid-level builder in the formats.html
package. There's way too much use of "configuration", and way too much
page-specific knowledge, embedded in the use of PageMode and switch
statements performing different actions based on the PageMode.
Instead, it should provide lower-level building blocks, and the
page-specific behavior should be replaced by (fluent?) calls to
customize the properties when the page-specific instance is created.
Most uses of "configuration" as well as the PageMode enum could/should
be eliminated from the abstraction.
b. The class normally generates a single <nav> element. IMO this is an
abuse of <nav> which should just be a container for a series of links.
As the code stands now, not only does it contain all the links, but it
also contains title text, the search box, and (with your change) the top
text. At some point, it should be rewritten to generate a <div>, which
may in turn contain <nav> elements surrounding the lists of navigation
links. You could persuade me that we need a different/better name, but
given it is used for both the header and footer, neither `Header` nor
`Footer` is a reasonable name. `Banner` maybe?
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.
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.
-----
Should the various declarations of bodyContent in the various subtypes
of HtmlDocletWriter be replaced by a single protected declaration in
HtmlDocletWriter itself?
-----
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.
-----
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.
----
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.
-- Jon
