Vincent Hennebert commented on FOP-2293:

Hi Seifeddine,

this is looking good. Just a few things:

bq. This dynamicNode boolean doesn't seem necessary to me. Why would you prefer 
a node to another just because it corresponds to a multi-switch? It's better to 
rely on the progress made in the Knuth sequence like is done in the default 

That perplexed me for a while. For instance, if we have 2 possible layouts: one 
where the 1st variant of the dynamic content doesn't fit and another case where 
the 2nd variant fits perfectly. The first possibility should have fewer total 
demerits than the second one. Which one should we choose?

Not sure what you mean here. If the 1st variant doesn't fit... well it doesn't 
fit :-) Did you mean, where the 1st variant _does_ fit?

I might see where you're going though: what if with one active node, it's the 
1st variant that fits, and with another active node, it's the 2nd? I see two 
possibilities: either select only one best node (per fitness class), that is 
the one with the fewest total demerits. Or select a best node per fitness class 
_and_ per selected variant. But that will increase the number of active nodes, 
and in practice might have little use.

For now, I would remove that dynamicNode boolean and compute demerits as usual.

bq. findBestFitPenalty: not sure why this method is necessary. If a node 
contains a non-null variant, that necessarily means that the element pointed to 
by the position field is the corresponding BestFitPenalty?

Not necessarily. First, because the par member variable is susceptible to 
change, hence invalidating the position field in the Knuth node. Second, there 
is an infinitely stretching glue at the end of the Knuth sequence which makes 
the algorithm create a new finalizing node.

I don't have any case in mind where the par variable is being modified _while_ 
the breaking algorithm is run? As to the second point, if the node doesn't 
point to the special penalty, that means that the multi-switch doesn't end the 
page. So it shouldn't be taken into account, not through the penalty at least. 
We are in the "middle of the page" case that we can tackle later on by adding a 
glue "at the right place" in the Knuth sequence.

And a couple other things:
* why this resetCurrentVariant? Can you not just reset it at the beginning of 
* multi-switch_best-fit.xml: I know that it's possible to check on the Knuth 
sequence, but in practice it's rarely done, and not ideal. We've had cases in 
the past where, after a slight alteration in the Knuth element generation, we 
had to re-visit all those test cases and update the checks, a rather painful 
process. A unit test would probably be better suited, where you can mock 
anything you want (using Mockito) and have more targeted checks. That said, 
this test case is definitely a good start to add checks on the area tree. 


> 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_best-fit_ext_rev3.patch, 
> FO_multi-switch_best-fit_ext_rev4.patch, 
> FO_multi-switch_best-fit_ext_rev5.patch, 
> FO_multi-switch_best-fit_ext_rev6.patch, 
> FO_multi-switch_best-fit_ext_rev7.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, two-valid-variants.fo
> 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