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. >> - 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. > 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. 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. <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... Thanks, Vincent
