Bug 46315 - fox:needs-balancing extension
Summary: fox:needs-balancing extension
Status: NEW
Alias: None
Product: Fop - Now in Jira
Classification: Unclassified
Component: general (show other bugs)
Version: trunk
Hardware: All All
: P3 enhancement
Target Milestone: ---
Assignee: fop-dev
URL:
Keywords:
Depends on: 46322
Blocks:
  Show dependency tree
 
Reported: 2008-12-01 02:36 UTC by Georg Datterl
Modified: 2012-04-11 06:17 UTC (History)
0 users



Attachments
Version 1 (8.84 KB, patch)
2008-12-01 02:37 UTC, Georg Datterl
Details | Diff
Version 1, modified to basically do the job (7.47 KB, patch)
2008-12-01 04:18 UTC, Vincent Hennebert
Details | Diff
Second try of patch (32.18 KB, patch)
2009-01-09 06:14 UTC, Georg Datterl
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Georg Datterl 2008-12-01 02:36:44 UTC
This is how far I got (if the attachment is correct):

Parameter value is transfered from fo:flow to Flow.
Parameter value is transfered from fo:block to Block.
Parameter is inherited from Flow if not specified in Block.

Parameter value is used in LayoutContext.

Open issues: 
How and where do I transfer the parameter value from Block to LayoutContext?
Comment 1 Georg Datterl 2008-12-01 02:37:55 UTC
Created attachment 22970 [details]
Version 1
Comment 2 Vincent Hennebert 2008-12-01 04:18:44 UTC
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
Comment 3 Georg Datterl 2009-01-09 06:14:31 UTC
Created attachment 23098 [details]
Second try of patch

Please review and comment.
Comment 4 Vincent Hennebert 2009-01-12 03:40:09 UTC
(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
Comment 5 Vincent Hennebert 2009-01-13 03:34:11 UTC
(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
Comment 6 Vincent Hennebert 2009-01-28 07:40:30 UTC
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
Comment 7 Glenn Adams 2012-04-07 01:42:01 UTC
resetting P2 open bugs to P3 pending further review
Comment 8 Glenn Adams 2012-04-11 06:17:27 UTC
change status from ASSIGNED to NEW for consistency