https://issues.apache.org/bugzilla/show_bug.cgi?id=46905
--- Comment #31 from Vincent Hennebert <[email protected]> 2009-06-26 11:22:22 PST --- Hi Andreas, (In reply to comment #29) > Created an attachment (id=23865) --> (https://issues.apache.org/bugzilla/attachment.cgi?id=23865) [details] > diff of the changes, so far... > > As indicated earlier, the changes in the patch still cause some issues with > around 20 testcases. Just thought I'd post it for review to see if everyone is > OK with some of the refactoring. > > Basically, the most important changes in that respect are localized in > BreakingAlgorithm, where I extracted some of the code blocks in the main loop > in findBreakingPoints() into protected methods, offering hooks for subclasses > to inject custom behavior. An implementation may choose to treat penalties > differently than the basic algorithm does; the implementation of > handlePenaltyAt() in PageBreakingAlgorithm can serve as an example. > > Change with respect to the previous patch: PageBreakingAlgorithm keeps track > of > the current keep-context, and the last too-short node before the context > changed. If the keep-context is not AUTO, then PBA will restart from that > too-short node, instead of using the superclass' implementation of > recoverFromTooLong(). Then we add nodes after that node, until all the columns > in that page are occupied. Since we do not yet implement changing IPD (hence > no > change in column-count), I have explicitly chosen to defer only the > overflowing > part to the next page, for the sake of simplicity. The silent assumption is > that the next page will have the same number of columns, so deferring the > first > part as well is pointless... Best case showing that effect is block-4 in the > sample: we only defer the content of the block that must be kept within > one-page (starting with "[BOB-4a] ..."). I've had a quick look at your patch. I have 2 small comments: - there are two compilation errors: one in KnuthPenalty about an unknown PENALTY_TYPE constant, one in PageBreaker, l.421, trying to convert a List into a LinkedList (that one is easily fixed). - I think you can commit the changes that are clean-up 'only' right now. They are improving the code readability quite a bit. And a bit of nit-picking: - in BlockStackingLM: in the getKeep*Property methods, I chose to throw IllegalStateExceptions because the only LMs that don't override those methods are LMs to which keeps don't apply. So calling those methods on such LMs is a genuine programming mistake, and not a TODONotYetImplementedException. - there's no reason to make the PageBreakingAlgorithm class public - in PageBreakingAlgorithm.createFootnotePages: tmpLength can be declared inside the while loop - I see you changed the 'while (iter.hasNext())' loops into 'for (Iterator iter = list.iterator(); iter.hasNext();)' and... I just wanted to say that it's great ;-) I'll try to have a look at the bigger changes in [Page]BreakingAlgorithm later on. Thanks, Vincent -- Configure bugmail: https://issues.apache.org/bugzilla/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are the assignee for the bug.
