I don’t believe so. Should I sign one and send it to the Apache administration?


De : Glenn Adams [mailto: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> 

 &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 

* 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.


> 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: 
> http://wiki.apache.org/xmlgraphics-fop/WhitespaceManagement
> 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


Reply via email to