This patch isn't really meant to be applied... Rather to be reviewed by interested parties to check if I'm not wrong. Changelog: * javadocs for the Knuth line- and page-breaking algorithms. Some items are marked with double question marks because I haven't found out yet what is their purpose. I will probably find eventually, but if anybody has immediate hints they will be welcome. * some methods have been marked deprecated because AFAICT they are not called anywhere. If this is agreed I'll remove them in my next patch * bugfix? In the last for loop of the method layoutmgr.PageBreakingAlgorithm.noBreakBetween I think the exit condition should be a strict comparison ('<' instead of '<='). Confirmation? * the javadoc comments for some methods have been removed because they will inherit them from their super-class * some checkstyle fixes
Created attachment 18447 [details] proposed patch
(In reply to comment #0) > This patch isn't really meant to be applied... Rather to be reviewed by > interested parties to check if I'm not wrong. Changelog: > * javadocs for the Knuth line- and page-breaking algorithms. Some items are > marked with double question marks because I haven't found out yet what is their > purpose. I will probably find eventually, but if anybody has immediate hints > they will be welcome. KnuthBlockBox: bpdim seems to be used in concert with the proprietary display-align="fill" value Luca implemented. See AbstractBreaker.optimizeLineLength(). If I understand it right it is somehow used to make sure all the pages have more or less the same amount of content (in bpd). pos and bAux are defined in ListElement/KnuthElement. BreakingAlgorithm: alignment: EN_BEFORE is not used. EN_START is used instead, since the class is used in both ipd and bpd. EN_BEFORE is mapped into EN_START. Actually, alignment uses a slightly different set of value than the FO properties, so reusing the integer constants may not be the best thing, but we're not under Java 5, yet, where we could use enums. bFirst is used for the text-indent property so only the first paragraph of a block is indented. See block_text-indent.xml. partOverflowRecovery is used in page breaking to defer an element which would overflow the available BPD to the next page if it's the only element in a part (=line or page). > * some methods have been marked deprecated because AFAICT they are not called > anywhere. If this is agreed I'll remove them in my next patch +1 > * bugfix? In the last for loop of the method > layoutmgr.PageBreakingAlgorithm.noBreakBetween I think the exit condition should > be a strict comparison ('<' instead of '<='). Confirmation? not sure. :-( > * the javadoc comments for some methods have been removed because they will > inherit them from their super-class I think Checkstyle will bark about that. If you do Ctrl-J in Eclipse, you get an automatic @see entry which satisfies Checkstyle. @inheritDoc does not work in every Java version. > * some checkstyle fixes HTH
Created attachment 18456 [details] Updated patch, whith some additions and corrections I think this patch may be applied. It contains new javadoc comments and adds some precisions to previous ones. Deprecated elements have been removed.
(In reply to comment #3) > Created an attachment (id=18456) [edit] > Updated patch, whith some additions and corrections > > I think this patch may be applied. It contains new javadoc comments and adds > some precisions to previous ones. Deprecated elements have been removed. Patch applied. Thank you! Precious work for anyone who wants to dive in here. http://svn.apache.org/viewvc?rev=414135&view=rev
Created attachment 18617 [details] First patch for before-floats implementation Basic implementation of before-floats. Known problems are: * bug when the whole normal content is typeset and there are both footnotes and floats left to be placed; * floats too big to even fit on one page alone are not handled yet; * the keep-with-previous property for the generated inline anchor-area is not implemented; * some clean-up in LayoutManagers should be possible; * others that I may have missed... Changelog: * Created a breaking subpackage where, if this is agreed, all classes related to the Knuth approach will eventually be placed. * Created an OutOfLineRecord class for dealing with both before-floats and footnotes. It is basically made of code extracted from PageBreakingAlgorithm that was related to footnote handling. Most of it could also be used for floats. (pending: documentation and testcases)
I finally had a chance to take a first look. What I've seen so far looks pretty nice. A first simple test behaved as would be expected. Making a before-float too big to fit on a page (although there are break points inside the content) results in an OutOfMemoryError (probably due to an infinite loop). It would be good if you would write a set of test cases for before-floats as your next task. This is to document what works and what doesn't and to give you and us more confidence when doing further chances in the code. Finally, would you compile a list of classes you propose to move into the "breaking" package? The idea itself is worth investigating since the layoutmgr package has already grown rather big again. What we need to decide now is whether to put the changes in a branch until they stabilize or if we put it in Trunk. I'd prefer a branch for now so in case I have time to finish the work on 0.93, we don't have any problems from this end. Keep up the good work. This looks very promising.
This is a good piece of work. A few remarks: 1. Copyright year on new classes should be only 2006. 2. Wiki page: The argument for an inner class in bullet 3 only refers to an object inner class. Therefore contradicts bullet 4, a static inner class, and is invalid. 3. fo:float seems to be the only FO that can be placed in inline and block content. This causes a problem because FOP distinguishes between inline and block LMs rather much. I do not think that this problem needs to be solved in this project. It should be fine to make an implementation which works for inline fo:float FOs. 4. Infos is not correct english; better call it ProgressInfo. cumulatedLengths is better changed to cumulativeLengths. 5. The implementation does not use the max and min property of the BPD of a float. In a text with little stretch and shrink, e.g. an adapted version of the test file footnote_basic.xml, there is no possibility to place the floats with less than infinite badness, and they are moved towards the end of the page sequence. 6. The OutOfLineRecord.getFootnoteSplit and OutOfLineRecord.getFloatSplit methods suggest that there is room for two subclasses. It would be nice if the two conditional blocks in PageBreakingAlgorithm.computeDifference, if (floats.existing()) and if (footnotes.existing()) could be merged, and the difference in the logic be moved to the footnotes and floats objects. But that is finishing touch.
(In reply to comment #7) Thanks for your comments, Simon. A few answers: > 2. Wiki page: The argument for an inner class in bullet 3 only refers > to an object inner class. Therefore contradicts bullet 4, a static > inner class, and is invalid. Well, the private fields of any ProgressInfo instance are directly accessible within OutOfLineRecord. This avoids using getter and setter methods everywhere in OutOfLineRecord, for the progressInfo field as well as the prevProgress parameters of the get[Footnote|Float]Split methods. That's what I meant. > 3. fo:float seems to be the only FO that can be placed in inline and > block content. This causes a problem because FOP distinguishes > between inline and block LMs rather much. I do not think that this > problem needs to be solved in this project. It should be fine to > make an implementation which works for inline fo:float FOs. Ok, noted. On the tests I've done this seems to work, but I don't know if it will in all cases. I think the solving of this problem may be done together with the implementation of the keep-with-previous property for the anchor element. I put it on my TODO-list. > 4. Infos is not correct english; better call it > ProgressInfo. cumulatedLengths is better changed to > cumulativeLengths. Thanks, always glad to improve my english ;-) > 5. The implementation does not use the max and min property of the BPD > of a float. In a text with little stretch and shrink, e.g. an > adapted version of the test file footnote_basic.xml, there is no > possibility to place the floats with less than infinite badness, > and they are moved towards the end of the page sequence. Yes. If I'm correct the shrink/stretch capabilities of footnotes aren't considered either. More generally there is a problem with too short nodes (considered as such because no stretching is possible), which are handled separately from the other nodes. The algorithm may prefer a normal node with a deferred float instead of a "too short" node with the float on the same page. I'm thinking about an improved algorithm which would handle such cases. I think I will give some ideas on the Wiki page soon, after I've submitted testcases. But before implementing it I'll perhaps work on side-floats, don't know yet. > 6. The OutOfLineRecord.getFootnoteSplit and > OutOfLineRecord.getFloatSplit methods suggest that there is room > for two subclasses. It would be nice if the two conditional blocks > in PageBreakingAlgorithm.computeDifference, if (floats.existing()) > and if (footnotes.existing()) could be merged, and the difference > in the logic be moved to the footnotes and floats objects. But that > is finishing touch. I agree (that some further refactoring is possible). As this part of the code may change if an improved algorithm is to be implemented, I didn't put too much effort in it. This will belong to a second refactoring step. Thanks, Vincent
Created attachment 18637 [details] The very same patch, with some english corrections as suggested by Simon Pepping * (OutOfLineRecord.)ProgressInfos renamed into ProgressInfo * (OutOfLineRecord.)cumulatedLengths renamed into cumulativeLengths
Created attachment 18644 [details] Testcases for before-floats Testcases for basic features of before-floats. There are also some disabled testcases for features known to be broken.
Applied the latest two patches to a temporary branch ("Temp_Floats"): http://svn.apache.org/viewvc?rev=427052&view=rev I managed to reenable one of the disabled test cases because you were fooled by the default values for widows and orphans. Having only 3 lines in a block does not allow any break possibilities with default widows and orphans. 4 lines creates one break possibility in the middle.
Vincent, would you please add a testcase for the outstanding bug? "* bug when the whole normal content is typeset and there are both footnotes and floats left to be placed;" Whenever possible with reasonable effort, I create a testcase before I start bugfixing. That documents the bug with a concrete and debug-friendly example and lets you know when you've finished fixing the bug. Even if you don't fix the bug right away, it is still helpful, because anyone else can start on the bugfix right away if he/she has time. "Test first", as in XP. Only good experiences so far. Note that adding a disabled testcase to the list automatically updates the website with "known issues" (at least after next site deployment, see http://xmlgraphics.apache.org/fop/knownissues.html).
Created attachment 18666 [details] Disabled testcases for (known) non-working features; patch against the Temp_Floats branch.
(In reply to comment #12) > Vincent, would you please add a testcase for the outstanding bug? > "* bug when the whole normal content is typeset and there are both footnotes and > floats left to be placed;" Done. I've also added a testcase for before-floats too large to even fit on one page alone. Vincent
change status from ASSIGNED to NEW for consistency