Here is a first patch for the auto table layout. It will be changing greatly in the next few days.
Created attachment 18726 [details] Patch with basic auto table
Hi Patrick, Just did a quick review of the patch, and one thing that immediately caught my eye, and thus I think needs more explaining (or an alternative solution): You seem to be inventing a whole new property 'column-min-width'. The properties infrastructure is there for support of standard FO properties (and ultimately extension properties). I get the impression it is being misused here to store a variable/value which is only relevant to FOP internally (more specifically: the auto-table layout algorithm) Cheers, Andreas
Looking a bit closer, IMO the minimum column-width should be derived from the layout context. Count the number of non-null elements in the Table's column-list (one time process), then divide the refIPD of the layout-context by the number of explicitly defined columns (alt.: the largest number of cells in a row --that is a value that could be determined in the FOTree, before layout begins) Strictly speaking, I think a value of 'proportional-column-width(1)' does not always suffice... How is this expression to be interpreted in case of a table with table-layout="auto", no explicit rows, and a varying number of cells in each row? I guess the editors had good reason to constrain the explicit use of proportional-column-width() to fixed- table-layout.
Not to be a pain, but just committed a small change to the trunk that logs an error for this. http://svn.apache.org/viewvc?rev=432195&view=rev Sorry. Kind of invalidates your patch, I guess. :/
You are right. I will have to find an alternative solution to this. I was only using it to store the minimum width of a column. Thank you, Patrick
(In reply to comment #3) > Looking a bit closer, IMO the minimum column-width should be derived from the layout context. Count > the number of non-null elements in the Table's column-list (one time process), then divide the refIPD of > the layout-context by the number of explicitly defined columns (alt.: the largest number of cells in a row I am not sure I follow you. When you talk about the column-list, is that the "ColumnSetup columns " in TableLayoutManager ? > --that is a value that could be determined in the FOTree, before layout begins) > > Strictly speaking, I think a value of 'proportional-column-width(1)' does not always suffice... > How is this expression to be interpreted in case of a table with table-layout="auto", no explicit rows, and > a varying number of cells in each row? > I guess the editors had good reason to constrain the explicit use of proportional-column-width() to fixed- > table-layout. Patrick
(In reply to comment #6) <snip /> > I am not sure I follow you. When you talk about the column-list, is that the > "ColumnSetup columns " in TableLayoutManager ? Not entirely. ColumnSetup will currently only contain the actual column count in case you have explicitly defined columns. There's a method ColumnSetup.createFromFirstRow(), but this maps to return the table's default column (= FOP internal). So, in case you have a table without explicit columns, the maximum column-count would already be different. We could quite easily keep track of the maximum number of cells in a row as the FOTree is built, so that, by the time layout starts, the layoutmanager could already know what the maximum column- count will be for the whole table without having to iterate over all the rows. HTH! Andreas
(In reply to comment #7) > > We could quite easily keep track of the maximum number of cells in a > row as the FOTree is built, so that, by the time layout starts, the > layoutmanager could already know what the maximum column- > count will be for the whole table without having to iterate over all the rows. To expand a bit more upon this (could also be of use for fixed-layout): Ideally, ColumnSetup should be made to deal with both implicit and explicit columns completely, so the TableContentLM does not need to sort this stuff out. The columns in ColumnSetup should be the table- grid columns. I did wonder whether this should be viewed on a per-page basis...? I mean: say you have a table with five cells per row (100+ rows), except for the last row which has six. Do we layout the whole table as if there were six columns, or only the last page? Cheers Andreas
(In reply to comment #8) > I did wonder whether this should be viewed on a per-page basis...? I mean: say you have a table with > five cells per row (100+ rows), except for the last row which has six. > Do we layout the whole table as if there were six columns, or only the last page? Look at it this way: You'd expect an fo:block to occupy the whole width on both pages if that block is broken between two pages and the first is a landscape page and the second is a portrait page. Now transfer this to fo:table where you might specify columns entirely using proportional-column-width() (or even using auto-table layout). Especially when specifying the table width using width="100%" you define that the area generated by the FO occupies 100% of the width of the containing area. So, especially in the light of the upcoming changes for "changing available IPD", there's a possibility that you'll have to recalculate the column setup for every page. Not that the table layout can already deal with this situation, but it will have to at some point.
(In reply to comment #9) > > I did wonder whether this should be viewed on a per-page basis...? > > I mean: say you have a table with > > five cells per row (100+ rows), except for the last row which has six. > > Do we layout the whole table as if there were six columns, or only the last page? > > Look at it this way: You'd expect an fo:block to occupy the whole width on both > pages if that block is broken between two pages and the first is a landscape > page and the second is a portrait page. Now transfer this to fo:table where you > might specify columns entirely using proportional-column-width() (or even using > auto-table layout). Yes, but what I'm referring to is more that the _proportions_ would remain the same over the whole table, no matter what the actual page-dimensions are. Given that proportional-column-width() and auto table-layout are mutually exclusive, distribution of remaining space can occur if: a) table-layout="fixed", explicit columns with relative widths (some using p-c-w()) b) table-layout="fixed", implicit columns with relative widths (from cells in the first row) c) table-layout="auto" Take case a): Suppose six defined columns, then in my original example, I'd expect the column-widths to have the same proportions on every page, no matter if the page contains a cell that occupies a particular column. This is what the average human being would expect, I guess, especially if it isn't the last column. Move on to case b): Suppose the first row contains five cells, each with a relative width of 20% --no proportional-column- width() here. Strictly following the Rec, we should end up with a table of five columns, equally large in proportion on all pages. What happens if I add a sixth cell to the last row? (I'd say we can safely ignore and complain about it, which we already do, IIC) Case c) seems to offers more liberty here, since not only the absolute, but also the relative values can be evaluated separately for each page.
(In reply to comment #4) > Not to be a pain, but just committed a small change to the trunk that logs an error for this. > > http://svn.apache.org/viewvc?rev=432195&view=rev > > Sorry. Kind of invalidates your patch, I guess. :/ I'm confused about this. When I checkout the Apache FOP source code from SVN and try to feed it some .fo files that previously worked with table-layout="auto" they fail. In my new patch I commented out your change just so that I may do some testing. I have to understand this better. Thank you for your feedback. Patrick
Created attachment 18736 [details] New patch. Now works better. Still needs work for spanned cells. I'm no longer using a new property to store my minimum column values, as pointed out by Andreas. I have also fixed some bugs in the optimal width calculation. This is still a "drafty" patch. Patrick
(In reply to comment #3) > Looking a bit closer, IMO the minimum column-width should be derived from the layout context. Count > the number of non-null elements in the Table's column-list (one time process), then divide the refIPD of > the layout-context by the number of explicitly defined columns (alt.: the largest number of cells in a row > --that is a value that could be determined in the FOTree, before layout begins) I'm not sure I follow you. I thought the minimum column-width was to be determined by the "largest minimum cell width (or the column 'width', whichever is larger)". (http://www.w3.org/TR/REC-CSS2/tables.html#width-layout) Do you mean that we would consider the "default" column-width to be the one calculated as you describe ? Patrick
(In reply to comment #11) > I'm confused about this. When I checkout the Apache FOP source code from SVN and > try to feed it some .fo files that previously worked with table-layout="auto" > they fail. Indeed. That's a result from the 'faulty' default for column-width we currently set in FOPropertyMapping. The value "proportional-column-width(1)" should not be used in case of table-layout="auto" (as per the Recommendation; strictly speaking the results in case of auto-layout are undefined...) It is easy enough to catch this internally, though. To solve this locally, I made the default an enum value of "auto", then checked for this later on, and replaced it with a 'new TableColLength(1.0, ...)' Roughly (in TableColumn.bind(), after setting columnWidth from the PropertyList): if (columnWidth.getEnum() == EN_AUTO) { columnWidth = new TableColLength(1.0, col); } This avoids triggering the function-evaluation, which in turn avoids the check for auto-layout and thus produces no validation error. Haven't committed this change, yet, since I was also playing with adding default columns from cells in the first row. Goal is to have TableColumn instances for every default column (instead of only one default column for the whole table) with their widths set to the width of the cell they were based on (ultimately the above described default TableColLength(), if the cell's width is "auto"). > In my new patch I commented out your change just so that I may do some testing. > I have to understand this better. I hope the above clarifies it a bit. If not, don't hesitate to ask further. What is also important in case of auto-layout, I think, is that the minimum-column-width should not simply be 'the available IPD divided by the number of columns' (or 'one table-unit'). The big difference with fixed-layout is precisely that, in case of auto-layout the minimum-column- width depends on the content. For example: if the column contains only one character of content each row, then the column's minimum-width would most likely turn out to be far less than 'proportional- column-width(1)'. Cheers, Andreas
(In reply to comment #14) <snip /> > What is also important in case of auto-layout, I think, is that the minimum-column-width should not > simply be 'the available IPD divided by the number of columns' (or 'one table-unit'). > The big difference with fixed-layout is precisely that, in case of auto-layout the minimum-column- > width depends on the content. Just thought I'd add: This actually means that for auto-layout, if we encounter a TableColumn whose width is an instance of TableColLength, then this width should be ignored. It means there were no explicit constraints placed on the column's width. It will depend *completely* on the content of the cells occupying that column (max-col-width = min-col-width). Cheers, Andreas
(In reply to comment #12) > Created an attachment (id=18736) [edit] > New patch. Now works better. Still needs work for spanned cells. > There is a reference to a TableHelper, but this file is not included in the patch. Can you attach it separately? Thanks, Andreas
(In reply to comment #13) > > I'm not sure I follow you. I thought the minimum column-width was to be > determined by the "largest minimum cell width (or the column 'width', whichever > is larger)". (http://www.w3.org/TR/REC-CSS2/tables.html#width-layout) Indeed! That only occurred to me afterwards... sorry. OTOH, this also means that you can't depend on 'proportional-column-width()', no? If there was no width specified, neither on the column, nor on the cell, then both minimum and maximum depend on the content (of the whole table :)) The only thing we know about the optimum width is that it lies somewhere in between, but it is independent of something like a 'default column width' (which we had defined wrongly in the first place). > Do you mean that we would consider the "default" column-width to be the one > calculated as you describe ? Yep. But, again, in the strict sense, this 'default' is irrelevant for auto-layout.
Created attachment 18737 [details] Table helper file that goes with the patch Sorry about that. Here is the file. It is only used to pass values. I will probably have to find a better mechanism for this. Thank you very much for all the feedback and help. Patrick
(In reply to comment #10) > (In reply to comment #9) > > > I did wonder whether this should be viewed on a per-page basis...? > > > I mean: say you have a table with > > > five cells per row (100+ rows), except for the last row which has six. > > > Do we layout the whole table as if there were six columns, or only the last page? > > > > Look at it this way: You'd expect an fo:block to occupy the whole width on both > > pages if that block is broken between two pages and the first is a landscape > > page and the second is a portrait page. Now transfer this to fo:table where you > > might specify columns entirely using proportional-column-width() (or even using > > auto-table layout). > > Yes, but what I'm referring to is more that the _proportions_ would remain the same over the whole > table, no matter what the actual page-dimensions are. Not necessarily for auto table layout, I'd say. Anyway, neither the CSS nor the FO spec tells us what algorithm to use to determine the column widths and how and if the widths have to be reevaluated for each page. > Given that proportional-column-width() and auto table-layout are mutually exclusive, distribution of > remaining space can occur if: > > a) table-layout="fixed", explicit columns with relative widths (some using p-c-w()) > b) table-layout="fixed", implicit columns with relative widths (from cells in the first row) > c) table-layout="auto" > > Take case a): > Suppose six defined columns, then in my original example, I'd expect the column-widths to have the > same proportions on every page, no matter if the page contains a cell that occupies a particular > column. This is what the average human being would expect, I guess, especially if it isn't the last > column. > > Move on to case b): > Suppose the first row contains five cells, each with a relative width of 20% --no proportional-column- > width() here. Strictly following the Rec, we should end up with a table of five columns, equally large in > proportion on all pages. What happens if I add a sixth cell to the last row? (I'd say we can safely ignore > and complain about it, which we already do, IIC) > > Case c) seems to offers more liberty here, since not only the absolute, but also the relative values can > be evaluated separately for each page. agreed.
(In reply to comment #17) <snip/> > If there was no width specified, neither on the column, nor on the cell, then both minimum and > maximum depend on the content (of the whole table :)) Again, not necessarily. We don't have to scan the whole table for the algorithm. <snip/>
Created attachment 18738 [details] testcase for table-layout =auto I can't get the checks right. Patrick
Created attachment 18739 [details] New patch. Works with Andreas modifications. Here is my new patch. It works with Adreas new modifications. I will address Andreas questions and comments tomorrow. Thank you, Patrick
(In reply to comment #18) > Created an attachment (id=18737) [edit] > Table helper file that goes with the patch > > Sorry about that. Here is the file. It is only used to pass values. I will > probably have to find a better mechanism for this. Not probably, you definitely have to find a different way. Communicating through static variables is a total no-go.
(In reply to comment #23) > (In reply to comment #18) > > > > Sorry about that. Here is the file. It is only used to pass values. I will > > probably have to find a better mechanism for this. > > Not probably, you definitely have to find a different way. Communicating through > static variables is a total no-go. Ouch! I wish I had looked closer at this one yesterday... This is indeed not acceptable. Why statics, Patrick? Remember that FOP in the general case does not run exclusively in one thread on a single-processor machine, so it might well be that there is more than one table for which layout is performed at the same time. If that happens, the TableHelper class members would be shared between threads. :/
Created attachment 18749 [details] New patch now avoiding static variables for min and max widths This patch doesn't make use of the static variables in table-helper anymore. Unfortunately it still uses the TableHelper.calculateMode static variable but I will be removing that soon. I'd rather update frequently even if there are upcoming changes. Cheers, Patrick
Hi Patrick, Thanks for the updates. Just to let you know: I'll look further into your patch ASAP. (If you've been following the commit list, you'll notice I'm getting a bit tired ;)) Will be back tomorrow. Cheers, Andreas
Patrick, A few requests and pointers: * Can you try and give the variables a bit more descriptive names? It's just that at times, I was getting a bit lost in the 'iX's... :) * Don't be afraid/ashamed to re-use existing structures: I get the impression that your MinMax datatype is only slightly different from the MinOptMax we already heavily use. Maybe you could use the latter...? * I'd also put the stuff related to table-layout in separate methods (instead of inserting the new code- blocks into existing methods), and make sure the extra objects don't get created for fixed layout. For fixed layout, it is unnecessary to have two TableContentLMs and two ColumnSetups... Also, as an alternative to the implementation of getMinMaxTextWidths() --still using the misleading name?-- I'm beginning to wonder whether the computation of min and max content widths is functionality that belongs in the ElementListUtils utility class. Its operand is always an element list. For the rest, it seems logical, but note that we still can do little more than judge the patch by looking at it, since it is still incomplete... It would be cool if you could provide us with an integrated patch containing all changes (including added files: this time the MinMax class is missing, but see above, maybe MinOptMax can be used?), so we can see it in action for ourselves. Thanks, Andreas
Thank you very much for the pointers Adreas. I will do my best to follow them as well as the requests, and I will post back here. Patrick
Created attachment 18816 [details] New patch taking most of Andreas comments into account "TableHelper" is not needed anymore. Now using MinOptMax. AutoTable code moved into methods.
Created attachment 18817 [details] Test case for table-layout=auto.
(In reply to comment #27) > Patrick, > > A few requests and pointers: > * Can you try and give the variables a bit more descriptive names? It's just that at times, I was getting a > bit lost in the 'iX's... :) These are just iterators really. I tried ising only i and i2. > * Don't be afraid/ashamed to re-use existing structures: I get the impression that your MinMax > datatype is only slightly different from the MinOptMax we already heavily use. Maybe you could use the > latter...? Thanks for the pointer. I used it and its perfect for the job. > * I'd also put the stuff related to table-layout in separate methods (instead of inserting the new code- > blocks into existing methods), and make sure the extra objects don't get created for fixed layout. For > fixed layout, it is unnecessary to have two TableContentLMs and two ColumnSetups... Done. > Also, as an alternative to the implementation of getMinMaxTextWidths() --still using the misleading > name? Yeah... should we go with getMinMaxContentWidths() ??? > -- I'm beginning to wonder whether the computation of min and max content widths is > functionality that belongs in the ElementListUtils utility class. Its operand is always an element list. This is done only once in the LineLayoutManager. > For the rest, it seems logical, but note that we still can do little more than judge the patch by looking at > it, since it is still incomplete... It would be cool if you could provide us with an integrated patch > containing all changes (including added files: this time the MinMax class is missing, but see above, > maybe MinOptMax can be used?), so we can see it in action for ourselves. There are no more additional files thanks to your help. I am now using MinOptMax and the TableHelper is no longer used at all. > Thanks, > > Andreas Thank you, Patrick
Patrick, Class org.apache.fop.layoutmgr.table.TableHelper is not in the patch. Simon
Created attachment 18828 [details] Removed import org.apache.fop.layoutmgr.table.TableHelper in TableLM Removed import org.apache.fop.layoutmgr.table.TableHelper in TableLM
(In reply to comment #32) > Patrick, > > Class org.apache.fop.layoutmgr.table.TableHelper is not in the patch. This class is not used anymore. The import was still there. Thanks for noticing me. > Simon Thank you, Patrcik
If it were that easy, I could have guessed it myself. But you do create an object of that class. Removing that object as well helps. Simon
Created attachment 18829 [details] Removed TableHelper object TableHelper was still around... sorry about that. Patrick
(In reply to comment #35) > If it were that easy, I could have guessed it myself. But you do create an > object of that class. Removing that object as well helps. > > Simon Sorry again ! This time I hope I got it right. Many thanks, Patrick
I've reviewed the latest patch. Here are my comments, some of which I already sent to Patrick when we chatted earlier via Skype: - in the testcase an overflow happens in the second column because border widths were not respected for calculating the column widths. - The resulting widths don't seem right. The first column should be wider, the third narrower to provide a more balanced result. - The variable names as mentioned by Andreas are still not changed. I don't particularly like names which contain indexes. Speaking names are always preferrable. I think there's room for improvement. - Your Java style is a little inconsistent. Indents are 4 spaces, there's a space after "if", a space before and after "==" etc. You might want to install a CheckStyle plugin. It could help you avoid such things by telling you where the problems are. If you could improve that a little, it will cause less work for us when we apply the patch. - System.out.println calls have to be changed to use log.*(). Use log.is*Enabled() if you construct Strings with variables for log levels of info or higher. - The main point: As you know I'm still unhappy that the TableContentLayoutManager (with all its children) is constructed twice. If this can be isolated for the auto-table-layout case (i.e. the fixed table layout is not affected) it is somewhat acceptable in the short-term but the next big goal has to be to split the element list generation from the line-breaking. - It would be good to have additional test cases showing stuff that does not work, yet, like a table with some fixed-width columns. - In the end, the message "table-layout="auto" is currently not supported by FOP" can be removed. :-) I'm happy to see that we're on the way on this topic. And it is good to see the discussion about the splitting of the element list generation from the line breaking. Unfortunately, I don't think the patch is ready to be applied, yet. What do the others think?
Patrick, I have reviewed your patch. These are my remarks: columns.columns and columns2.columns contain the same TableColumn objects, columns.colWidths and columns2.colWidths contain the same TableColLength objects: columns ColumnSetup (id=79) columns ArrayList<E> (id=96) [0] TableColumn (id=104) [1] TableColumn (id=105) [2] TableColumn (id=102) colWidths ArrayList<E> (id=97) [0] null [1] TableColLength (id=114) [2] TableColLength (id=115) [3] TableColLength (id=108) maxColIndexReferenced 0 table Table (id=85) columns2 ColumnSetup (id=81) columns ArrayList<E> (id=98) [0] TableColumn (id=104) [1] TableColumn (id=105) [2] TableColumn (id=102) colWidths ArrayList<E> (id=99) [0] null [1] TableColLength (id=114) [2] TableColLength (id=115) [3] TableColLength (id=108) maxColIndexReferenced 0 table Table (id=85) Consequently, in 'copy column setup' column and column2 are the same object. Apparently there is no need for columns2. One can set the column widths, use them for the auto table layout calculations, and set them again to the calculated widths. In TableContentLayoutManager.createElementsForRowGroup, the call to determineAutoColumnWidths may be too early. The construction of the element list of the primary is not yet finished. Cf. the call to the ElementListObserver. In LineLayoutManager.collectInlineKnuthElements minMaxW is not correctly calculated when an LM has multiple child LMs. Each child LM overwrites the results of its previous sibling, and the last one remains. The same is true for the calls to minMaxTextWidths() in BlockStackingLM and TableContentLM, when they have multiple child LMs. This would be a possible scenario: The BlockLevelLMs implement minMaxTextWidths(). Basically they request the same from their child LMs, and return the combined results. TableContentLM calls minMaxTextWidths() on all the TableCellLMs and combine the results per column. LineLM implements minMaxTextWidths() as collectKnuthElements and returns the combined results for minMaxW of its paragraphs and inline blocks. LineLM keeps the constructed element lists in knuthParagraphs (check). When the proper column widths have been calculated, getNextKnuthElements is called. In this call, LineLM uses the constructed element lists (check). In this way, the column width calculations are done without invoking the table stepper algorithm, and the element lists are constructed only once. Ideally it would be programmed such that a call to minMaxTextWidths() is optional, that is, if it is called, the Knuth element lists are kept for reuse in getNextKnuthElements(), and if it is not, getNextKnuthElements() constructs the element lists as it does now. The difficult part will be in TableContentLM, taking spanned cells into account and reusing existing methods while bypassing the grid unit construction. Implementing without spanned cells is the first approximation. Perhaps a simplified version of getKnuthElementsForRowIterator is needed: you want to dive into the table cells without bothering about vertical cell alignments and page breaks. Alternatively, the grid units are constructed without line breaking in the cells, and reused in the call to getNextKnuthElements(). (pgus should be a member of TableContentLM?) Simon
Comment on attachment 18817 [details] Test case for table-layout=auto. Could we change <info> text to say that we are checking "auto" layout here?
Hello, If you guys have nothing against - I'll look into this bug/possible enhancements during next 2 weeks (and then either give up or provide some new code :)). Of course, I will need to review all comments up to date and educate myself on basic Knuth architecture. Andrejus
(In reply to comment #40) > (From update of attachment 18817 [details] [edit]) > Could we change <info> text to say that we are checking "auto" layout here? > Sure, it's just a copy/paste mistake.
Created attachment 18940 [details] Test case for table-layout=auto. Corrected typo in <info> section that we are checking table-layout="auto"
Created attachment 18941 [details] Removes the warning that table-layout="auto" is not supported by FOP
Created attachment 18942 [details] Currently produced PDF from corresponding testcase Area Tree
Created attachment 19152 [details] Initial UML diagram prepared with free EclipseUML plugin This is my initial try to visually collect some classes related to Table auto layout feature implementation. Not sure guys if you could see it properly inside for example Internet Explorer. But it's (SVG) the only allowed format for save in free version of EclipseUML plugin. No gifs/jpegs are allowed. Maybe I should switch instead to low cost MyEclipse for such diagramming...
Don't think SVG is a problem... If you insist on JPEG or GIF, there's always Batik to transcode it :)
(In reply to comment #47) > Don't think SVG is a problem... If you insist on JPEG or GIF, there's always Batik to transcode it :) I'm just trying to find kind of common denominator for that diagramming when others could also easy review/comment. So far I was not able to view generated SVG correctly inside my IE6 or Firefox 2.0. If you use any other open source/free viewer for that and could see everything correctly - please advice.
Reset assignee so mails go to list.
resetting P2 open bugs to P3 pending further review
increase priority for bugs with a patch