On Dec 18, 2007, at 17:13, Vincent Hennebert wrote:
<snip />
Unfortunate, maybe, but I'm not sure why implementing an override
for a
method that contains behavior that is specific to instances of the
Wrapper class should be considered a hack...?
Because Wrapper is no longer agnostic, and contains an if statement
that
could be avoided. Whether text nodes are acceptable or not should be
determined in the same way as for validateChildNode, by deferring
to the
parent node.
If validateChildNode fails, then no addChildNode method is called;
that
should be the same for addCharacters IMO (call it addTextNode).
Actually, the equivalent of addChildNode() for FOText is more or less
what currently happens in FObjMixed.flushText().
The addCharacters() method does little more than record the
characters /so far/.
Reason is mainly that the SAX parser is not obliged to report an
uninterrupted block of text as one single instance of char[], so the
consolidation into a single FOText instance (or multiple instances,
should the text exceed Short.MAX_VALUE characters) happens ... at the
moment the /next/ child node is added, or the current node ends.
Because only when the parser has reported the next startElement() or
endElement() event, can we be absolutely certain that no more
characters() events will follow the current one.
FTM I'm trying to fit it in in the current design as cleanly as
possible.
Valid concern, sorry if I appeared rude.
Oh, don't worry, that was certainly not how it came across.
Still, it’s a hack, so it should either be removed, or temporarily
left as is
(even if it’s meant to stay in the code for years ;-) ) but
properly documented
as such.
A TODO comment, with basically the few lines you wrote to answer my
question, would do the job (and then I wouldn’t have asked the
question
:-P).
I’m certainly not against introducing hacks in the code, I’ve done
that
myself in a few places, but they should clearly appear as such IMO, so
that newcomers aren’t lost or misled.
Sure thing. I'll take it into account.
BTW: I'm currently doing some rather intensive cleanup and
improvement (I hope ;-)) of the javadocs in the fop.fo package and
subpackages, so will include it in that commit.
It's good to see that the question is raised, instead of my
changes being blindly accepted.
Well after all it’s an opportunity to explore the topic in more extent
:-)
Exactly! :-)
<snip />
Note, for instance, that there's a significant number of FOs that
allow
no children, and this leads to a lot of code-duplication, as they all
individually implement the method like:
protected void validateChildNode(...) throws ValidationException {
invalidChildError(..., "XSL Content Model: empty");
}
Actually this is more something like “invalidChildError(loc, nsURI,
localName);”, which is different (!), and which I’m happy with, see
below.
I was thinking in the direction of using a dummy marker interface, so
the duplicate overrides can all be removed, and consolidated in the
superclass.
For example:
public interface EmptyFObj { //empty body, only used as a marker
interface, like Cloneable or Serializable }
...
public class ExternalGraphic extends AbstractGraphics implements
EmptyFObj { ... }
...
in FObj.validateChildNode():
if (this instanceof EmptyFObj) {
invalidChildError(..., "XSL Content Model: empty");
}
While this is a very sensible solution, I’m a bit uncomfortable
with it.
This would put validation at two different levels: one in FObj, which
will test for every EmptyFObj; and one in each remaining non EmptyFObj
class, further down in the hierarchy.
No, the idea would be more that a call to super.validateChildNode()
would automatically take care of this particular aspect.
The only difference would be that, if the node is allowed to be non-
empty (i.e. does not implement the marker interface),
it does not even need to make this call... it can simply rely on its
own implementation. :-)
I'm more inclined, actually, to put this very basic validation as
high as possible in the class hierarchy.
Moreover an if test would now be performed for each FObj instance.
Nope, precisely not if the non-empty FOs properly implement their own
rules, and do not call super.validateChildNode().
It is only true that, /iff/ they rely on the superclass'
implementation, /then/ this test will be performed.
That should, normally, only be the case for all the empty FOs. If
they can have children, those will need to be validated.
It does leave room, though, for creating a FONode that implements the
empty-interface, and still overrides that method...
Then again, in standard Java one can also design an object like:
class CloneMeForFun implements Cloneable {
...
public CloneMeForFun clone(CloneMeForFun cmff) {
if (cmff.isInNoMoodToBeCloned) {
throw new CloneNotSupportedException(...);
}
}
}
That kills a bit the object-oriented design IMO. It seems more
consistent to me to rely on method
re-definition, after all this is the very purpose of object
inheritance.
Hmm... The purpose of inheritance to me is more: to rely on the
superclass implementation where possible, and implement specific
behavior only where needed.
Paraphrased: use method redefinition if necessary, and where it is
necessary, try to implement parts of the process in abstract
superclasses as much as possible, if they can be shared between
subclasses.
Actually the current situation looks fine to me, since the methods do
nothing but deferring to a common invalidChildError method (at
least all
the methods I’ve checked). So what can be factorised has already been
done.
Except for those methods containing nothing but that call... Duh! :-P
<snip />
Take:
<fo:block white-space-treatment="preserve">
**<fo:block-container>
****<fo:wrapper>
******<fo:block>
The *'s are preserved space-characters. The first two are allowed,
the
other six are, very strictly speaking an error (albeit, an easily
recoverable one).
(And what about the 4 in the middle?)
:-) Good point.
I don’t agree with your interpretation. I’d say:
- fo:block accepts PCDATA, so the first 2 stars will be recorded and
subject to further whitespace refinement (and appear to be
retained).
- fo:block-container doesn’t accept PCDATA, so the 4 stars in the
middle
will be silently ignored as XML whitespace characters resulting from
some pretty-printing process.
- the last 6 stars will be silently ignored as well, due to the
content
model of the wrapper’s parent; had this parent been an fo:block,
they
would have been recorded as text and be subject to further
whitespace
refinement (and retained).
WDYT?
Precisely what happens right now. Only, you can even add the whole
Bible in between <fo:wrapper> and <fo:block>, and all its characters
will simply dissipate without warning... :-)
Cheers
Andreas
