On 19.03.2007 14:40:49 Vincent Hennebert wrote:
> Hi Jeremias,
>
> Thanks for your answers.
>
> Jeremias Maerki a écrit :
> > On 17.03.2007 01:37:16 Vincent Hennebert wrote:
> >> Hi,
> >>
> >> There are things unclear to me in the addAreasAndFlushRow method of the
> >> RowPainter class. I hope someone can shed some light:
> >
> > I wrote this a long time ago. Let's hope I can dig some of it out again.
> >
> >> - why is a Map used to store y-offsets of rows? That seems to indicate
> >> that rows are not added in a sequential order, or that there could be
> >> hole between them, which sounds unlikely to me.
> >
> > Frankly, I don't know anymore. Bad documenting on my part. The main
> > reason I think was header and footer positioning because you first get
> > the headers then you move to the body and finally do the footers. Since
> > the areas generated by the table are reference areas they each have
> > relative positioning (X and Y) inside the table.
> >
> > You can easily disable the offset map and see if you can make it run
> > with only the yoffset variable. Don't have time to try it myself ATM.
> > Sorry.
>
> Ok, after some further testing I figured out that some structure to
> store the y offsets of each row /is/ needed, but a List-like one should
> make the trick.
> This is needed because (by simplifying a bit) areas for cells are added
> once the end of the area is reached; if the cell spans over several rows
> we need to know what was the y-offset of the row where it begins.
Right. Figured that out myself now.
>
> >> - there is one condition that I don't understand on l.204: in case the
> >> primary grid unit at the given column isn't null, then the
> >> corresponding areas are added only if:
> >> - forcedFlush == true, or
> >> - the end of the cell is reached on the current row AND (the current
> >> grid unit is null or is the last in the row-spanning direction).
> >> What's the purpose of that second member of the and?
> >
> > This is for situations like this:
> > - an empty grid unit has to be painted
> > - a cell could be empty (i.e. not providing any positions)
> > - a cell that is broken over multiple pages has already contributed
> > all its content on the previous page and does not contribute any new
> > positions on the current position so we have to make sure the cell is
> > painted all the same.
>
> Hmmm, I was thinking that if the corresponding primary grid unit is not
> null, this means that the cell begins on the current page.
> After removing the second part of the "and" all the tests still pass.
Actually, there was a check missing which I added. Now you cannot remove
that part anymore without breaking the tests. Removing that part causes
one cell to be omitted.
>
> > Grid units might not have been generated when there are no borders to
> > paint.
> >
> > That's basically what the comment under line 204 says. Again you can
> > comment some of the if and run the table test cases to see what happens.
> >
> >> - also, inside the if branch, in case the primary grid unit was null,
> >> then the primary grid units from the current grid unit (i.e., on the
> >> current row) is retrieved if:
> >> - it does not correspond to an empty cell;
> >> - the cell doesn't span in the column direction
> >> - it is the last grid unit
> >> Why such conditions, and can the primary grid unit still be null after
> >> that (as implied by the if l.223)?
> >
> > I'd have to dive deeper into this one.
> >
> >> There seems to be some careful selection of the cells to be painted,
> >> which is still a bit cryptic to me...
> >
> > What I can offer is to allocate some time next week (Wed or Fri) to
> > better comment (and possibly refactor) the code there. At the time of
>
> Thanks. Commenting/explaining would be enough I think, because I'll
> probably have to refactor things anyway to implement collapsing-border
> model. I would be already less afraid of breaking things if the
> remaining uncertainties are cleared.
You'll always have the tests. ;-)
> If you could also have a look at the "while (offset == null)" loop in
> the addAreasForCell method on l.247, that would be great. I believe it
> is not necessary, and again commenting it out doesn't make the tests
> fail. But perhaps you'll remember of why you wrote it in the first place
> and if it is still needed in some situation not covered by the tests.
I agree that it is probably superfluous but since I want to be careful
about touching running code I only put a comment there. An alternative
would be an exception that tells the user no notify us ("user tests").
:-)
> <snip/>
>
> Speaking of refactoring I'm thinking of avoiding null grid units and use
> some kind of flyweight pattern instead. There are plenty of places all
> over the code where they are tested for null, and I think it would
> simplify things (especially in the addAreas method, it seems).
> Just in case somebody has already thought about that and would have
> comments towards/against that...
Makes sense.
Jeremias Maerki (how's got to stop documenting code now)
