Andreas L Delmelle wrote:
> On Dec 18, 2007, at 17:13, Vincent Hennebert wrote:
>
>>> 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().

There’s a difference (AFAIU): addChildNode won’t be called if 
validateChildNode failed. Whereas in the case of flushText validation is 
performed at the same time the text is added. It’s “too late”.


> 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,

Which IMO doesn’t prevent to perform validation at the addCharacters 
call.

<snip/>
> 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.

Great! :-)


<snip />
>>> 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.

This is already more or less the case with the invalidChildError method. 
I’d just add something like an acceptsEmptyContent method which would 
call invalidChildError with a more meaningful message (“Content model: 
empty”).


>> 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.

Then the EmptyFObj marker class wouldn’t be necessary, neither the if 
test in FObj.validateChildNode, if only empty FObjs make call to 
super.validateChildNode!?


> 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(...);
>     }
>   }
> }

Sorry, don’t follow you, what are you trying to explain here?


>> 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.

No: the abstract super-class implements only the bits common to /every/ 
sub-class. Moreover it must be sub-class agnostic: no “if this 
instanceof ThatParticularSubClass” test. Simply because if I (or even 
a non yet existing third party!) create a new sub-class, then I mustn’t 
have to change the abstract super-class to take this new class into 
account.

In the particular case of FObj, the validateChildNode method implemented 
in this class would apply /only/ to fobjs with empty content. This is 
wrong; either each and every class in the FObj hierarchy should make 
a call to super.validateChildNode, and add specific code in its own 
method, or none of it.
FObj is an abstract class: it defines method that each concrete 
sub-class must implement, and that method must remain empty if no 
behaviour can be extracted out of all of the concrete FObjs.

You’ll have to have very strong arguments to make me change my mind on 
this :-P


>> 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

Which is perfectly ok to me, we did everything we could do.
And what if there were /two/ widespread behaviours? Which one to choose?  
This is the case anyway, with all those objects which accept (%block;)+ 
as children.


>>> <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... :-)

And that’s what needs to be caught, because it’s not XML white space 
characters only. And that’s all, the rest can remain as is IMO.


Vincent


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

Reply via email to