[ 
https://issues.apache.org/jira/browse/FOP-2293?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13827905#comment-13827905
 ] 

Vincent Hennebert commented on FOP-2293:
----------------------------------------

Hi Seifeddine,

thanks for your patch. I’ve partially applied it in [rev. 
1543891|http://svn.apache.org/r1543891].

The approach is taking shape. A few comments:

In BestFitLayoutUtils:
* calculateLength: there’s no need to add a dummy penalty at the beginning of 
the list. Yes, there are issues with space resolution, but if you add the 
penalty you will run into some bugs and if you remove it you will run into 
other bugs. The real solution to the problem is to change space resolution to 
perform partial resolution on a subset of the Knuth sequence, and refine it 
later on. But this is out of the scope of this work and can be left as is for 
now (for that matter, footnotes have been suffering from the same problem for 
years).
* getKnuthList: there’s no need to add an empty box before the BestFitPenalty. 
A penalty doesn’t need to be preceded by a box to be a legal break point, only 
a glue does.

In PageBreakingAlgorithm:
* The WhiteSpaceManager class in PageBreakingAlgorithm looks problematic to me. 
Information about a page break can now be found at two places: in this class, 
and in KnuthPageNode. This will inevitably create inconsistencies and 
difficulties to manage state that is distributed over several entities. This is 
illustrated by the fact that you have to manage a list of alternatives that 
(IIUC) will depend on the selected break point. That list is already there in 
the form of KnuthPageNodes! The KnuthPageNode class should just be augmented to 
contain the selected variant of the multi-switch, if any.
* in computeDifference: I’m not too sure of the meaning of the added {{if 
(element instanceof BestFitPenalty)}} test. What do you have in mind? Also, the 
second test {{if (diff > 0 && dynamicContentBPD > 0 && !(element instanceof 
BestFitPenalty))}}: is it to handle the case where a multi-switch is in the 
middle of a page? In that case, you can’t simply deactivate a variant. It’s 
perfectly possible to have one layout where the multi-switch is at the bottom 
of a page, and one where it’s in the middle of a page. Which layout will be 
selected in the end depends on what follows. The middle-of-page case will be 
handled by inserting a glue at the right place in the Knuth sequence, but you 
can leave that for now and concentrate on page breaks.
* in handleBestFitPenalty: again, just because in one case there is no space 
for a variant doesn’t mean that there won’t be space in any other cases. See 
uploaded file two-valid-variants.fo. Comment out the multi-switch and play with 
the space-before.minimum value: if left to 20pt, the variant with 2 lines on 
the first page is selected, if set to 15pt, the variant with 3 lines on the 
first page is selected. Once you re-enable the multi-switch, that will have an 
impact on which variant will be selected: the one-line variant in the first 
case, the two-line variant in the second case.


I really encourage you to create a simple layout test to check your 
implementation. This two-valid-variants example could be a good start. Also, 
you may want to run a test coverage tool to ensure that the code you write is 
actually run and tested.

Thanks,
Vincent


> 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_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
(v6.1#6144)

Reply via email to