[jira] [Commented] (FOP-2293) Whitespace management extension

["Vincent Hennebert (JIRA)" <ji...@apache.org>]

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

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

Hi Seifeddine,

thanks for your patch. I noticed you refer to the [WhitespaceManagement|http://wiki.apache.org/xmlgraphics-fop/WhitespaceManagement] 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 LayoutContext.
* 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 progress.

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:
[http://docs.oracle.com/javase/tutorial/java/javaOO/enum.html]
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: [https://svn.eu.apache.org/repos/asf/xmlgraphics/fop/branches/Temp_WhitespaceManagement] Please switch your local copy to that branch and submit your next patch against it.

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