Vincent Hennebert commented on FOP-2293:

Hi Seifeddine,

thanks for your second patch. Some comments and answers to your questions:

* BestFit.bind: the {{for}} loop for getting the fitting strategy is great; 
just move it to the FittingStrategy class itself as it is of general use.

* BestFitLayoutManager.getNextKnuthElements: the comment at the end of the 
method is great to indicate that this is temporary and that you are planning to 
return to it later. The general practice in such a case is to add the TODO 
keyword, that will be highlighted by the IDE and is easily searchable.

bq. Creating a new KnuthPenalty for every alternative is not ideal. It should 
be created once and stored in the alternative.
Why do you say that? I don't create a KnuthPenalty for every alternative.

My bad, every penalty element is actually considered only once by the breaking 
algorithm. It would still be better though to move the creation of the penalty 
inside the alternative. That would be better encapsulation, and that would 
avoid you to create another one in the else part at the end of the method.

bq. validateChildNode: I don’t think you want to check elements in the fox 
namespace. If you start like this you might as well check for elements in the 
SVG namespace, and the MathML namespace, etc. Some generic, all-purpose 
solution would have to be devised to check foreign elements, but this is 
getting off-topic.
-> Well, I did it because I don't want nested alternative-block or best-fit. I 
understand your point but what else can I do to prevent the user from messing 
it up? Maybe the best way would be something like this:

You make a good point. However, you will also have to consider more complex 
cases like deep nesting: for example, a best-fit block inside a block inside an 
alternative-block inside a best-fit block.

bq. totalWidth should not be updated if an alternative is found, since you’re 
dealing with a penalty element.
That broke the JUnit test. It no longer gives the expected result.

The test still passes on my side?

The multiple-feasible-nodes.fo file is attached to this JIRA issue.

To conclude with the namespace issue: like Pascal said, since the 
fitting-strategy property is meant to be used on a fox element only, it 
shouldn’t be put in the fox namespace.

Regarding the FOP wiki: did you try to create an account?


> 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: bestfit.fo, doc.pdf, multiple-feasible-nodes.fo, 
> patch.patch, patch-rev1.1.patch, patch-rev1.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 is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to