Re: (Changing Vote) Re: [VOTE] Andreas L. Delmelle for committer
Andreas L. Delmelle wrote:
Hi Andreas,
i hope you dont mind a little feedback on this.
snip/
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
declaration:
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 fop.fo.flow.TableCell.
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.
Chris
RE: (Changing Vote) Re: [VOTE] Andreas L. Delmelle for committer
-Original Message-
From: Glen Mazza [mailto:[EMAIL PROTECTED]
snip /
I'm impressed with your very rapid learning of how the
code works and interest in fixing it.
Well, just don't be too impressed here. I used to top the class when it came
to Pascal/C/C++, so that gives an obvious head-start in understanding source
code (but I *did* finally quit, remember?)
Actually, you're ahead of me right now in this section
of the code--I haven't looked much at tables and hence
can't respond yet to your questions. Hopefully the others
can comment!
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
declaration:
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...)
2. Add colspan and rowspan instance variables to the cell LM, to store the
column/row-spanning properties of the corresponding fop.fo.flow.TableCell.
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). 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!
Cheers,
Andreas
RE: (Changing Vote) Re: [VOTE] Andreas L. Delmelle for committer
Andreas, I'm impressed with your very rapid learning of how the code works and interest in fixing it. Actually, you're ahead of me right now in this section of the code--I haven't looked much at tables and hence can't respond yet to your questions. Hopefully the others can comment! Thanks, Glen --- Andreas L. Delmelle [EMAIL PROTECTED] wrote: BTW *if* he is, perhaps he (or s.o. else) could explain in terms of design, what he thinks should 'happen' in case of row- or column-spanned table-cells. __ Do you Yahoo!? New Yahoo! Photos - easier uploading and sharing. http://photos.yahoo.com/
RE: (Changing Vote) Re: [VOTE] Andreas L. Delmelle for committer
Andreas L. Delmelle wrote: -Original Message- From: Glen Mazza [mailto:[EMAIL PROTECTED] snip / In particular, his recent handling of a messy disagreement between the committers a couple of weeks back was very helpful Good to see that it's considered to be helpful. I, myself, was not too sure at first about whether or not to put it in such strong wordings. Still not fully satisfied, though, but if Victor's still monitoring the list (closely or from a distance), I guess my intention is at least partly fulfilled. I think you have misunderstood Glen's intent. Cold turkey would have been better for me, but seemed irresponsible. I have been monitoring the list to see if Jeremias would respond on the Driver question. Any future correspondence needing my attention should be directed to me off-list. Victor Mote
RE: (Changing Vote) Re: [VOTE] Andreas L. Delmelle for committer
-Original Message-
From: Glen Mazza [mailto:[EMAIL PROTECTED]
snip /
In particular, his recent handling of a messy
disagreement between the committers a couple of weeks
back was very helpful
Good to see that it's considered to be helpful. I, myself, was not too sure
at first about whether or not to put it in such strong wordings. Still not
fully satisfied, though, but if Victor's still monitoring the list (closely
or from a distance), I guess my intention is at least partly fulfilled.
BTW *if* he is, perhaps he (or s.o. else) could explain in terms of design,
what he thinks should 'happen' in case of row- or column-spanned
table-cells. I'm staring at the related classes, of which the ones in
fop.fo.flow seem to be equipped for handling this ( TableCell has member
vars numColumnsSpanned and numRowsSpanned ), but IIC, then a similar
functionality is still lacking in the cell's layout manager (in
fop.layoutmgr.table.Cell).
What do you think? Is the layout context in getNextBreakPoss() incorrectly
set to the current column, instead of the current and n following columns,
where n equals (numColumnsSpanned - 1)? Or rather, AFAICT
context.getRefIPD() sets cellIPD to the IPD of the first column in the range
the cell occupies, regardless of colspan specifications.
I was still having a bit of difficulty finding out how to access the
corresponding TableCell Object from the layout manager... --so, it inherits
a FObj member via BlockStackingLayoutManager-AbstractLayoutManager.
Hmmm. Something like the following in fop.layoutmgr.table.Cell:
import org.apache.fop.fo.flow.TableCell;
...
then, further on, right above the spot where I added the little patch a few
days ago:
if( (TableCell).fobj.getNumColumnsSpanned() = 1 ) {
cellIPD = context.getRefIPD();
} else {
// something
}
Would this be the correct direction, or is the LayoutContext in this case
supposed to be returning the correct IPD? I'd guess the layout context isn't
aware of the properties of the reference fo object, so this logic definitely
belongs in the layout managers.
Perhaps something missing at Row level?
In fop.layoutmgr.table.Row.getNextBreakPoss() this happens at line 194:
childLC.setRefIPD(col.getWidth());
I think the widths of the subsequent (n - 1) columns should be added to
this.
As it is right now, this would be the same as
int cspan = 1;
int refIPD = 0;
for( int cnt = cspan; --cnt = 0; ) {
refIPD += ((Column).columns.get(cellcount - 1 + cnt);
}
childLC.setRefIPD(refIPD);
Now it's only a matter of retrieving the colspan for the Cell. For this
purpose, I thought it necessary to equip Cell.java with a colspan member var
and corresponding getNumColumnsSpanned(), then change the first line in the
above to:
int cspan = ((Cell)cellList(cellcount - 1)).getNumColumnsSpanned();
This seems to partially solve the problem. The cellIPD gets set to the
correct value if you log it, but...
in the Row class, method addAreas() at line 352:
xoffset += col.getWidth();
So, a similar construction over here, and we've got ourselves a patch :)
Expect this soon, just gonna add a few testcases (--you never know)
Thanks again for the vote of confidence!
Cheers,
Andreas
