[
https://issues.apache.org/jira/browse/FOP-2293?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&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
comments:
* 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.
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_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)
