Vincent Hennebert commented on FOP-2293:

Hi Seifeddine,

thanks for your patch. I noticed you refer to the 
 page on the developer wiki. It would be good if you could put your doc into a 
new section of that page, explaining what exactly you are planning to implement 
and any divergence from what is already there.

Some high-level comments about your patch:
* Please set up Checkstyle using the checkstyle-5.5.xml file at the root of the 
project and fix all the warnings. I already fixed some of them but not all. 
Among other things, it’s important to always put statements in {{if}} or 
{{else}} blocks into braces, even if there is only one statement, to prevent 
mistakes in case statements are added later on.
* FOP aims to be Java 1.5 compliant. Please compile the code with a 1.5 JDK, to 
make sure you are not using methods from the standard library that were added 
in Java 1.6 or later.
* Use the @Override annotation whenever you override a method. You should be 
able to set up your IDE to do that automatically for you.
* Try and use enhanced for loops rather than iterators whenever you can, as 
this usually makes the code much more concise. I did it in the filter method of 
FIRST_FIT as illustration
* The naming of extension elements will probably have to be revised to make 
them more concise and explicit. But we can handle that later on.
* Similarly, we will have to clarify whether the extension elements generate 
areas or simply return the areas from their child elements, whether that would 
be reference areas, etc.
* The fitting-strategy property should not be in the fox namespace since it’s 
not meant to be used on elements from other namespaces. Simply leave it without 
any namespace.
* The code doesn’t seem to work if the extension element is the last element of 
the flow.
* Eventually, support for changing IPD will have to be added. I don’t think 
that would work as it is?

And here are some finer-grain comments in specific parts of the code:

h6. BreakingAlgorithm
* handleBestFitPenalty
** Move this method to after considerLegalBreak, to follow the reading order 
(from the higher-level method to the lower-level method).
** totalWidth should not be updated if an alternative is found, since you’re 
dealing with a penalty element.
** Why should the test for difference be > 0 only? A negative difference is ok 
as long as the adjustment ratio is >= -1.
** Creating a new KnuthPenalty for every alternative is not ideal. It should be 
created once and stored in the alternative.
** alternativeManager will work only if there is only one extension in the 
document. If there are more, they will be mixed up in the same instance (see 
multiple-feasible-nodes.fo). Its logic should probably be moved to 
BestFitPenalty, which should also allow to avoid passing it around through 
* considerLegalBreak: the test ‘if element instanceof BestFitPenalty’ should 
not be put there as this method is also used for inline content, where such 
elements will never be present. It should be moved to a method that will be 
overridden in PageBreakingAlgorithm.
* if there’s no room on the page, the content will be unconditionally put on 
the next page. Is that what you want? I suppose this aspect is still work in 

h6. AlternativeManager
getBestAlternative: don’t catch the NPE. Change the code so that it tolerates a 
null value, or if you really can’t then add an ‘if != null’ test

h6. BestFitPenalty
instead of having a getAlternativeCount and a getAlternative(int) method, 
simply define one getAlternatives method that returns the list (or the 
collection if ordering is not important) of alternatives.

h6. ElementListUtils
* The calcFullContentLength shouldn’t be necessary: you can probably call 
SpaceResolver.resolveElementList first and then use the standard 
calcContentLength method.
* Eventually the creation of BestFitLayoutManagerMaker and 
AlternativeBlocklayoutManagerMaker will have to be moved to somewhere else than 
LayoutManagerMapping since they are extension elements. But we can address that 
later on.

h6. KnuthAlgorithmTestCase
This is an excellent start. The BestFitPenaltyTestCase inner class probably is 
unnecessary, the method can just be moved to the outer class. Eventually this 
test case can be completed to handle more cases (different and more complex 
situations), moved into their own class if necessary.

h6. AlternativeBlock
* I renamed FO_NAME into OBJECT_NAME as this is not a formatting object
* validateChildNode: I don’t think you want to check elements in the fox 
namespace. If you start like this you might as well check for elements in the 
SVG namespace, and the MathML namespace, etc. Some generic, all-purpose 
solution would have to be devised to check foreign elements, but this is 
getting off-topic.

h6. BestFit
* bind
** The determination of the strategy can be made a bit more robust by moving 
the code into the enum. Add a name field to each value of the enum and then use 
the values() method to iterate through them and return the appropriate one. See 
here for some hints:
I already modified the enum to replace the FittingStrategy class, which allows 
to simplify the code quite a bit.
** The validation of the property value should leverage FOP’s property 
sub-system. This can be done by defining the property using an 
EnumProperty.Maker instead of a StringProperty.Maker. Unfortunately, because 
that system was designed before the switch to Java 1.5, it would have to be 
modified to use Java enums. Maybe now is the time to do it. We can get back to 
that later.
* processNode: not sure why you redefine it?

h6. AlternativeBlockLayoutManager
* getNextKnuthElements: please, don’t do the same mistake as in other 
getNextKnuthElements methods and find a better name for the returnedList and 
returnList variables :-)
* addChildArea: not sure if the test curBlockArea != null is necessary, since 
it is initialized in getParentArea. Also, IIUC you are going to deal with block 
areas only, as per the definition of fox:alternative-block
* there is code duplicated from other LMs. This is fine for now, but eventually 
it would be good to remove that duplication from all the LMs. We can get back 
to that later.

I’ve applied your patch to a temporary branch: 
 Please switch your local copy to that branch and submit your next patch 
against it.

Feel free to ask if you have any questions.


> 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: bestfit.fo, doc.pdf, 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 is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira

Reply via email to