Good point. Seifeddine, please follow the instructions on this page to
sign and send you ICLA:
http://www.apache.org/licenses/#clas
It will be necessary when your work is merged to trunk.

Thanks,
Vincent


On 08/11/13 17:45, Glenn Adams wrote:
yes, please coordinate with vincent

On Friday, November 8, 2013, Dridi Seifeddine wrote:

I don’t believe so. Should I sign one and send it to the Apache
administration?



*De :* Glenn Adams [mailto:gl...@skynav.com <javascript:_e({}, 'cvml',
'gl...@skynav.com');>]
*Envoyé :* jeudi 7 novembre 2013 22:36
*À :* FOP Developers
*Objet :* Re: [jira] [Commented] (FOP-2293) Whitespace management
extension



Is there an CLA on file for Seiffidine?



On Thu, Nov 7, 2013 at 2:11 PM, Vincent Hennebert (JIRA) <j...@apache.org>
wrote:


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


Reply via email to