[
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
