Details

    • Type: Improvement Improvement
    • Status: Open
    • Resolution: Unresolved
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: unqualified
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All
    • External issue ID:
      46315

      Description

      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?

      1. fox_needs-balancing_incomplete.patch
        9 kB
        Georg Datterl
      2. fox_needs-balancing_2.patch
        7 kB
        Vincent Hennebert
      3. fox_disable-column-balancing_v2.patch
        32 kB
        Georg Datterl

        Issue Links

          Activity

          Hide
          Georg Datterl added a comment -

          Attachment fox_needs-balancing_incomplete.patch has been added with description: Version 1

          Show
          Georg Datterl added a comment - Attachment fox_needs-balancing_incomplete.patch has been added with description: Version 1
          Hide
          Vincent Hennebert added a comment -

          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

          Show
          Vincent Hennebert added a comment - 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
          Hide
          Vincent Hennebert added a comment -

          Attachment fox_needs-balancing_2.patch has been added with description: Version 1, modified to basically do the job

          Show
          Vincent Hennebert added a comment - Attachment fox_needs-balancing_2.patch has been added with description: Version 1, modified to basically do the job
          Hide
          Georg Datterl added a comment -

          Please review and comment.

          Show
          Georg Datterl added a comment - Please review and comment.
          Hide
          Georg Datterl added a comment -

          Attachment fox_disable-column-balancing_v2.patch has been added with description: Second try of patch

          Show
          Georg Datterl added a comment - Attachment fox_disable-column-balancing_v2.patch has been added with description: Second try of patch
          Hide
          Vincent Hennebert added a 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

          Show
          Vincent Hennebert added a 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
          Hide
          Vincent Hennebert added a comment -

          (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 FOP-1607 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 FOP-1607 is fixed. More in the next episode...

          Vincent

          Show
          Vincent Hennebert added a comment - (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 FOP-1607 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 FOP-1607 is fixed. More in the next episode... Vincent
          Hide
          Vincent Hennebert added a comment -

          Patch finally applied. I let the bug open for now and add a dependency on FOP-1607 as a reminder that this feature will need to be re-checked when that other bug is fixed.

          Thanks!
          Vincent

          Show
          Vincent Hennebert added a comment - Patch finally applied. I let the bug open for now and add a dependency on FOP-1607 as a reminder that this feature will need to be re-checked when that other bug is fixed. Thanks! Vincent
          Hide
          Glenn Adams added a comment -

          resetting P2 open bugs to P3 pending further review

          Show
          Glenn Adams added a comment - resetting P2 open bugs to P3 pending further review
          Hide
          Glenn Adams added a comment -

          change status from ASSIGNED to NEW for consistency

          Show
          Glenn Adams added a comment - change status from ASSIGNED to NEW for consistency

            People

            • Assignee:
              Unassigned
              Reporter:
              Georg Datterl
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development