On Jun 21, 2008, at 18:10, [EMAIL PROTECTED] wrote:
Author: acumiskey
Date: Sat Jun 21 09:10:03 2008
New Revision: 670217
URL: http://svn.apache.org/viewvc?rev=670217&view=rev
Log:
Moved the contents of TableBody into a new abstract base class
TablePart, which is now subclassed by TableBody, TableHeader and
TableFooter.
In the meantime, had a closer look at the changes, and had a few tiny
remarks.
Rather than coming across as simply nagging, I already committed the
changes as well (see: http://svn.apache.org/viewvc?rev=670323&view=rev)
I moved getLocalName() and getNameId() from TablePart back to
TableBody, where they seem to belong...
At least, TableBody then contains /some/ behavior justifying its
existence. ;-)
<snip />
Added: xmlgraphics/fop/trunk/src/java/org/apache/fop/fo/flow/table/
TablePart.java
URL: http://svn.apache.org/viewvc/xmlgraphics/fop/trunk/src/java/
org/apache/fop/fo/flow/table/TablePart.java?rev=670217&view=auto
======================================================================
========
<snip />
+
+ /** [EMAIL PROTECTED] */
+ public void startOfNode() throws FOPException {
+ super.startOfNode();
+ if (this instanceof TableHeader) {
+ getFOEventHandler().startHeader((TableHeader)this);
+ } else if (this instanceof TableFooter) {
+ getFOEventHandler().startFooter((TableFooter)this);
+ } else {
+ getFOEventHandler().startBody((TableBody)this);
+ }
+ }
No idea if this was already there before your changes, but I'm not
sure I like this pattern.
Makes me wonder: Why have subclasses, if the superclass handles
everything through instanceof checks? Superclasses are meant to
contain common behavior, and the generated events are particular to
the subclass here, so not really common. The only thing they have in
common is that they generate /an/ event... Suppose at some point we
have a new TablePart subclass, then we would --alter the superclass'
startOfNode() to generate its event (?)
I have changed this so TableBody, TableHeader and TableFooter each
have their own startOfNode() which triggers the appropriate event in
the FOEventHandler, after calling super.startOfNode().
Same for endOfNode(), although there, it was even stranger.
In TablePart, we have:
+
+ /** [EMAIL PROTECTED] */
+ public void endOfNode() throws FOPException {
+ if (!inMarker()) {
+ pendingSpans = null;
+ columnNumberManager = null;
+ }
+
+ if (this instanceof TableHeader) {
+ getFOEventHandler().endHeader((TableHeader)this);
+ } else if (this instanceof TableFooter) {
+ getFOEventHandler().endFooter((TableFooter)this);
+ } else {
+ getFOEventHandler().endBody((TableBody)this);
+ }
+
+ if (!(tableRowsFound || tableCellsFound)) {
+ missingChildElementError("marker* (table-row+|table-
cell+)", true);
+ getParent().removeChild(this);
+ } else {
+ finishLastRowGroup();
+ }
+ }
and in TableHeader and TableFooter, we have overrides for endOfNode()
that basically do the same thing as the last few lines, and don't
call super.endOfNode(), making the check redundant.
I kept the overrides, but changed them to trigger the endHeader()/
endFooter() events after calling super.endOfNode(). The corresponding
if-else branches have been removed from both methods.
Apart from these small oversights, the intention is definitely
appreciated!
Cheers
Andreas
