Andreas L. Delmelle wrote:

Hi Andreas,

i hope you dont mind a little feedback on this.


Well, this is as far as I got (--actually, now I think at least column-span
is solved fully. Then again, so I did the first time ;)

In ascending order of importance:

1. In a number of methods in fop.layoutmgr.Row, I encountered the following
  int size = columns.size();
I would make this an instance variable in this case, so it gets declared
only once for every row LM, instead of with each call to the LM's
getNextBreakPoss() or addAreas() (--and inside a while-loop: there would
have to be a *very* good reason for this. I see none in particular.)
(Also, but this I'm absolutely not sure about, is it *really* necessary to
be able to access the columns as objects at row level? For now, only their
widths seem to be used/referenced, so we might be able to use an array of
ints containing just these... Not that it's totally unimaginable that we
need to access other props, but we could always provide accessor methods for
them in the Row's parentLM. After all, they *are* in fact direct descendants
of the fo:table in XSL-FO...)

I would hold off on removing the column objects from the row LM until after a design for auto table layouts and border collapsing has been at least considered.

2. Add colspan and rowspan instance variables to the cell LM, to store the column/row-spanning properties of the corresponding Also add accessor methods for these, so they can be referenced from within the row LM. (see further)

3. Add a cspanPrev instance variable to the cell LM, to store the number of
columns spanned by the previous cells in the same row (-- or previous rows,
in case of rowspan).

what about when a cell spans both rows and columns? perhaps two variables cspanPrev and rspanPrev would be a better approach here?

For this variable, a setter would also have to be
provided to allow the row LMs to alter it. When a given cell with index i in
the current row spans m multiple rows, the values of the proposed variable
can then be increased for the cells with index >= i for the next m - 1 rows,
increased by the value colspan - 1 of the current cell.

4. (This as a result of my altering the code in fop.layoutmgr.Row; found
myself typing up the same loop and needing it another time, so) Add a
convenience method to the row LM, say :

private int computeCellRefIPD( int cellidx ) {...}

which roughly contains the loop in the invalid (excuse for a) patch I
submitted a few days ago, with the necessary corrections.
This function would calculate the reference IPD of a given cell. In
combination with the above suggestions, the result will take into account:
- the column-spanning of the cell itself
- the row-spanning of cells in previous rows (and their column-spanning), so
effectively using the right base column for every cell

The things I'm still looking for are
- the correct adjustment for the cell LM's height in case of rowspan
- the adjustment for the x-coordinate of the cells affected by a rowspan
- some vague problems wrt breaks/keeps I can't seem to describe yet (a
gut-feeling, as they say)

Can't be that hard... Naively tried (rowHeight * rowspan) for the first, but
this obviously doesn't work in case one of the related rows is higher than
the current :)

Expect a new patch proposal soon!

The rest of what you propose sounds good.


Reply via email to