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 [ <javascript:_e({}, 'cvml',
> '');>]
> *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) <>
> wrote:
>     [
> Vincent Hennebert commented on FOP-2293:
> ----------------------------------------
> Hi Seifeddine,
> thanks for your patch. I’ve applied it in [rev. 1539809|
>]. 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: <>

Reply via email to