yes, please coordinate with vincent On Friday, November 8, 2013, Dridi Seifeddine wrote:
> I don’t believe so. Should I sign one and send it to the Apache > administration? > > > > *De :* Glenn Adams [mailto:gl...@skynav.com <javascript:_e({}, 'cvml', > 'gl...@skynav.com');>] > *Envoyé :* jeudi 7 novembre 2013 22:36 > *À :* FOP Developers > *Objet :* Re: [jira] [Commented] (FOP-2293) Whitespace management > extension > > > > Is there an CLA on file for Seiffidine? > > > > On Thu, Nov 7, 2013 at 2:11 PM, Vincent Hennebert (JIRA) <j...@apache.org> > wrote: > > > [ > https://issues.apache.org/jira/browse/FOP-2293?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13816427#comment-13816427] > > Vincent Hennebert commented on FOP-2293: > ---------------------------------------- > > Hi Seifeddine, > > thanks for your patch. I’ve applied it in [rev. 1539809| > http://svn.apache.org/r1539809]. I have the following questions and > comments: > > * FOPropertyMapping: The fox:fitting-strategy property is no longer needed > is it? Same for Alternative.FittingStrategy. > * BestFit: same here? > * MultiCase.isActuated: not sure what you have in mind here? A multi-case > cannot be actuated? Only a multi-toggle can. This method is used in > MultiSwitch.finalizeNode, but I don’t get the point of that latter method? > * MultiToggle: why bother implementing it? AFAIU you won’t need it in your > extension. If you implement it you have to test it... > > I didn’t include the test case as I think it still needs a bit of work: > * I wouldn’t qualify nested multi-switch elements as simple test. Maybe > you want to concentrate on the basics first and handle advanced use cases > later. > * Instead of using space-after to artificially grow the size of a > multi-case, you probably want to use a series of block elements. Space is > subject to element resolution (See SpaceResolver.resolveElementList), and > may disappear as the result of that. At any rate, it should in the present > case since the space is conditional and would end a reference-area. > All that to say: better use plain blocks and leave complex content for > later, once the basics are working. > * It would be good if the test case were reflecting the choice of a > variant at the bottom of a page. In this test the multi-switch elements are > found in the middle of the page, which AFAIU you haven’t implemented yet. > * Please remove tabs from XML files and use a two-space indentation: tabs > are as troublesome in XML as they are in Java. > > When running the test case I noticed that the following warning is still > issued: “The following feature isn’t implemented by Apache FOP, yet: > fo:multi-switch”. You may want to fix that. > > I noticed you tried a different approach at managing variants, by > detecting page overflows and restarting the breaking algorithm at an > earlier node after having discarded the current variant and moved on to the > next one. That seems like a good idea, but the code is deceptively simple > and I think you will get in trouble as you move foward in the > implementation: > * Imagine the following test case: let’s call E the element just before > the multi-switch, and assume that there is a legal break after that > element. But that legal break results into a tooShortNode. Also, the first > variant of the multi-switch leads to a tooLongNode, but the second variant > perfectly fits on the page. When you are at the multi-switch, you will have > a tooLongNode, the number of active nodes will drop to zero, which triggers > the node recovery mechanism. Since there is a tooShortNode available, the > algorithm will pick it (it’s better to have not enough content on a page > than overflowing content that may be clipped). So the final break will be > at element E, and the user will wonder why FOP broke the page there while > there was enough room on the page to fit the second variant. > * Even if you find a fix for the preceding case, restarting at the node > before every time a variant must be skipped is sub-optimal and creates > unnecessary work for the breaking algorithm. > * The node recovery mechanism is particularly tricky and hard to > understand, so fiddling with it is not advised unless there is no other > option... > > I suggest you carry on with the approach you had in your previous patches, > which I think was integrating better with the algorithm, and in the end > will lead to more robust and simpler code. You ‘just’ have to pass > information to the KnuthPageNode about which variant of the multi-switch is > associated to it, and all the way down the line until you are back in > MultiSwitchLM. > > Feel free to ask if you have any questions. > > Thanks, > Vincent > > > > Whitespace management extension > > ------------------------------- > > > > Key: FOP-2293 > > URL: <https://issues.apache.org/jira/browse/FOP-2293> >
