Hi Andreas,

Andreas L Delmelle wrote:
> On Dec 18, 2007, at 13:17, Vincent Hennebert wrote:
> <snip />
>>>> IIUC, this check isn’t necessary? If test children weren’t allowed this
>>>> would have been caught by the validateChildNode method?
>>> No, because validateChildNode() is not called for text-nodes.
>> Hmmm, this is unfortunate IMO. That makes validation unreliable, and
>> that forces to implement hacks like in Wrapper.
> 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).

> FTM I'm trying to fit it in in the current design as cleanly as
> possible.

Valid concern, sorry if I appeared rude. 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 
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.

> 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 

> Then again, as I indicated earlier, something seems fishy/awkward about
> hardcoding the validation rules in the FOs, since this leaves too little
> room for adding extensions.

That can be discussed independently in my opinion, so I won’t enter the 
details too much. Basically validateChildNode should only deal with 
elements from the XSL-FO namespace. The validation of foreign element 
belongs to another process that will indeed have to be discussed 
somehow, somewhere, at some moment ;-)

> IMO, we need some way of registering valid child nodes at runtime. The
> default rules in the Recommendation can be fixed at compile time, but
> there is also still room for improvement.
> 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 

> 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. Moreover an if test would now be 
performed for each FObj instance. 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.

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 

The ideal solution is probably to use object composition, with 
a delegate object dedicated to validation. FObjs would be passed the 
appropriate object at their creation. We would have 
a NoChildAllowedValidator, a MixedContentValidator, etc. Then only one 
NoChildAllowedValidator instance would be needed for every FObj with 
empty content. And for those FObjs which have a particular content 
model, we could use an anonymous class etc... A bit overkill maybe ;-), 
that’s why I’m happy with the current situation as long as nothing more 
is done than calling a common factorised method.

>>> One could argue that the default characters() event should also do
>>> some
>>> form of validation, but that could turn out to be very tricky. The SAX
>>> parser is obliged to report *all* characters (including possible
>>> ignorable whitespace). We would already need to perform
>>> space-normalization here to make sure there really are stray characters
>>> in places where they do not belong...
>> Well it’s not the same kind of normalization as required by the
>> Recommendation. For objects accepting mixed content we would accept any
>> kind of text node, and the proper normalization would indeed be done
>> later.
>> For the other objects we just need to know if the text node contains XML
>> whitespace only (space, tab, line-feed, carriage-return). Something like
>> String.trim().length() == 0 should be enough for the validation. WDYT?
> Hmm... but in some cases, the white-space could be significant
> (depending on the FO properties
> white-space-treatment/white-space-collapse/linefeed-treatment of the
> containing block, if any).
> 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?)
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).


Vincent Hennebert                            Anyware Technologies
http://people.apache.org/~vhennebert         http://www.anyware-tech.com
Apache FOP Committer                         FOP Development/Consulting

Reply via email to