AddAreas in RowPainter

[Vincent Hennebert <vi...@anyware-tech.com>]

Hi,

There are things unclear to me in the addAreasAndFlushRow method of the
RowPainter class. I hope someone can shed some light:
- 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.
- 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?
- 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)?
There seems to be some careful selection of the cells to be painted,
which is still a bit cryptic to me...

Thanks,
Vincent

Re: AddAreas in RowPainter

[Jeremias Maerki <de...@jeremias-maerki.ch>]

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)

Re: AddAreas in RowPainter

[Vincent Hennebert <vi...@anyware-tech.com>]

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

Re: AddAreas in RowPainter

[Jeremias Maerki <de...@jeremias-maerki.ch>]

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.

> - 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.

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
writing I was simply focused on making the test cases work. I obviously
spent too little time better documenting some of the complex aspects
there. Maybe some of the code can be simplified. That's the problem if
you work towards making tests work: you simply stop when all tests pass
and you don't go back to refactor. Bad excuse, I know. But I was happy
to make the code work in the first place.


Jeremias Maerki