Seifeddine Dridi commented on FOP-2293:

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.
I don't see things like that. A penalty is used to separate lines/paragraphs, 
but what if an FO contains only a multi-switch. Without the empty box at the 
start, the layout manager would think that the best-fit penalty is useless and 
so will proceed to deleting it, since there is really nothing to "break".

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.
I don't fully agree with you. While I think there might be inconsistencies 
between the information stored by the breaking algorithm and the 
WhiteSpaceManager class, I also believe that the type of information each one 
needs are not necessarily the same. I could have just augmented the KnuthNode 
like you said, but I have no control over the created Knuth nodes and they can 
be deleted/replaced at any time without my notice -- I prefer to leave my code 
separated from the Knuth algorithm and whenever an information is needed it 
will be requested-- that's why I created my own classes instead of relying on 
the existing ones. This will also ease maintenance later.

in computeDifference: I’m not too sure of the meaning of the added if (element 
instanceof BestFitPenalty) test. What do you have in mind?
Penalties which are too long (r < -1) are disabled very early, and they won't 
get registered in WhiteSpaceManager (see handleBestFitPenalty()).

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?
It's used to detect if a normal content appeared after an already registered 
best-fit penalty, in which case we either have to pick another variant hoping 
that it will be long enough to force the algorithm to make a new page, or 
remove the currently active one altogether.

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.
I'm aware of the potential problems with space-specifiers. Unfortunately, the 
space-resolution rules defined in the XSL spec are not trivial to understand, 
but I'm progressing nevertheless.

Thanks for your review, I appreciate your effort.

> 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, 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