You are viewing a plain text version of this content. The canonical link for it is here.
Posted to fop-commits@xmlgraphics.apache.org by je...@apache.org on 2005/05/12 16:13:45 UTC
cvs commit: xml-fop/src/java/org/apache/fop/layoutmgr/table TableStepper.java TableContentLayoutManager.java EffRow.java
jeremias 2005/05/12 07:13:45
Modified: src/java/org/apache/fop/layoutmgr/table Tag:
Temp_KnuthStylePageBreaking TableStepper.java
TableContentLayoutManager.java EffRow.java
Log:
Fix for ArrayIndexOutOfBoundsException when empty grid units are involved.
Convenience accessor for GridUnits on EffRow.
Revision Changes Path
No revision
No revision
1.1.2.3 +2 -2 xml-fop/src/java/org/apache/fop/layoutmgr/table/Attic/TableStepper.java
Index: TableStepper.java
===================================================================
RCS file: /home/cvs/xml-fop/src/java/org/apache/fop/layoutmgr/table/Attic/TableStepper.java,v
retrieving revision 1.1.2.2
retrieving revision 1.1.2.3
diff -u -r1.1.2.2 -r1.1.2.3
--- TableStepper.java 12 May 2005 13:52:15 -0000 1.1.2.2
+++ TableStepper.java 12 May 2005 14:13:45 -0000 1.1.2.3
@@ -81,7 +81,7 @@
}
private GridUnit getActiveGridUnit(int column) {
- return (GridUnit)getActiveRow().getGridUnits().get(column);
+ return getActiveRow().getGridUnit(column);
}
private PrimaryGridUnit getActivePrimaryGridUnit(int column) {
1.1.2.13 +8 -8 xml-fop/src/java/org/apache/fop/layoutmgr/table/Attic/TableContentLayoutManager.java
Index: TableContentLayoutManager.java
===================================================================
RCS file: /home/cvs/xml-fop/src/java/org/apache/fop/layoutmgr/table/Attic/TableContentLayoutManager.java,v
retrieving revision 1.1.2.12
retrieving revision 1.1.2.13
diff -u -r1.1.2.12 -r1.1.2.13
--- TableContentLayoutManager.java 12 May 2005 13:52:15 -0000 1.1.2.12
+++ TableContentLayoutManager.java 12 May 2005 14:13:45 -0000 1.1.2.13
@@ -269,7 +269,7 @@
if (next != null) {
guCount = Math.max(guCount, next.getGridUnits().size());
}
- GridUnit gu = (GridUnit)row.getGridUnits().get(0);
+ GridUnit gu = row.getGridUnit(0);
//Create empty grid units to hold resolved borders of neighbouring cells
//TODO maybe this needs to be done differently (and sooner)
for (int i = 0; i < guCount - row.getGridUnits().size(); i++) {
@@ -285,11 +285,11 @@
//nop, borders are already assigned at this point
} else {
for (int i = 0; i < row.getGridUnits().size(); i++) {
- gu = (GridUnit)row.getGridUnits().get(i);
+ gu = row.getGridUnit(i);
GridUnit other;
int flags = 0;
if (prev != null && i < prev.getGridUnits().size()) {
- other = (GridUnit)prev.getGridUnits().get(i);
+ other = prev.getGridUnit(i);
} else {
other = null;
}
@@ -312,7 +312,7 @@
flags = 0;
if (next != null && i < next.getGridUnits().size()) {
- other = (GridUnit)next.getGridUnits().get(i);
+ other = next.getGridUnit(i);
} else {
other = null;
}
@@ -364,7 +364,7 @@
int maxCellHeight = 0;
for (int j = 0; j < row.getGridUnits().size(); j++) {
maxColumnCount = Math.max(maxColumnCount, row.getGridUnits().size());
- GridUnit gu = (GridUnit)row.getGridUnits().get(j);
+ GridUnit gu = row.getGridUnit(j);
if ((gu.isPrimary() || (gu.getColSpanIndex() == 0 && gu.isLastGridUnitRowSpan()))
&& !gu.isEmpty()) {
PrimaryGridUnit primary = gu.getPrimary();
@@ -706,10 +706,10 @@
//Add areas for row
//addRowBackgroundArea(rowFO, lastRowHeight, layoutContext.getRefIPD(), yoffset);
for (int i = 0; i < gridUnits.length; i++) {
- GridUnit currentGU = (GridUnit)lastRow.getGridUnits().get(i);
+ GridUnit currentGU = lastRow.getGridUnit(i);
if ((gridUnits[i] != null)
&& (forcedFlush || (end[i] == gridUnits[i].getElements().size() - 1))
- && (currentGU.isLastGridUnitRowSpan())) {
+ && (currentGU == null || currentGU.isLastGridUnitRowSpan())) {
if (log.isDebugEnabled()) {
log.debug((forcedFlush ? "FORCED " : "") + "flushing..." + i + " "
+ start[i] + "-" + end[i]);
1.1.2.2 +9 -1 xml-fop/src/java/org/apache/fop/layoutmgr/table/Attic/EffRow.java
Index: EffRow.java
===================================================================
RCS file: /home/cvs/xml-fop/src/java/org/apache/fop/layoutmgr/table/Attic/EffRow.java,v
retrieving revision 1.1.2.1
retrieving revision 1.1.2.2
diff -u -r1.1.2.1 -r1.1.2.2
--- EffRow.java 11 May 2005 15:13:49 -0000 1.1.2.1
+++ EffRow.java 12 May 2005 14:13:45 -0000 1.1.2.2
@@ -60,6 +60,14 @@
return gridUnits;
}
+ public GridUnit getGridUnit(int index) {
+ if (index >= 0 && index < gridUnits.size()) {
+ return (GridUnit)gridUnits.get(index);
+ } else {
+ return null;
+ }
+ }
+
public void setFlagForAllGridUnits(int flag, boolean value) {
Iterator iter = gridUnits.iterator();
while (iter.hasNext()) {
---------------------------------------------------------------------
To unsubscribe, e-mail: fop-commits-unsubscribe@xmlgraphics.apache.org
For additional commands, e-mail: fop-commits-help@xmlgraphics.apache.org
Re: cvs commit: xml-fop/src/java/org/apache/fop/layoutmgr/table TableStepper.java TableContentLayoutManager.java EffRow.java
Posted by Jeremias Maerki <de...@greenmail.ch>.
Message received and understood, code improved. I'll pay more attention
next time. Looks like peer review really works around here. Thanks, Glen.
On 13.05.2005 01:38:24 Glen Mazza wrote:
> jeremias@apache.org wrote:
>
> >jeremias 2005/05/12 07:13:45
> >
> > Modified: src/java/org/apache/fop/layoutmgr/table Tag:
> > Temp_KnuthStylePageBreaking TableStepper.java
> > TableContentLayoutManager.java EffRow.java
> > Log:
> > Fix for ArrayIndexOutOfBoundsException when empty grid units are involved.
> >
> >
>
> Jeremias, I don't see this as a fix--you seem to be converting a
> RunTimeException into a logical error (system runs but you get bad
> output.) The latter is many more times harder and more stressful to fix
> because with an LE we have no idea where the problem is--FOTree, Layout,
> Renderers, PDF Library, user version of JDK/Adobe Acrobat, etc., etc.
> Converting RTE's into LE's IMO does not really create rigorous, robust,
> low-maintenance coding.
>
> >
> > + public GridUnit getGridUnit(int index) {
> > + if (index >= 0 && index < gridUnits.size()) {
> > + return (GridUnit)gridUnits.get(index);
> > + } else {
> > + return null;
> > + }
> > + }
> >
> >
>
> If the caller is so incompetent to be requesting grid unit #42 for a
> system with only 10 grid units--shouldn't we have FOP to halt with the
> Array Index RTE so we can get that bug quickly identified and fixed?
> (Or, if we can't fix it immediately, put a Band-Aid fix in the caller
> instead of the callee?) The quiet returning of "null" thwarts that, and
> when users start complaining about bad output due to the LE, we won't
> know where the problem is to fix it. In addition to wearing out
> committers wading through the renderers and the PDF library when an RTE
> would have told them to quickly look at the FO package, we'll also have
> to ask the users a bunch of irrelevant questions such as their versions
> of Adobe Acrobat, etc. I don't see how an LE helps us here.
>
> I mentioned this to you earlier because of a odd change you made (line
> 98 of [1]) to the Span class to create an LE instead of an RTE should
> PSLM ask for an invalid column. I don't understand your rationale--if
> PSLM is asking for the wrong columns, the output will be messed up
> anyway. Best then to choose the solution--i.e., the RTE--that allows us
> to quickly zero in on the problem.
>
> I converted your change back[2, line 94/85] to explicitly return an
> IllegalStateException, and it was good I did so--I later had an error in
> the PSLM coding, asked for an invalid column, and quickly was informed
> by the RTE what the problem was so I could immediately fix it.
>
> Thanks,
> Glen
>
> [1]
> http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/area/Span.java?r1=1.6&r2=1.6.2.1&diff_format=h
>
> [2]
> http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/area/Span.java?r1=1.6.2.1&r2=1.6.2.2&diff_format=h
Jeremias Maerki
Re: cvs commit: xml-fop/src/java/org/apache/fop/layoutmgr/table
TableStepper.java TableContentLayoutManager.java EffRow.java
Posted by Glen Mazza <gm...@apache.org>.
jeremias@apache.org wrote:
>jeremias 2005/05/12 07:13:45
>
> Modified: src/java/org/apache/fop/layoutmgr/table Tag:
> Temp_KnuthStylePageBreaking TableStepper.java
> TableContentLayoutManager.java EffRow.java
> Log:
> Fix for ArrayIndexOutOfBoundsException when empty grid units are involved.
>
>
Jeremias, I don't see this as a fix--you seem to be converting a
RunTimeException into a logical error (system runs but you get bad
output.) The latter is many more times harder and more stressful to fix
because with an LE we have no idea where the problem is--FOTree, Layout,
Renderers, PDF Library, user version of JDK/Adobe Acrobat, etc., etc.
Converting RTE's into LE's IMO does not really create rigorous, robust,
low-maintenance coding.
>
> + public GridUnit getGridUnit(int index) {
> + if (index >= 0 && index < gridUnits.size()) {
> + return (GridUnit)gridUnits.get(index);
> + } else {
> + return null;
> + }
> + }
>
>
If the caller is so incompetent to be requesting grid unit #42 for a
system with only 10 grid units--shouldn't we have FOP to halt with the
Array Index RTE so we can get that bug quickly identified and fixed?
(Or, if we can't fix it immediately, put a Band-Aid fix in the caller
instead of the callee?) The quiet returning of "null" thwarts that, and
when users start complaining about bad output due to the LE, we won't
know where the problem is to fix it. In addition to wearing out
committers wading through the renderers and the PDF library when an RTE
would have told them to quickly look at the FO package, we'll also have
to ask the users a bunch of irrelevant questions such as their versions
of Adobe Acrobat, etc. I don't see how an LE helps us here.
I mentioned this to you earlier because of a odd change you made (line
98 of [1]) to the Span class to create an LE instead of an RTE should
PSLM ask for an invalid column. I don't understand your rationale--if
PSLM is asking for the wrong columns, the output will be messed up
anyway. Best then to choose the solution--i.e., the RTE--that allows us
to quickly zero in on the problem.
I converted your change back[2, line 94/85] to explicitly return an
IllegalStateException, and it was good I did so--I later had an error in
the PSLM coding, asked for an invalid column, and quickly was informed
by the RTE what the problem was so I could immediately fix it.
Thanks,
Glen
[1]
http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/area/Span.java?r1=1.6&r2=1.6.2.1&diff_format=h
[2]
http://cvs.apache.org/viewcvs.cgi/xml-fop/src/java/org/apache/fop/area/Span.java?r1=1.6.2.1&r2=1.6.2.2&diff_format=h