Is there an CLA on file for Seiffidine?
On Thu, Nov 7, 2013 at 2:11 PM, Vincent Hennebert (JIRA) <j...@apache.org>wrote:
> 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
> * 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
> * 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
> * 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
> 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
> Feel free to ask if you have any questions.
> > Whitespace management extension
> > -------------------------------
> > Key: FOP-2293
> > URL: https://issues.apache.org/jira/browse/FOP-2293
> > Project: Fop
> > Issue Type: New Feature
> > Components: general
> > Affects Versions: trunk
> > Reporter: Seifeddine Dridi
> > Priority: Minor
> > Labels: XSL-FO
> > Fix For: trunk
> > Attachments: FO_multi-switch_best-fit_ext_rev2.patch,
> FO_multi-switch_test.fo, FO_multi-switch_with_best-fit_extension.patch,
> bestfit.fo, doc.pdf, multi-switch_bestfit.fo, multiple-feasible-nodes.fo,
> patch-rev1.1.patch, patch-rev1.patch, patch-rev2.patch, patch.patch
> > I have been working on an extension for whitespace management, similar
> to what's described here:
> > The logic of the extension is very simple: the user defines a set of
> alternatives that he wishes to insert at the end of a page, then if there
> is enough space left, FOP will pick the alternative that best matches the
> user's selection criteria (first fit, smallest fit, biggest fit).
> > This is my first work on FOP and it took me almost 2 months to reach
> this stage in development. But it's not the end of course, so I'm relying
> on your feedback to improve it.
> > Thank you
> This message was sent by Atlassian JIRA