Re: cvs commit: xml-fop/src/java/org/apache/fop/layoutmgr/table TableStepper.java TableContentLayoutManager.java EffRow.java

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: xml-fop/src/java/org/apache/fop/layoutmgr/table TableStepper.java TableContentLayoutManager.java EffRow.java

gmazza
[hidden email] 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

Reply | Threaded
Open this post in threaded view
|

Re: cvs commit: xml-fop/src/java/org/apache/fop/layoutmgr/table TableStepper.java TableContentLayoutManager.java EffRow.java

Jeremias Maerki
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:

> [hidden email] 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