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

Reply via email to