| Summary: | fox:needs-balancing extension | ||
|---|---|---|---|
| Product: | Fop - Now in Jira | Reporter: | Georg Datterl <gd> |
| Component: | general | Assignee: | fop-dev |
| Status: | NEW --- | ||
| Severity: | enhancement | ||
| Priority: | P3 | ||
| Version: | trunk | ||
| Target Milestone: | --- | ||
| Hardware: | All | ||
| OS: | All | ||
| Bug Depends on: | 46322 | ||
| Bug Blocks: | |||
| Attachments: |
Version 1
Version 1, modified to basically do the job Second try of patch |
||
|
Description
Georg Datterl
2008-12-01 02:36:44 UTC
Created attachment 22970 [details]
Version 1
Created attachment 22971 [details]
Version 1, modified to basically do the job
Hi Georg,
Thanks for the patch. This is basically what needs to be done. I attach a modified version of your patch with the following comments:
- you don't need to do anything on the Flow object actually. Since the property is defined as inherited, the property sub-system will take care of this automatically.
- it's best to move the definition of the property from the createBlockAndLineProperties method to createLayoutProperties (where the span property is also defined)
- you can re-use the genericBoolean field in FOPropertyMapping
- the default value is true and not inherit. The inherit characteristic is defined separately
- you can't play with LayoutContext in that way. The value of the property needs to be known before Knuth elements are added to the element list (I don't want to enter the details too much). Your best bet is to mimic the way the span property is handled, see FlowLayoutManager in the attached patch
Please have a look at the attached patch; it worked for me on a basic example but more extensive testing is needed. To ensure it doesn't break anything you can run 'ant junit' on the command line at the base of the project. It will run all the test and print a big fat warning in case one is broken.
Next step:
- clean up a bit
- make the modifications adhere to FOP's standard (see checkstyle-4.0.xml at the root of the project)
- add a test case for the new feature (see the test/layoutengine/standard-testcases/ directory)
Thanks!
Vincent
Created attachment 23098 [details]
Second try of patch
Please review and comment.
(In reply to comment #3) > Created an attachment (id=23098) [details] > Second try of patch > > Please review and comment. > Hi Georg, Thanks for the patch. It's almost ready to be committed, but I'd like to simplify the test case a bit (remove the inline elements and the Arial font specification). Unfortunately that affects line height so the whole test needs to be re-worked to give to same results as before. I'll probably finish that tomorrow. Vincent (In reply to comment #4) > (In reply to comment #3) > > Created an attachment (id=23098) [details] [details] > > Second try of patch > > > > Please review and comment. > > > > Hi Georg, > > Thanks for the patch. It's almost ready to be committed, but I'd like to > simplify the test case a bit (remove the inline elements and the Arial font > specification). Unfortunately that affects line height so the whole test needs > to be re-worked to give to same results as before. I'll probably finish that > tomorrow. > > Vincent It appears that bug #46322 is on the way of properly testing this extension, with the 'one line of the spanning block at the bottom of the page' problem. Either the test will need to be tweaked in order to work around the bug, at the risk of not being extensive enough, or it will have to be disabled until bug #46322 is fixed. More in the next episode... Vincent Patch finally applied. I let the bug open for now and add a dependency on bug #46322 as a reminder that this feature will need to be re-checked when that other bug is fixed. Thanks! Vincent resetting P2 open bugs to P3 pending further review change status from ASSIGNED to NEW for consistency |