FOP
  1. FOP
  2. FOP-1671

[PATCH] Support for bleed, trim and crop box and scaling

    Details

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

      Description

      The attached patch adds support for 4 new simple-page-master fop extension attributes, namely

      • fox:crop-box
      • fox:trim-box
      • fox:bleed-box
      • fox:scale

      The box attributes can consist out of up to 4 numeric values (top,left, width, height) and can have units as suffix to each of the numbers.

      The scale attribute can consist out of two numbers each between 0 and 1

      It is implemented for PDF and Java2D renderers.

      1. FOP_scalePage_CropBox_BleedBox_TrimBox_AWTRenderer_v1.patch
        2 kB
        Peter Coppens
      2. FOP_scalePage_CropBox_BleedBox_TrimBox_v4.patch
        18 kB
        Peter Coppens
      3. FOP_scalePage_CropBox_BleedBox_TrimBox_v5.patch
        28 kB
        Peter Coppens
      4. FOP_scalePage_CropBox_BleedBox_TrimBox_v6.patch
        28 kB
        Peter Coppens
      5. FOP_scalePage_CropBox_BleedBox_TrimBox_v7.patch
        30 kB
        Peter Coppens
      6. FOP_scalePage_CropBox_BleedBox_TrimBox_v9.patch
        30 kB
        Peter Coppens
      7. PageBoundaries_fix.patch
        0.7 kB
        Boris Y.
      8. prepress.diff
        35 kB
        Vincent Hennebert
      9. PrepressPDF_0.95.patch
        53 kB
        Boris Y.

        Activity

        Glenn Adams made changes -
        Affects Version/s trunk [ 12323672 ]
        Affects Version/s all [ 12323671 ]
        Gavin made changes -
        Field Original Value New Value
        Assignee fop-dev [ fop-dev@xmlgraphics.apache.org ]
        Hide
        Glenn Adams added a comment -

        increase priority for bugs with a patch

        Show
        Glenn Adams added a comment - increase priority for bugs with a patch
        Hide
        Jeremias Maerki added a comment -

        (In reply to comment #43)
        > Hi All,
        >
        > We have found one issue during testing this new feature.
        > The issue lies in PageBoundaries.java in calculating crop/bleed boxes.
        >
        > The offsets order is: [top, right, bottom, left], so to calculate Y size of
        > the final box we should use the 'bottom' instead of 'top' offset :
        >
        >
        > return new Rectangle(originalRect.x - coords[3],
        > - originalRect.y - coords[0],
        > + originalRect.y - coords[2],
        > originalRect.width + coords[3] + coords[1],
        > originalRect.height + coords[0] + coords[2]);
        >
        >
        >
        > Please find in the attachments the fix patch. (Comment#41)
        > Also I have attached the full patch for FOP-0.95 version (Comment#42) if
        > somebody will have a need to use this feature with previous version.

        I've taken a look at that. Thanks for spotting the problem, Boris, but your solution was not the right one. But you brought me on the right track. I've just found out what our mistake is: PDF specifies the boxes as "rectangles" which are defined as "llx lly urx ury" (i.e. lower left to upper right). But our/FOP's Rectangle2D objects are actually "upper left to lower right". In PageBoundaries we're still in FOP's coordinate system which starts at the upper left. So we have to calculate the right values for the default PDF coordinate system. Boris' change would have broken a test case and created a bug on the bitmap production side. So the right change is to do a transformation from FOP's internal coordinate system to PDF's default one in PDFDocumentHandler: http://svn.apache.org/viewvc?rev=884241&view=rev

        Boris, can you please verify that this fix also work for you? Thanks!

        Show
        Jeremias Maerki added a comment - (In reply to comment #43) > Hi All, > > We have found one issue during testing this new feature. > The issue lies in PageBoundaries.java in calculating crop/bleed boxes. > > The offsets order is: [top, right, bottom, left] , so to calculate Y size of > the final box we should use the 'bottom' instead of 'top' offset : > > > return new Rectangle(originalRect.x - coords [3] , > - originalRect.y - coords [0] , > + originalRect.y - coords [2] , > originalRect.width + coords [3] + coords [1] , > originalRect.height + coords [0] + coords [2] ); > > > > Please find in the attachments the fix patch. (Comment#41) > Also I have attached the full patch for FOP-0.95 version (Comment#42) if > somebody will have a need to use this feature with previous version. I've taken a look at that. Thanks for spotting the problem, Boris, but your solution was not the right one. But you brought me on the right track. I've just found out what our mistake is: PDF specifies the boxes as "rectangles" which are defined as "llx lly urx ury" (i.e. lower left to upper right). But our/FOP's Rectangle2D objects are actually "upper left to lower right". In PageBoundaries we're still in FOP's coordinate system which starts at the upper left. So we have to calculate the right values for the default PDF coordinate system. Boris' change would have broken a test case and created a bug on the bitmap production side. So the right change is to do a transformation from FOP's internal coordinate system to PDF's default one in PDFDocumentHandler: http://svn.apache.org/viewvc?rev=884241&view=rev Boris, can you please verify that this fix also work for you? Thanks!
        Hide
        Boris Y. added a comment -

        Hi All,

        We have found one issue during testing this new feature.
        The issue lies in PageBoundaries.java in calculating crop/bleed boxes.

        The offsets order is: [top, right, bottom, left], so to calculate Y size of the final box we should use the 'bottom' instead of 'top' offset :

        return new Rectangle(originalRect.x - coords[3],

        • originalRect.y - coords[0],
          + originalRect.y - coords[2],
          originalRect.width + coords[3] + coords[1],
          originalRect.height + coords[0] + coords[2]);

        Please find in the attachments the fix patch. (Comment#41)
        Also I have attached the full patch for FOP-0.95 version (Comment#42) if somebody will have a need to use this feature with previous version.

        Show
        Boris Y. added a comment - Hi All, We have found one issue during testing this new feature. The issue lies in PageBoundaries.java in calculating crop/bleed boxes. The offsets order is: [top, right, bottom, left] , so to calculate Y size of the final box we should use the 'bottom' instead of 'top' offset : return new Rectangle(originalRect.x - coords [3] , originalRect.y - coords [0] , + originalRect.y - coords [2] , originalRect.width + coords [3] + coords [1] , originalRect.height + coords [0] + coords [2] ); Please find in the attachments the fix patch. (Comment#41) Also I have attached the full patch for FOP-0.95 version (Comment#42) if somebody will have a need to use this feature with previous version.
        Hide
        Boris Y. added a comment -

        Attachment PrepressPDF_0.95.patch has been added with description: Full patch for FOP-0.95 version

        Show
        Boris Y. added a comment - Attachment PrepressPDF_0.95.patch has been added with description: Full patch for FOP-0.95 version
        Hide
        Boris Y. added a comment -

        Attachment PageBoundaries_fix.patch has been added with description: Fix for /trunk

        Show
        Boris Y. added a comment - Attachment PageBoundaries_fix.patch has been added with description: Fix for /trunk
        Hide
        Vincent Hennebert added a comment -

        (In reply to comment #39)
        > 3 of the 4 latest comments have been hopefully satisfyingly taken care of:
        > http://svn.apache.org/viewvc?rev=800401&view=rev

        This is looking much better now, thanks. There are still a few remaining issues that I'll handle in the next days.

        Show
        Vincent Hennebert added a comment - (In reply to comment #39) > 3 of the 4 latest comments have been hopefully satisfyingly taken care of: > http://svn.apache.org/viewvc?rev=800401&view=rev This is looking much better now, thanks. There are still a few remaining issues that I'll handle in the next days.
        Hide
        Jeremias Maerki added a comment -

        3 of the 4 latest comments have been hopefully satisfyingly taken care of:
        http://svn.apache.org/viewvc?rev=800401&view=rev

        Show
        Jeremias Maerki added a comment - 3 of the 4 latest comments have been hopefully satisfyingly taken care of: http://svn.apache.org/viewvc?rev=800401&view=rev
        Hide
        Vincent Hennebert added a comment -

        (In reply to comment #37)
        > Applied the patch with the changes already mentioned. Thanks a lot for your
        > patch Peter, and thanks also for your patience with my late involvement.
        > http://svn.apache.org/viewvc?rev=800142&view=rev
        >
        > I hope we will have a chance some day to improve the property subsystem so we
        > can also handle extension like this more elegantly.

        This is not quite finished I'm afraid:

        • I thought we agreed to set the default value of crop-offset to bleed
        • errors in the specification of extension properties are not redirected through the event mechanism
        • there are typos in the PageBoundariesAttributes and PageScaleAttributes classes
        • there is an encapsulation problem: the fall back boxes used when one of the properties has not been defined are controlled by the client code, instead of being handled inside the extension class itself (PageBoundariesAttributes). This can easily lead to inconsistencies (one default value used in the PDF renderer, another one in the Java2D renderer).

        Re-opening the bug as a reminder, as I don't have the time to handle that right now.

        Show
        Vincent Hennebert added a comment - (In reply to comment #37) > Applied the patch with the changes already mentioned. Thanks a lot for your > patch Peter, and thanks also for your patience with my late involvement. > http://svn.apache.org/viewvc?rev=800142&view=rev > > I hope we will have a chance some day to improve the property subsystem so we > can also handle extension like this more elegantly. This is not quite finished I'm afraid: I thought we agreed to set the default value of crop-offset to bleed errors in the specification of extension properties are not redirected through the event mechanism there are typos in the PageBoundariesAttributes and PageScaleAttributes classes there is an encapsulation problem: the fall back boxes used when one of the properties has not been defined are controlled by the client code, instead of being handled inside the extension class itself (PageBoundariesAttributes). This can easily lead to inconsistencies (one default value used in the PDF renderer, another one in the Java2D renderer). Re-opening the bug as a reminder, as I don't have the time to handle that right now.
        Hide
        Jeremias Maerki added a comment -

        Applied the patch with the changes already mentioned. Thanks a lot for your patch Peter, and thanks also for your patience with my late involvement.
        http://svn.apache.org/viewvc?rev=800142&view=rev

        I hope we will have a chance some day to improve the property subsystem so we can also handle extension like this more elegantly.

        Show
        Jeremias Maerki added a comment - Applied the patch with the changes already mentioned. Thanks a lot for your patch Peter, and thanks also for your patience with my late involvement. http://svn.apache.org/viewvc?rev=800142&view=rev I hope we will have a chance some day to improve the property subsystem so we can also handle extension like this more elegantly.
        Hide
        Jeremias Maerki added a comment -

        (In reply to comment #34)
        > (In reply to comment #33)
        > > Peter, why can fox:scale not have any number greater than 1? Why only support
        > > shrinking but not upsizing?
        >
        > Hmmm...probably just an oversight on our end. For the sake of generality it
        > should be possible to also use >1 values. You want us to send a new patch?

        Not necessary, thanks. I'll just change that myself then.

        Show
        Jeremias Maerki added a comment - (In reply to comment #34) > (In reply to comment #33) > > Peter, why can fox:scale not have any number greater than 1? Why only support > > shrinking but not upsizing? > > Hmmm...probably just an oversight on our end. For the sake of generality it > should be possible to also use >1 values. You want us to send a new patch? Not necessary, thanks. I'll just change that myself then.
        Hide
        Peter Coppens added a comment -

        (In reply to comment #32)
        > Thanks for the new patch, Peter. I've taken a look and found a few issues. I've
        > already started fixing them. Among them:
        > - In the Wiki I've switched the meaning for crop-offset to align with what
        > AntennaHouse did (see their illustration): crop-offset expands from the
        > TrimBox, not the BleedBox. That is currently not reflected in the code, so I'll
        > change that, too, if there's no opposition.
        Ok for me.

        Show
        Peter Coppens added a comment - (In reply to comment #32) > Thanks for the new patch, Peter. I've taken a look and found a few issues. I've > already started fixing them. Among them: > - In the Wiki I've switched the meaning for crop-offset to align with what > AntennaHouse did (see their illustration): crop-offset expands from the > TrimBox, not the BleedBox. That is currently not reflected in the code, so I'll > change that, too, if there's no opposition. Ok for me.
        Hide
        Peter Coppens added a comment -

        (In reply to comment #33)
        > Peter, why can fox:scale not have any number greater than 1? Why only support
        > shrinking but not upsizing?

        Hmmm...probably just an oversight on our end. For the sake of generality it should be possible to also use >1 values. You want us to send a new patch?

        Show
        Peter Coppens added a comment - (In reply to comment #33) > Peter, why can fox:scale not have any number greater than 1? Why only support > shrinking but not upsizing? Hmmm...probably just an oversight on our end. For the sake of generality it should be possible to also use >1 values. You want us to send a new patch?
        Hide
        Jeremias Maerki added a comment -

        Peter, why can fox:scale not have any number greater than 1? Why only support shrinking but not upsizing?

        Show
        Jeremias Maerki added a comment - Peter, why can fox:scale not have any number greater than 1? Why only support shrinking but not upsizing?
        Hide
        Jeremias Maerki added a comment -

        Thanks for the new patch, Peter. I've taken a look and found a few issues. I've already started fixing them. Among them:

        • In the Wiki I've switched the meaning for crop-offset to align with what AntennaHouse did (see their illustration): crop-offset expands from the TrimBox, not the BleedBox. That is currently not reflected in the code, so I'll change that, too, if there's no opposition.
        • In the AWT preview, the positioning wasn't ok, yet, when there's a bleed or crop-offset.
        • The Java2DRenderer generates an ugly page border which I've disabled locally. Not sure why we even had that in place. It doesn't make much sense.
        • Also, I've changed the background painting in the Java2DRenderer to use the BleedBox instead of the page size.

        Otherwise, the patch makes a good impression. The whole thing is now very intuitive to use, just like I imagined it should be. I'll just allow some time for additional feedback from others. In the meantime, I'll finish the changes to the patch I've started and finally commit the whole thing. BTW, I've also written a little demo FO which demonstrates the features and how I would go about doing crop marks with SVG. I'll commit that after the patch is processed.

        Show
        Jeremias Maerki added a comment - Thanks for the new patch, Peter. I've taken a look and found a few issues. I've already started fixing them. Among them: In the Wiki I've switched the meaning for crop-offset to align with what AntennaHouse did (see their illustration): crop-offset expands from the TrimBox, not the BleedBox. That is currently not reflected in the code, so I'll change that, too, if there's no opposition. In the AWT preview, the positioning wasn't ok, yet, when there's a bleed or crop-offset. The Java2DRenderer generates an ugly page border which I've disabled locally. Not sure why we even had that in place. It doesn't make much sense. Also, I've changed the background painting in the Java2DRenderer to use the BleedBox instead of the page size. Otherwise, the patch makes a good impression. The whole thing is now very intuitive to use, just like I imagined it should be. I'll just allow some time for additional feedback from others. In the meantime, I'll finish the changes to the patch I've started and finally commit the whole thing. BTW, I've also written a little demo FO which demonstrates the features and how I would go about doing crop marks with SVG. I'll commit that after the patch is processed.
        Hide
        Peter Coppens added a comment -

        Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v9.patch has been added with description: Patch including Jeremias'es proposed changes

        Show
        Peter Coppens added a comment - Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v9.patch has been added with description: Patch including Jeremias'es proposed changes
        Hide
        Jeremias Maerki added a comment -

        I've started a Wiki page for the prepress feature: http://wiki.apache.org/xmlgraphics-fop/PrepressSupport

        I'll follow up with some additional thoughts on the fop-dev list as discussion in Bugzilla is a bit awkward.

        Show
        Jeremias Maerki added a comment - I've started a Wiki page for the prepress feature: http://wiki.apache.org/xmlgraphics-fop/PrepressSupport I'll follow up with some additional thoughts on the fop-dev list as discussion in Bugzilla is a bit awkward.
        Hide
        Vincent Hennebert added a comment -

        (In reply to comment #26)
        > Hi Vincent,
        >
        > (In reply to comment #22)
        <snip/>
        > > A note about the patch: it seemed more sensible to me to deal with integer
        > > values rather than double. Indeed every length value is internally converted
        > > into a whole number expressed in mpt (AFAICT). That means that I'm using
        > > Rectangle instead of Rectangle2D, and I changed some parameters into some
        > > classes accordingly (mainly, o.a.f.layoutmgr.Page.java,
        > > o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads
        > > the value of the "bounds" attribute as double instead of integer. AFAIK, the
        > > XML area tree is produced with whole numbers for the page boundaries; I don't
        > > see why a user would change that into decimal numbers (which would mean a
        > > precision below the mpt!). So I also modified the AreaTreeParser. I don't think
        > > this may create any problem?
        >
        > I don't think so. I'll extract your changes from your patch and apply that
        > separately. That should make it easier for Peter, too.

        I've just committed a change to also replace Rectangle2D with Rectangle in
        PDFFactory and PDFPage. Forgot to do that in my patch.

        > > Also, a few questions:
        > <snip/>
        > > > fox:crop-offset: <length>

        {1,4}

        > > > Default: 0pt
        > > > Same behaviour as with fox:bleed.
        > > > The MediaBox is calculated by expanding the BleedBox by the crop offsets.
        > >
        > > Just to be sure: values will be allowed to be negative, right?
        >
        > Negative values don't make much sense, if I didn't miss anything.

        Actually not, because the simple-page-master's width and height would define
        the TrimBox. What I had in mind is to make the BleedBox coincide with the
        simple-page-master's boundaries, and define a TrimBox inside it. I was thinking
        that sometimes the document may be easier to design this way (especially when
        you want areas with dark backgrounds to bleed off the boundary of the final
        page). I guess it's not actually the case.

        <snip/>

        Vincent

        Show
        Vincent Hennebert added a comment - (In reply to comment #26) > Hi Vincent, > > (In reply to comment #22) <snip/> > > A note about the patch: it seemed more sensible to me to deal with integer > > values rather than double. Indeed every length value is internally converted > > into a whole number expressed in mpt (AFAICT). That means that I'm using > > Rectangle instead of Rectangle2D, and I changed some parameters into some > > classes accordingly (mainly, o.a.f.layoutmgr.Page.java, > > o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads > > the value of the "bounds" attribute as double instead of integer. AFAIK, the > > XML area tree is produced with whole numbers for the page boundaries; I don't > > see why a user would change that into decimal numbers (which would mean a > > precision below the mpt!). So I also modified the AreaTreeParser. I don't think > > this may create any problem? > > I don't think so. I'll extract your changes from your patch and apply that > separately. That should make it easier for Peter, too. I've just committed a change to also replace Rectangle2D with Rectangle in PDFFactory and PDFPage. Forgot to do that in my patch. > > Also, a few questions: > <snip/> > > > fox:crop-offset: <length> {1,4} > > > Default: 0pt > > > Same behaviour as with fox:bleed. > > > The MediaBox is calculated by expanding the BleedBox by the crop offsets. > > > > Just to be sure: values will be allowed to be negative, right? > > Negative values don't make much sense, if I didn't miss anything. Actually not, because the simple-page-master's width and height would define the TrimBox. What I had in mind is to make the BleedBox coincide with the simple-page-master's boundaries, and define a TrimBox inside it. I was thinking that sometimes the document may be easier to design this way (especially when you want areas with dark backgrounds to bleed off the boundary of the final page). I guess it's not actually the case. <snip/> Vincent
        Hide
        Peter Coppens added a comment -

        > > >
        > > > fox:crop-box: (trim-box|bleed-box|media-box)
        > > > Default: media-box
        > > > The crop box controls how Acrobat display the page or how the Java2DRenderer
        > > > sizes the output media. The PDF spec defines that the CropBox defaults to the
        > > > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box
        > > > extension which could take three "magic" values: "trim-box", "bleed-box" and
        > > > "media-box" to set the CropBox to one of those three other boxes. That should
        > > > cover 95% of all use cases. If anyone needs more control, that could easily
        > > > added later.
        > >
        > > I'm really not sure about that one. Calling it 'crop-box' is likely to create
        > > confusion with the 'crop-offset' property above. Apparently the CropBox is not
        > > used in prepress at all, so I would suggest to forget about that for the
        > > moment. That is, make the CropBox match with the MediaBox.
        >
        > Yes, I thought about that naming closeness, too. Not sure if that's so easy to
        > resolve. If Peter is OK with this, I guess the fox:crop-box could be left out
        > for the moment. I personally don't need it although at some point, such a
        > property might be useful to some.
        >
        > <snip/>

        Hmmm...not entirely happy with this unfortunately. We have a webapp that using a flex wysiwyg editor allows creation of print materials but that uses adobe's pdf browser plugin for end-user proofing and approval. In some cases we'd rather display the trimbox iso the mediabox so my guess is we should be able to control the CropBox setting, no?

        Perhaps a name change for the attribute is sufficient? E.g. something like crop-box-selector, or crop-box-source or something similar?

        Show
        Peter Coppens added a comment - > > > > > > fox:crop-box: (trim-box|bleed-box|media-box) > > > Default: media-box > > > The crop box controls how Acrobat display the page or how the Java2DRenderer > > > sizes the output media. The PDF spec defines that the CropBox defaults to the > > > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box > > > extension which could take three "magic" values: "trim-box", "bleed-box" and > > > "media-box" to set the CropBox to one of those three other boxes. That should > > > cover 95% of all use cases. If anyone needs more control, that could easily > > > added later. > > > > I'm really not sure about that one. Calling it 'crop-box' is likely to create > > confusion with the 'crop-offset' property above. Apparently the CropBox is not > > used in prepress at all, so I would suggest to forget about that for the > > moment. That is, make the CropBox match with the MediaBox. > > Yes, I thought about that naming closeness, too. Not sure if that's so easy to > resolve. If Peter is OK with this, I guess the fox:crop-box could be left out > for the moment. I personally don't need it although at some point, such a > property might be useful to some. > > <snip/> Hmmm...not entirely happy with this unfortunately. We have a webapp that using a flex wysiwyg editor allows creation of print materials but that uses adobe's pdf browser plugin for end-user proofing and approval. In some cases we'd rather display the trimbox iso the mediabox so my guess is we should be able to control the CropBox setting, no? Perhaps a name change for the attribute is sufficient? E.g. something like crop-box-selector, or crop-box-source or something similar?
        Hide
        Jeremias Maerki added a comment -

        Partially applied Vincent's patch (from today). I've also applied the undisputed parts of Peter's patch. So the next patch by Peter will only need to be about the extensions and their integration into the renderers. I hope the result will merge as painlessly as possible.
        http://svn.apache.org/viewvc?rev=798511&view=rev

        Show
        Jeremias Maerki added a comment - Partially applied Vincent's patch (from today). I've also applied the undisputed parts of Peter's patch. So the next patch by Peter will only need to be about the extensions and their integration into the renderers. I hope the result will merge as painlessly as possible. http://svn.apache.org/viewvc?rev=798511&view=rev
        Hide
        Jeremias Maerki added a comment -

        Hi Vincent,

        (In reply to comment #22)
        > I can't afford to spend much time on this patch so I'm happy to hand over the
        > job to you. I'm attaching a new patch with my own modifications as I had
        > started to prepare it for commit. Not sure it's going to be any useful given
        > the different approach you'd like to take, but who knows.

        I'm glad to take over.

        > A note about the patch: it seemed more sensible to me to deal with integer
        > values rather than double. Indeed every length value is internally converted
        > into a whole number expressed in mpt (AFAICT). That means that I'm using
        > Rectangle instead of Rectangle2D, and I changed some parameters into some
        > classes accordingly (mainly, o.a.f.layoutmgr.Page.java,
        > o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads
        > the value of the "bounds" attribute as double instead of integer. AFAIK, the
        > XML area tree is produced with whole numbers for the page boundaries; I don't
        > see why a user would change that into decimal numbers (which would mean a
        > precision below the mpt!). So I also modified the AreaTreeParser. I don't think
        > this may create any problem?

        I don't think so. I'll extract your changes from your patch and apply that separately. That should make it easier for Peter, too.

        > Also, a few questions:
        <snip/>
        > > fox:crop-offset: <length>

        {1,4}

        > > Default: 0pt
        > > Same behaviour as with fox:bleed.
        > > The MediaBox is calculated by expanding the BleedBox by the crop offsets.
        >
        > Just to be sure: values will be allowed to be negative, right?

        Negative values don't make much sense, if I didn't miss anything.

        > It would make more sense to me to set the default value of crop-offset to the
        > value of bleed.

        Right.

        > > BTW, the naming above pretty much matches other FO implementations, so should
        > > we ever have a standard for these properties (like by reviving exslfo.sf.net),
        > > it's likely we probably don't have to change much besides the namespace prefix.
        > >
        > > fox:crop-box: (trim-box|bleed-box|media-box)
        > > Default: media-box
        > > The crop box controls how Acrobat display the page or how the Java2DRenderer
        > > sizes the output media. The PDF spec defines that the CropBox defaults to the
        > > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box
        > > extension which could take three "magic" values: "trim-box", "bleed-box" and
        > > "media-box" to set the CropBox to one of those three other boxes. That should
        > > cover 95% of all use cases. If anyone needs more control, that could easily
        > > added later.
        >
        > I'm really not sure about that one. Calling it 'crop-box' is likely to create
        > confusion with the 'crop-offset' property above. Apparently the CropBox is not
        > used in prepress at all, so I would suggest to forget about that for the
        > moment. That is, make the CropBox match with the MediaBox.

        Yes, I thought about that naming closeness, too. Not sure if that's so easy to resolve. If Peter is OK with this, I guess the fox:crop-box could be left out for the moment. I personally don't need it although at some point, such a property might be useful to some.

        <snip/>

        Show
        Jeremias Maerki added a comment - Hi Vincent, (In reply to comment #22) > I can't afford to spend much time on this patch so I'm happy to hand over the > job to you. I'm attaching a new patch with my own modifications as I had > started to prepare it for commit. Not sure it's going to be any useful given > the different approach you'd like to take, but who knows. I'm glad to take over. > A note about the patch: it seemed more sensible to me to deal with integer > values rather than double. Indeed every length value is internally converted > into a whole number expressed in mpt (AFAICT). That means that I'm using > Rectangle instead of Rectangle2D, and I changed some parameters into some > classes accordingly (mainly, o.a.f.layoutmgr.Page.java, > o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads > the value of the "bounds" attribute as double instead of integer. AFAIK, the > XML area tree is produced with whole numbers for the page boundaries; I don't > see why a user would change that into decimal numbers (which would mean a > precision below the mpt!). So I also modified the AreaTreeParser. I don't think > this may create any problem? I don't think so. I'll extract your changes from your patch and apply that separately. That should make it easier for Peter, too. > Also, a few questions: <snip/> > > fox:crop-offset: <length> {1,4} > > Default: 0pt > > Same behaviour as with fox:bleed. > > The MediaBox is calculated by expanding the BleedBox by the crop offsets. > > Just to be sure: values will be allowed to be negative, right? Negative values don't make much sense, if I didn't miss anything. > It would make more sense to me to set the default value of crop-offset to the > value of bleed. Right. > > BTW, the naming above pretty much matches other FO implementations, so should > > we ever have a standard for these properties (like by reviving exslfo.sf.net), > > it's likely we probably don't have to change much besides the namespace prefix. > > > > fox:crop-box: (trim-box|bleed-box|media-box) > > Default: media-box > > The crop box controls how Acrobat display the page or how the Java2DRenderer > > sizes the output media. The PDF spec defines that the CropBox defaults to the > > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box > > extension which could take three "magic" values: "trim-box", "bleed-box" and > > "media-box" to set the CropBox to one of those three other boxes. That should > > cover 95% of all use cases. If anyone needs more control, that could easily > > added later. > > I'm really not sure about that one. Calling it 'crop-box' is likely to create > confusion with the 'crop-offset' property above. Apparently the CropBox is not > used in prepress at all, so I would suggest to forget about that for the > moment. That is, make the CropBox match with the MediaBox. Yes, I thought about that naming closeness, too. Not sure if that's so easy to resolve. If Peter is OK with this, I guess the fox:crop-box could be left out for the moment. I personally don't need it although at some point, such a property might be useful to some. <snip/>
        Hide
        Jeremias Maerki added a comment -

        Peter, I'll respond to Vincent's comments shortly. I think his changes shouldn't affect your changes too much. I think Vincent's comments make sense and I'll see to it that they can be committed separately (they don't really have much to do with the issue at hand). Then it will ideally just be a "svn up" on your side to resolve this. And a minor SVN conflict in the worst case. I suggest you just continue as intended.

        Show
        Jeremias Maerki added a comment - Peter, I'll respond to Vincent's comments shortly. I think his changes shouldn't affect your changes too much. I think Vincent's comments make sense and I'll see to it that they can be committed separately (they don't really have much to do with the issue at hand). Then it will ideally just be a "svn up" on your side to resolve this. And a minor SVN conflict in the worst case. I suggest you just continue as intended.
        Hide
        Peter Coppens added a comment -

        We were just about to start to rework the patch according to the suggestions made by Jeremias which seem to make a lot of sense, but I am not so sure anymore that is still useful given Vincent's alternative implementation/efforts which we were unaware of.

        It's a bit unclear right now how we can continue to contribute given the different implementation efforts

        We still have a bit of bandwith to spare on this (it is important for us) but we'd rather make sure those efforts are part of a consolidated action

        Can someone advice?

        Thanks

        Peter

        Show
        Peter Coppens added a comment - We were just about to start to rework the patch according to the suggestions made by Jeremias which seem to make a lot of sense, but I am not so sure anymore that is still useful given Vincent's alternative implementation/efforts which we were unaware of. It's a bit unclear right now how we can continue to contribute given the different implementation efforts We still have a bit of bandwith to spare on this (it is important for us) but we'd rather make sure those efforts are part of a consolidated action Can someone advice? Thanks Peter
        Hide
        Vincent Hennebert added a comment -

        Attachment prepress.diff has been added with description: Updated patch with some clean-up and modifications

        Show
        Vincent Hennebert added a comment - Attachment prepress.diff has been added with description: Updated patch with some clean-up and modifications
        Hide
        Vincent Hennebert added a comment -

        Hi Jeremias,

        I can't afford to spend much time on this patch so I'm happy to hand over the job to you. I'm attaching a new patch with my own modifications as I had started to prepare it for commit. Not sure it's going to be any useful given the different approach you'd like to take, but who knows.

        A note about the patch: it seemed more sensible to me to deal with integer values rather than double. Indeed every length value is internally converted into a whole number expressed in mpt (AFAICT). That means that I'm using Rectangle instead of Rectangle2D, and I changed some parameters into some classes accordingly (mainly, o.a.f.layoutmgr.Page.java, o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads the value of the "bounds" attribute as double instead of integer. AFAIK, the XML area tree is produced with whole numbers for the page boundaries; I don't see why a user would change that into decimal numbers (which would mean a precision below the mpt!). So I also modified the AreaTreeParser. I don't think this may create any problem?

        Also, a few questions:

        (In reply to comment #21)
        <snip/>
        >
        > The simple-page-master's width and height properties shall define the TrimBox.
        > If there is no bleed and crop mark area, the MediaBox will be equal to the
        > TrimBox, or rather just the MediaBox is generated in this case, like it happens
        > today.
        >
        > fox:bleed: <length>

        {1,4}
        > Default: 0pt
        > If there is only one value, it applies to all sides. If there are two values,
        > the top and bottom bleed widths are set to the first value and the right and
        > left bleed widths are set to the second. If there are three values, the top is
        > set to the first value, the left and right are set to the second, and the
        > bottom is set to the third. If there are four values, they apply to the top,
        > right, bottom, and left, respectively. (Corresponds to
        > http://www.w3.org/TR/xsl11/#padding)
        > The BleedBox is calculated by expanding the TrimBox by the bleed widths.
        > (I'd prefer to call the property fox:bleed rather than fox:bleed-box as we
        > don't set the BleedBox values directly. We specify the bleed amount.)
        >
        > fox:crop-offset: <length>{1,4}

        > Default: 0pt
        > Same behaviour as with fox:bleed.
        > The MediaBox is calculated by expanding the BleedBox by the crop offsets.

        Just to be sure: values will be allowed to be negative, right?

        It would make more sense to me to set the default value of crop-offset to the value of bleed.

        > BTW, the naming above pretty much matches other FO implementations, so should
        > we ever have a standard for these properties (like by reviving exslfo.sf.net),
        > it's likely we probably don't have to change much besides the namespace prefix.
        >
        > fox:crop-box: (trim-box|bleed-box|media-box)
        > Default: media-box
        > The crop box controls how Acrobat display the page or how the Java2DRenderer
        > sizes the output media. The PDF spec defines that the CropBox defaults to the
        > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box
        > extension which could take three "magic" values: "trim-box", "bleed-box" and
        > "media-box" to set the CropBox to one of those three other boxes. That should
        > cover 95% of all use cases. If anyone needs more control, that could easily
        > added later.

        I'm really not sure about that one. Calling it 'crop-box' is likely to create confusion with the 'crop-offset' property above. Apparently the CropBox is not used in prepress at all, so I would suggest to forget about that for the moment. That is, make the CropBox match with the MediaBox.

        > I don't have much feedback on fox:scale. I guess it can be useful but I don't
        > see a big use case for an extension. I'd rather want to control that from
        > application code. But it shouldn't get in the way of anything so I have not
        > problem with it.
        >
        > WDYT?

        Vincent

        Show
        Vincent Hennebert added a comment - Hi Jeremias, I can't afford to spend much time on this patch so I'm happy to hand over the job to you. I'm attaching a new patch with my own modifications as I had started to prepare it for commit. Not sure it's going to be any useful given the different approach you'd like to take, but who knows. A note about the patch: it seemed more sensible to me to deal with integer values rather than double. Indeed every length value is internally converted into a whole number expressed in mpt (AFAICT). That means that I'm using Rectangle instead of Rectangle2D, and I changed some parameters into some classes accordingly (mainly, o.a.f.layoutmgr.Page.java, o.a.f.area.PageViewport.java). But then I noticed that the AreaTreeParser reads the value of the "bounds" attribute as double instead of integer. AFAIK, the XML area tree is produced with whole numbers for the page boundaries; I don't see why a user would change that into decimal numbers (which would mean a precision below the mpt!). So I also modified the AreaTreeParser. I don't think this may create any problem? Also, a few questions: (In reply to comment #21) <snip/> > > The simple-page-master's width and height properties shall define the TrimBox. > If there is no bleed and crop mark area, the MediaBox will be equal to the > TrimBox, or rather just the MediaBox is generated in this case, like it happens > today. > > fox:bleed: <length> {1,4} > Default: 0pt > If there is only one value, it applies to all sides. If there are two values, > the top and bottom bleed widths are set to the first value and the right and > left bleed widths are set to the second. If there are three values, the top is > set to the first value, the left and right are set to the second, and the > bottom is set to the third. If there are four values, they apply to the top, > right, bottom, and left, respectively. (Corresponds to > http://www.w3.org/TR/xsl11/#padding ) > The BleedBox is calculated by expanding the TrimBox by the bleed widths. > (I'd prefer to call the property fox:bleed rather than fox:bleed-box as we > don't set the BleedBox values directly. We specify the bleed amount.) > > fox:crop-offset: <length>{1,4} > Default: 0pt > Same behaviour as with fox:bleed. > The MediaBox is calculated by expanding the BleedBox by the crop offsets. Just to be sure: values will be allowed to be negative, right? It would make more sense to me to set the default value of crop-offset to the value of bleed. > BTW, the naming above pretty much matches other FO implementations, so should > we ever have a standard for these properties (like by reviving exslfo.sf.net), > it's likely we probably don't have to change much besides the namespace prefix. > > fox:crop-box: (trim-box|bleed-box|media-box) > Default: media-box > The crop box controls how Acrobat display the page or how the Java2DRenderer > sizes the output media. The PDF spec defines that the CropBox defaults to the > MediaBox, so it makes sense to do the same here. We could define a fox:crop-box > extension which could take three "magic" values: "trim-box", "bleed-box" and > "media-box" to set the CropBox to one of those three other boxes. That should > cover 95% of all use cases. If anyone needs more control, that could easily > added later. I'm really not sure about that one. Calling it 'crop-box' is likely to create confusion with the 'crop-offset' property above. Apparently the CropBox is not used in prepress at all, so I would suggest to forget about that for the moment. That is, make the CropBox match with the MediaBox. > I don't have much feedback on fox:scale. I guess it can be useful but I don't > see a big use case for an extension. I'd rather want to control that from > application code. But it shouldn't get in the way of anything so I have not > problem with it. > > WDYT? Vincent
        Hide
        Jeremias Maerki added a comment -

        Hi Peter,
        incidentally, I need this functionality myself in a project I'm currently working on. I've locally applied your patch to play with it. I apologize for the late feedback.

        I notice you chose the page size of the simple-page-master as the baseline for the MediaBox. I would have expected the SPM's size to define the TrimBox instead. Bleed and cut marks would then lie outside the actual logical page. I've checked what other FO implementations do and they seem to follow that pattern rather than your approach.

        I'm also finding the specification of the areas a bit counter-intuitive. A simple value only sets the left side, rather than the value for all four sides as with other FO properties. I guess that also points to Andreas' comment about reusing property infrastructure where this is already handled. Granted, it is not easy to use. I'm not even sure myself if this can easily be reused with some serious refactoring. At any rate, a print shop will usually just give you the information that you should use 2 or 3 mm for the bleed area. Just specifying one simple value is quite handy.

        Rather than just criticizing, I'm willing to invest some time to help with this. I would like to make a counter-proposal for the extensions:

        The simple-page-master's width and height properties shall define the TrimBox. If there is no bleed and crop mark area, the MediaBox will be equal to the TrimBox, or rather just the MediaBox is generated in this case, like it happens today.

        fox:bleed: <length>

        {1,4}
        Default: 0pt
        If there is only one value, it applies to all sides. If there are two values, the top and bottom bleed widths are set to the first value and the right and left bleed widths are set to the second. If there are three values, the top is set to the first value, the left and right are set to the second, and the bottom is set to the third. If there are four values, they apply to the top, right, bottom, and left, respectively. (Corresponds to http://www.w3.org/TR/xsl11/#padding)
        The BleedBox is calculated by expanding the TrimBox by the bleed widths.
        (I'd prefer to call the property fox:bleed rather than fox:bleed-box as we don't set the BleedBox values directly. We specify the bleed amount.)

        fox:crop-offset: <length>{1,4}

        Default: 0pt
        Same behaviour as with fox:bleed.
        The MediaBox is calculated by expanding the BleedBox by the crop offsets.

        BTW, the naming above pretty much matches other FO implementations, so should we ever have a standard for these properties (like by reviving exslfo.sf.net), it's likely we probably don't have to change much besides the namespace prefix.

        fox:crop-box: (trim-box|bleed-box|media-box)
        Default: media-box
        The crop box controls how Acrobat display the page or how the Java2DRenderer sizes the output media. The PDF spec defines that the CropBox defaults to the MediaBox, so it makes sense to do the same here. We could define a fox:crop-box extension which could take three "magic" values: "trim-box", "bleed-box" and "media-box" to set the CropBox to one of those three other boxes. That should cover 95% of all use cases. If anyone needs more control, that could easily added later.

        I don't have much feedback on fox:scale. I guess it can be useful but I don't see a big use case for an extension. I'd rather want to control that from application code. But it shouldn't get in the way of anything so I have not problem with it.

        WDYT?

        Show
        Jeremias Maerki added a comment - Hi Peter, incidentally, I need this functionality myself in a project I'm currently working on. I've locally applied your patch to play with it. I apologize for the late feedback. I notice you chose the page size of the simple-page-master as the baseline for the MediaBox. I would have expected the SPM's size to define the TrimBox instead. Bleed and cut marks would then lie outside the actual logical page. I've checked what other FO implementations do and they seem to follow that pattern rather than your approach. I'm also finding the specification of the areas a bit counter-intuitive. A simple value only sets the left side, rather than the value for all four sides as with other FO properties. I guess that also points to Andreas' comment about reusing property infrastructure where this is already handled. Granted, it is not easy to use. I'm not even sure myself if this can easily be reused with some serious refactoring. At any rate, a print shop will usually just give you the information that you should use 2 or 3 mm for the bleed area. Just specifying one simple value is quite handy. Rather than just criticizing, I'm willing to invest some time to help with this. I would like to make a counter-proposal for the extensions: The simple-page-master's width and height properties shall define the TrimBox. If there is no bleed and crop mark area, the MediaBox will be equal to the TrimBox, or rather just the MediaBox is generated in this case, like it happens today. fox:bleed: <length> {1,4} Default: 0pt If there is only one value, it applies to all sides. If there are two values, the top and bottom bleed widths are set to the first value and the right and left bleed widths are set to the second. If there are three values, the top is set to the first value, the left and right are set to the second, and the bottom is set to the third. If there are four values, they apply to the top, right, bottom, and left, respectively. (Corresponds to http://www.w3.org/TR/xsl11/#padding ) The BleedBox is calculated by expanding the TrimBox by the bleed widths. (I'd prefer to call the property fox:bleed rather than fox:bleed-box as we don't set the BleedBox values directly. We specify the bleed amount.) fox:crop-offset: <length>{1,4} Default: 0pt Same behaviour as with fox:bleed. The MediaBox is calculated by expanding the BleedBox by the crop offsets. BTW, the naming above pretty much matches other FO implementations, so should we ever have a standard for these properties (like by reviving exslfo.sf.net), it's likely we probably don't have to change much besides the namespace prefix. fox:crop-box: (trim-box|bleed-box|media-box) Default: media-box The crop box controls how Acrobat display the page or how the Java2DRenderer sizes the output media. The PDF spec defines that the CropBox defaults to the MediaBox, so it makes sense to do the same here. We could define a fox:crop-box extension which could take three "magic" values: "trim-box", "bleed-box" and "media-box" to set the CropBox to one of those three other boxes. That should cover 95% of all use cases. If anyone needs more control, that could easily added later. I don't have much feedback on fox:scale. I guess it can be useful but I don't see a big use case for an extension. I'd rather want to control that from application code. But it shouldn't get in the way of anything so I have not problem with it. WDYT?
        Hide
        Peter Coppens added a comment -

        Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v7.patch has been added with description: Box implementation for Java2d now uses "pdf box settings"

        Show
        Peter Coppens added a comment - Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v7.patch has been added with description: Box implementation for Java2d now uses "pdf box settings"
        Hide
        Peter Coppens added a comment -

        The attached patch fixes the previous comment

        "There is an inconsistency between PDF and Java2D regarding the coordinates of
        the boxes: in PDF the x and y coordinates are relative to the left and bottom
        sides, in Java2D they are relative to the left and /top/ sides. One of the two
        possibilities will have to be chosen, probably the PDF way."

        The boxes for the Java2D renderer now also follows the "PDF way"

        Show
        Peter Coppens added a comment - The attached patch fixes the previous comment "There is an inconsistency between PDF and Java2D regarding the coordinates of the boxes: in PDF the x and y coordinates are relative to the left and bottom sides, in Java2D they are relative to the left and /top/ sides. One of the two possibilities will have to be chosen, probably the PDF way." The boxes for the Java2D renderer now also follows the "PDF way"
        Hide
        Vincent Hennebert added a comment -

        Hi,

        (In reply to comment #18)
        > >>I fixed that by multiplying the scales by scaleFactor instead of
        > >>overwriting that latter:
        > Yeah..that seems correct.

        In the meantime, I found out why the Swing renderer doesn't take the scale extension into account: the AWTRenderer class (which should have been named SwingRenderer really) defines its own getPageImageSize method instead of re-using the stuff from Java2DRenderer.getPageImage. I'll see if I find the energy to fix that.

        > > I have doubts about that scale extension, I must say. It seems very ad-hoc to
        > > me. Can't that be left to some post-processing mechanism? For PDF output this
        > > usually is a job that is handled by the printer. For PNG output I'm sure that
        > > there are plenty of programs that can do that very well (actually I had a
        > > better quality result when re-scaling the PNG output with an external program
        > > than by using the new extension —might be a problem with the Java2D renderer
        > > though).
        > >
        > Obviously scaling can be handled through a post processing step, just like
        > adding the pdf boxes can be handled using e.g. PDFBox after fop has rendered
        > the stylesheet to pdf. This is what we currently use. But it is very inelegant
        > as we now need to also store 'template/stylesheet' information outside the
        > stylesheet, dispatch postprocessing based on output type, and it also adds
        > extra processing overhead where, with the integrated approach, almost no extra
        > overhead is needed. Once confronted with things like 'adverts' where page size
        > options are very restricted by publishers it does seem to make sense to
        > integrate it all together, at least from a 'users' perspective. Whether it
        > makes sense for fo(p), I feel not very well placed to comment (at lease the box
        > requirement has been requested before)

        Note that I don't question the box extensions, which are indeed useful and have already been requested in the past. Only the scale extension was looking very specific to me.

        > > Also, is there a use case for a non-proportional scale (x scale != y scale)?
        > > Not that having different x and y factors makes the whole thing a lot more
        > > complicated, but...
        > >
        > Publishers do restrict aspect ratio's. It does not make sense, layout wise, to
        > do 'big' non-proportional scalings, but small factors allow to reuse the same
        > stylesheet page content, for different 'publishers' and that does make the
        > amount of maintenance a lot more manageable.

        Ok. Makes sense.

        There is an inconsistency between PDF and Java2D regarding the coordinates of the boxes: in PDF the x and y coordinates are relative to the left and bottom sides, in Java2D they are relative to the left and /top/ sides. One of the two possibilities will have to be chosen, probably the PDF way.

        Thanks,
        Vincent

        Show
        Vincent Hennebert added a comment - Hi, (In reply to comment #18) > >>I fixed that by multiplying the scales by scaleFactor instead of > >>overwriting that latter: > Yeah..that seems correct. In the meantime, I found out why the Swing renderer doesn't take the scale extension into account: the AWTRenderer class (which should have been named SwingRenderer really) defines its own getPageImageSize method instead of re-using the stuff from Java2DRenderer.getPageImage. I'll see if I find the energy to fix that. > > I have doubts about that scale extension, I must say. It seems very ad-hoc to > > me. Can't that be left to some post-processing mechanism? For PDF output this > > usually is a job that is handled by the printer. For PNG output I'm sure that > > there are plenty of programs that can do that very well (actually I had a > > better quality result when re-scaling the PNG output with an external program > > than by using the new extension —might be a problem with the Java2D renderer > > though). > > > Obviously scaling can be handled through a post processing step, just like > adding the pdf boxes can be handled using e.g. PDFBox after fop has rendered > the stylesheet to pdf. This is what we currently use. But it is very inelegant > as we now need to also store 'template/stylesheet' information outside the > stylesheet, dispatch postprocessing based on output type, and it also adds > extra processing overhead where, with the integrated approach, almost no extra > overhead is needed. Once confronted with things like 'adverts' where page size > options are very restricted by publishers it does seem to make sense to > integrate it all together, at least from a 'users' perspective. Whether it > makes sense for fo(p), I feel not very well placed to comment (at lease the box > requirement has been requested before) Note that I don't question the box extensions, which are indeed useful and have already been requested in the past. Only the scale extension was looking very specific to me. > > Also, is there a use case for a non-proportional scale (x scale != y scale)? > > Not that having different x and y factors makes the whole thing a lot more > > complicated, but... > > > Publishers do restrict aspect ratio's. It does not make sense, layout wise, to > do 'big' non-proportional scalings, but small factors allow to reuse the same > stylesheet page content, for different 'publishers' and that does make the > amount of maintenance a lot more manageable. Ok. Makes sense. There is an inconsistency between PDF and Java2D regarding the coordinates of the boxes: in PDF the x and y coordinates are relative to the left and bottom sides, in Java2D they are relative to the left and /top/ sides. One of the two possibilities will have to be chosen, probably the PDF way. Thanks, Vincent
        Hide
        Peter Coppens added a comment -

        >>I fixed that by multiplying the scales by scaleFactor instead of
        >>overwriting that latter:
        Yeah..that seems correct.

        > I have doubts about that scale extension, I must say. It seems very ad-hoc to
        > me. Can't that be left to some post-processing mechanism? For PDF output this
        > usually is a job that is handled by the printer. For PNG output I'm sure that
        > there are plenty of programs that can do that very well (actually I had a
        > better quality result when re-scaling the PNG output with an external program
        > than by using the new extension —might be a problem with the Java2D renderer
        > though).
        >
        Obviously scaling can be handled through a post processing step, just like adding the pdf boxes can be handled using e.g. PDFBox after fop has rendered the stylesheet to pdf. This is what we currently use. But it is very inelegant as we now need to also store 'template/stylesheet' information outside the stylesheet, dispatch postprocessing based on output type, and it also adds extra processing overhead where, with the integrated approach, almost no extra overhead is needed. Once confronted with things like 'adverts' where page size options are very restricted by publishers it does seem to make sense to integrate it all together, at least from a 'users' perspective. Whether it makes sense for fo(p), I feel not very well placed to comment (at lease the box requirement has been requested before)

        >
        > Also, is there a use case for a non-proportional scale (x scale != y scale)?
        > Not that having different x and y factors makes the whole thing a lot more
        > complicated, but...
        >
        Publishers do restrict aspect ratio's. It does not make sense, layout wise, to do 'big' non-proportional scalings, but small factors allow to reuse the same stylesheet page content, for different 'publishers' and that does make the amount of maintenance a lot more manageable.

        Thanks

        Peter

        Show
        Peter Coppens added a comment - >>I fixed that by multiplying the scales by scaleFactor instead of >>overwriting that latter: Yeah..that seems correct. > I have doubts about that scale extension, I must say. It seems very ad-hoc to > me. Can't that be left to some post-processing mechanism? For PDF output this > usually is a job that is handled by the printer. For PNG output I'm sure that > there are plenty of programs that can do that very well (actually I had a > better quality result when re-scaling the PNG output with an external program > than by using the new extension —might be a problem with the Java2D renderer > though). > Obviously scaling can be handled through a post processing step, just like adding the pdf boxes can be handled using e.g. PDFBox after fop has rendered the stylesheet to pdf. This is what we currently use. But it is very inelegant as we now need to also store 'template/stylesheet' information outside the stylesheet, dispatch postprocessing based on output type, and it also adds extra processing overhead where, with the integrated approach, almost no extra overhead is needed. Once confronted with things like 'adverts' where page size options are very restricted by publishers it does seem to make sense to integrate it all together, at least from a 'users' perspective. Whether it makes sense for fo(p), I feel not very well placed to comment (at lease the box requirement has been requested before) > > Also, is there a use case for a non-proportional scale (x scale != y scale)? > Not that having different x and y factors makes the whole thing a lot more > complicated, but... > Publishers do restrict aspect ratio's. It does not make sense, layout wise, to do 'big' non-proportional scalings, but small factors allow to reuse the same stylesheet page content, for different 'publishers' and that does make the amount of maintenance a lot more manageable. Thanks Peter
        Hide
        Peter Coppens added a comment -

        Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_AWTRenderer_v1.patch has been added with description: AWTRenderer change to take into account fop:scale setting

        Show
        Peter Coppens added a comment - Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_AWTRenderer_v1.patch has been added with description: AWTRenderer change to take into account fop:scale setting
        Hide
        Vincent Hennebert added a comment -

        There seems to be another problem: in the Swing preview the zoom no longer works. I fixed that by multiplying the scales by scaleFactor instead of overwriting that latter:
        if (scales != null)

        { scaleX *= scales.getX(); scaleY *= scales.getY(); }

        but then scrolling bars appear the same way as if fox:scale had not been specified. When resizing the window they appear whereas there is obviously still room for the whole document to fit in.

        I have doubts about that scale extension, I must say. It seems very ad-hoc to me. Can't that be left to some post-processing mechanism? For PDF output this usually is a job that is handled by the printer. For PNG output I'm sure that there are plenty of programs that can do that very well (actually I had a better quality result when re-scaling the PNG output with an external program than by using the new extension —might be a problem with the Java2D renderer though).

        Also, is there a use case for a non-proportional scale (x scale != y scale)? Not that having different x and y factors makes the whole thing a lot more complicated, but...

        Thanks,
        Vincent

        (In reply to comment #13)
        > (In reply to comment #8)
        > > Created an attachment (id=23941) [details] [details]
        > > updated patch in an attempt to address fop-dev suggestions
        >
        > Thanks for the updated patch. I tried to run the AWT renderer and it appears to
        > have been broken by the changes. The main window is opened but no document is
        > displayed, and I get the following exception:
        > Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x
        > + width) is outside raster
        > at
        > sun.awt.image.IntegerInterleavedRaster.createWritableChild(IntegerInterleavedRaster.java:450)
        > at java.awt.image.BufferedImage.getSubimage(BufferedImage.java:1156)
        > at
        > org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:379)
        > at
        > org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:425)
        > at
        > org.apache.fop.render.awt.viewer.ImageProxyPanel.paintComponent(ImageProxyPanel.java:124)
        > at javax.swing.JComponent.paint(JComponent.java:1027)
        > at javax.swing.JComponent.paintChildren(JComponent.java:864)
        > at javax.swing.JComponent.paint(JComponent.java:1036)
        > at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122)
        > at
        > javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277)
        > at javax.swing.RepaintManager.paint(RepaintManager.java:1217)
        > at javax.swing.JComponent._paintImmediately(JComponent.java:5070)
        > at javax.swing.JComponent.paintImmediately(JComponent.java:4880)
        > at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803)
        > at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714)
        > at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694)
        > at
        > javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128)
        > at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
        > at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
        > at
        > java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
        > at
        > java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
        > at
        > java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
        > at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
        > at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
        > at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)
        >
        > I'll investigate (but: do you really need this feature also for the AWT
        > renderer? )
        >
        > Thanks,
        > Vincent

        Show
        Vincent Hennebert added a comment - There seems to be another problem: in the Swing preview the zoom no longer works. I fixed that by multiplying the scales by scaleFactor instead of overwriting that latter: if (scales != null) { scaleX *= scales.getX(); scaleY *= scales.getY(); } but then scrolling bars appear the same way as if fox:scale had not been specified. When resizing the window they appear whereas there is obviously still room for the whole document to fit in. I have doubts about that scale extension, I must say. It seems very ad-hoc to me. Can't that be left to some post-processing mechanism? For PDF output this usually is a job that is handled by the printer. For PNG output I'm sure that there are plenty of programs that can do that very well (actually I had a better quality result when re-scaling the PNG output with an external program than by using the new extension —might be a problem with the Java2D renderer though). Also, is there a use case for a non-proportional scale (x scale != y scale)? Not that having different x and y factors makes the whole thing a lot more complicated, but... Thanks, Vincent (In reply to comment #13) > (In reply to comment #8) > > Created an attachment (id=23941) [details] [details] > > updated patch in an attempt to address fop-dev suggestions > > Thanks for the updated patch. I tried to run the AWT renderer and it appears to > have been broken by the changes. The main window is opened but no document is > displayed, and I get the following exception: > Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x > + width) is outside raster > at > sun.awt.image.IntegerInterleavedRaster.createWritableChild(IntegerInterleavedRaster.java:450) > at java.awt.image.BufferedImage.getSubimage(BufferedImage.java:1156) > at > org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:379) > at > org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:425) > at > org.apache.fop.render.awt.viewer.ImageProxyPanel.paintComponent(ImageProxyPanel.java:124) > at javax.swing.JComponent.paint(JComponent.java:1027) > at javax.swing.JComponent.paintChildren(JComponent.java:864) > at javax.swing.JComponent.paint(JComponent.java:1036) > at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122) > at > javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277) > at javax.swing.RepaintManager.paint(RepaintManager.java:1217) > at javax.swing.JComponent._paintImmediately(JComponent.java:5070) > at javax.swing.JComponent.paintImmediately(JComponent.java:4880) > at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803) > at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714) > at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694) > at > javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128) > at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209) > at java.awt.EventQueue.dispatchEvent(EventQueue.java:597) > at > java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269) > at > java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184) > at > java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174) > at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169) > at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161) > at java.awt.EventDispatchThread.run(EventDispatchThread.java:122) > > I'll investigate (but: do you really need this feature also for the AWT > renderer? ) > > Thanks, > Vincent
        Hide
        Peter Coppens added a comment -

        Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v6.patch has been added with description: Updated patch avoid awt issue

        Show
        Peter Coppens added a comment - Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v6.patch has been added with description: Updated patch avoid awt issue
        Hide
        Peter Coppens added a comment -

        Another attempt (hopefully) not breaking awt renderer

        Show
        Peter Coppens added a comment - Another attempt (hopefully) not breaking awt renderer
        Hide
        Peter Coppens added a comment -

        >
        > Thanks for the updated patch. I tried to run the AWT renderer and it appears to
        > have been broken by the changes. The main window is opened but no document is
        > displayed, and I get the following exception:
        > Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x
        > + width) is outside raster
        > at

        Oops...that is not so good

        >
        > I'll investigate (but: do you really need this feature also for the AWT
        > renderer? )
        >
        Actually 'only' for png output but both awt and png output probably originate from the Java2DRenderer ?

        Show
        Peter Coppens added a comment - > > Thanks for the updated patch. I tried to run the AWT renderer and it appears to > have been broken by the changes. The main window is opened but no document is > displayed, and I get the following exception: > Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x > + width) is outside raster > at Oops...that is not so good > > I'll investigate (but: do you really need this feature also for the AWT > renderer? ) > Actually 'only' for png output but both awt and png output probably originate from the Java2DRenderer ?
        Hide
        Vincent Hennebert added a comment -

        (In reply to comment #8)
        > Created an attachment (id=23941) [details]
        > updated patch in an attempt to address fop-dev suggestions

        Thanks for the updated patch. I tried to run the AWT renderer and it appears to have been broken by the changes. The main window is opened but no document is displayed, and I get the following exception:
        Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x + width) is outside raster
        at sun.awt.image.IntegerInterleavedRaster.createWritableChild(IntegerInterleavedRaster.java:450)
        at java.awt.image.BufferedImage.getSubimage(BufferedImage.java:1156)
        at org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:379)
        at org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:425)
        at org.apache.fop.render.awt.viewer.ImageProxyPanel.paintComponent(ImageProxyPanel.java:124)
        at javax.swing.JComponent.paint(JComponent.java:1027)
        at javax.swing.JComponent.paintChildren(JComponent.java:864)
        at javax.swing.JComponent.paint(JComponent.java:1036)
        at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122)
        at javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277)
        at javax.swing.RepaintManager.paint(RepaintManager.java:1217)
        at javax.swing.JComponent._paintImmediately(JComponent.java:5070)
        at javax.swing.JComponent.paintImmediately(JComponent.java:4880)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803)
        at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714)
        at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694)
        at javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128)
        at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209)
        at java.awt.EventQueue.dispatchEvent(EventQueue.java:597)
        at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269)
        at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184)
        at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169)
        at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161)
        at java.awt.EventDispatchThread.run(EventDispatchThread.java:122)

        I'll investigate (but: do you really need this feature also for the AWT renderer? )

        Thanks,
        Vincent

        Show
        Vincent Hennebert added a comment - (In reply to comment #8) > Created an attachment (id=23941) [details] > updated patch in an attempt to address fop-dev suggestions Thanks for the updated patch. I tried to run the AWT renderer and it appears to have been broken by the changes. The main window is opened but no document is displayed, and I get the following exception: Exception in thread "AWT-EventQueue-0" java.awt.image.RasterFormatException: (x + width) is outside raster at sun.awt.image.IntegerInterleavedRaster.createWritableChild(IntegerInterleavedRaster.java:450) at java.awt.image.BufferedImage.getSubimage(BufferedImage.java:1156) at org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:379) at org.apache.fop.render.java2d.Java2DRenderer.getPageImage(Java2DRenderer.java:425) at org.apache.fop.render.awt.viewer.ImageProxyPanel.paintComponent(ImageProxyPanel.java:124) at javax.swing.JComponent.paint(JComponent.java:1027) at javax.swing.JComponent.paintChildren(JComponent.java:864) at javax.swing.JComponent.paint(JComponent.java:1036) at javax.swing.JComponent.paintToOffscreen(JComponent.java:5122) at javax.swing.BufferStrategyPaintManager.paint(BufferStrategyPaintManager.java:277) at javax.swing.RepaintManager.paint(RepaintManager.java:1217) at javax.swing.JComponent._paintImmediately(JComponent.java:5070) at javax.swing.JComponent.paintImmediately(JComponent.java:4880) at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:803) at javax.swing.RepaintManager.paintDirtyRegions(RepaintManager.java:714) at javax.swing.RepaintManager.seqPaintDirtyRegions(RepaintManager.java:694) at javax.swing.SystemEventQueueUtilities$ComponentWorkRequest.run(SystemEventQueueUtilities.java:128) at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:209) at java.awt.EventQueue.dispatchEvent(EventQueue.java:597) at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:269) at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:184) at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:174) at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:169) at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:161) at java.awt.EventDispatchThread.run(EventDispatchThread.java:122) I'll investigate (but: do you really need this feature also for the AWT renderer? ) Thanks, Vincent
        Hide
        Peter Coppens added a comment -

        (In reply to comment #11)
        > Yeah, I saw that, but that still doesn't tell me what I'm looking for. What's
        > the use case for using these properties? What functionality do they offer
        > that's not in other properties in the specification? Is this for background
        > images or something? Watermarks?
        >
        > Based on Andreas's comments in this bug this sounds like a common feature
        > request, I've just never heard of it.
        >
        > I'm only curious because I use FOP pretty extensively and wonder if these
        > extensions add something I would find useful.

        Ah..ic

        The box'es set the pdf page boxes with the same name. I found http://www.prepressure.com/pdf/basics/page_boxes useful (not necessarily finding the pdf spec an easy read ). These boxes are often used to drive what is actually printed as compared to what e.g. acrobat displays

        Scale is something we needed to use fop for creating adverts. The size of the output depends on e.g. the magazine you want your advert to be published in. As we want to use same stylesheet for different magazines a scale factor makes that possible

        Show
        Peter Coppens added a comment - (In reply to comment #11) > Yeah, I saw that, but that still doesn't tell me what I'm looking for. What's > the use case for using these properties? What functionality do they offer > that's not in other properties in the specification? Is this for background > images or something? Watermarks? > > Based on Andreas's comments in this bug this sounds like a common feature > request, I've just never heard of it. > > I'm only curious because I use FOP pretty extensively and wonder if these > extensions add something I would find useful. Ah..ic The box'es set the pdf page boxes with the same name. I found http://www.prepressure.com/pdf/basics/page_boxes useful (not necessarily finding the pdf spec an easy read ). These boxes are often used to drive what is actually printed as compared to what e.g. acrobat displays Scale is something we needed to use fop for creating adverts. The size of the output depends on e.g. the magazine you want your advert to be published in. As we want to use same stylesheet for different magazines a scale factor makes that possible
        Hide
        Sean Griffin added a comment -

        Yeah, I saw that, but that still doesn't tell me what I'm looking for. What's the use case for using these properties? What functionality do they offer that's not in other properties in the specification? Is this for background images or something? Watermarks?

        Based on Andreas's comments in this bug this sounds like a common feature request, I've just never heard of it.

        I'm only curious because I use FOP pretty extensively and wonder if these extensions add something I would find useful.

        Show
        Sean Griffin added a comment - Yeah, I saw that, but that still doesn't tell me what I'm looking for. What's the use case for using these properties? What functionality do they offer that's not in other properties in the specification? Is this for background images or something? Watermarks? Based on Andreas's comments in this bug this sounds like a common feature request, I've just never heard of it. I'm only curious because I use FOP pretty extensively and wonder if these extensions add something I would find useful.
        Hide
        Peter Coppens added a comment -

        (In reply to comment #9)
        > I think I might have missed something on the mailing lists explaining these
        > features, but Peter, can you explain what these new properties do? What they
        > offer that it's in the spec?

        See beginning of bugzilla entry which says
        <quote>

        Patch adding support for bleed, trim and crop box and scaling

        The attached patch adds support for 4 new simple-page-master fop extension
        attributes, namely

        • fox:crop-box
        • fox:trim-box
        • fox:bleed-box
        • fox:scale

        The box attributes can consist out of up to 4 numeric values (top,left, width,
        height) and can have units as suffix to each of the numbers.

        The scale attribute can consist out of two numbers each between 0 and 1

        It is implemented for PDF and Java2D renderers.
        </quote>

        Show
        Peter Coppens added a comment - (In reply to comment #9) > I think I might have missed something on the mailing lists explaining these > features, but Peter, can you explain what these new properties do? What they > offer that it's in the spec? See beginning of bugzilla entry which says <quote> Patch adding support for bleed, trim and crop box and scaling The attached patch adds support for 4 new simple-page-master fop extension attributes, namely fox:crop-box fox:trim-box fox:bleed-box fox:scale The box attributes can consist out of up to 4 numeric values (top,left, width, height) and can have units as suffix to each of the numbers. The scale attribute can consist out of two numbers each between 0 and 1 It is implemented for PDF and Java2D renderers. </quote>
        Hide
        Sean Griffin added a comment -

        I think I might have missed something on the mailing lists explaining these features, but Peter, can you explain what these new properties do? What they offer that it's in the spec?

        Show
        Sean Griffin added a comment - I think I might have missed something on the mailing lists explaining these features, but Peter, can you explain what these new properties do? What they offer that it's in the spec?
        Hide
        Peter Coppens added a comment -

        Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v5.patch has been added with description: updated patch in an attempt to address fop-dev suggestions

        Show
        Peter Coppens added a comment - Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v5.patch has been added with description: updated patch in an attempt to address fop-dev suggestions
        Hide
        Andreas L. Delmelle added a comment -

        Just noting this for general interest (has little or nothing to do with this issue per se):
        The previous comments suddenly reminded me that I have always considered the current way that extension /properties/ are handled, as lacking in robustness. We practically force potential implementors of extension properties to modify FOP's codebase. For extension elements or attributes, the pattern is much more open and generic. They can be implemented without necessarily having to modify FOP and recompile. The same should become the case for extension properties, eventually. We probably will want to take a look at offering something like an 'ExtensionPropertyMapping' (currently non-existent), so that implementors can re-use existing PropertyMakers, define their own initial values/enums/keywords, mark properties as inherited, and so on...

        There are already a few 'native' extension properties, for which we define symbolic literals in fo.Constants and which are also registered in fo.FOPropertyMapping. I have never really been too happy with that practice. Contributors are invited to follow that pattern, which will only lead to more clutter.

        Tackling that issue, however, is not for the faint of heart. It would likely also imply revisiting the way properties are attached to the FONodes and how they are exposed to the outside (layoutengine & renderers), so would lead to changes in a Lot of classes (capital 'L', if you catch the drift...)

        Show
        Andreas L. Delmelle added a comment - Just noting this for general interest (has little or nothing to do with this issue per se): The previous comments suddenly reminded me that I have always considered the current way that extension /properties/ are handled, as lacking in robustness. We practically force potential implementors of extension properties to modify FOP's codebase. For extension elements or attributes, the pattern is much more open and generic. They can be implemented without necessarily having to modify FOP and recompile. The same should become the case for extension properties, eventually. We probably will want to take a look at offering something like an 'ExtensionPropertyMapping' (currently non-existent), so that implementors can re-use existing PropertyMakers, define their own initial values/enums/keywords, mark properties as inherited, and so on... There are already a few 'native' extension properties, for which we define symbolic literals in fo.Constants and which are also registered in fo.FOPropertyMapping. I have never really been too happy with that practice. Contributors are invited to follow that pattern, which will only lead to more clutter. Tackling that issue, however, is not for the faint of heart. It would likely also imply revisiting the way properties are attached to the FONodes and how they are exposed to the outside (layoutengine & renderers), so would lead to changes in a Lot of classes (capital 'L', if you catch the drift...)
        Hide
        Andreas L. Delmelle added a comment -

        (In reply to comment #5)
        >
        > Well, since the extension attributes are specific to the PDF renderer, I think

        Sorry, ignore this. I just noticed that in the patch, the extension is also implemented for the Java2D-based renderers...

        Show
        Andreas L. Delmelle added a comment - (In reply to comment #5) > > Well, since the extension attributes are specific to the PDF renderer, I think Sorry, ignore this. I just noticed that in the patch, the extension is also implemented for the Java2D-based renderers...
        Hide
        Andreas L. Delmelle added a comment -

        (In reply to comment #4)
        > - I have a concern about when the properties are actually parsed. Like it is
        > now they will be parsed only at rendering stage, so if there is a mistake in
        > them the error will be thrown rather 'late' in the process (only after layout
        > has been performed). May that be a problem? Do we want a 'fail-fast' behaviour
        > instead? Open question.

        Well, since the extension attributes are specific to the PDF renderer, I think it is not a blocker to treat them as plain 'foreign attributes' at parse-time. One case comes to mind where we would need to treat them as genuine 'properties': suppose we want to be able to specify them as expressions, based on other properties.

        Show
        Andreas L. Delmelle added a comment - (In reply to comment #4) > - I have a concern about when the properties are actually parsed. Like it is > now they will be parsed only at rendering stage, so if there is a mistake in > them the error will be thrown rather 'late' in the process (only after layout > has been performed). May that be a problem? Do we want a 'fail-fast' behaviour > instead? Open question. Well, since the extension attributes are specific to the PDF renderer, I think it is not a blocker to treat them as plain 'foreign attributes' at parse-time. One case comes to mind where we would need to treat them as genuine 'properties': suppose we want to be able to specify them as expressions, based on other properties.
        Hide
        Vincent Hennebert added a comment -

        I've had a more detailed look at the patch and it looks good to me. A few other comments:

        • the definition and parsing of the extension properties should not be put in ExtensionElementMapping, but in a class (and a sub-package) of its own. Where exactly it should be put is not entirely clear to me yet. Probably in a new o.a.f.render.extensions package. How to name the new sub-package also is an open question (scalingcropbleedtrim? rather ugly... pageboundaries (taken from the PDF spec)? prepress?).
        • amendment to what I said about the event notification mechanism: the extension should not use it, rather throw appropriate exceptions, which would be passed over by the PDF library (the classes in o.a.f.pdf) to the PDFDocumentHandler. The former two should remain independent of the notification mechanism to allow later extraction from the FOP codebase and modularization. Only the PDFDocumentHandler must be aware of the notification mechanism.
        • the regexp parsing properties should be made slightly more robust and unit-tested.
        • I have a concern about when the properties are actually parsed. Like it is now they will be parsed only at rendering stage, so if there is a mistake in them the error will be thrown rather 'late' in the process (only after layout has been performed). May that be a problem? Do we want a 'fail-fast' behaviour instead? Open question.

        I'm happy to do all the mentioned changes myself, but this will be in one week at the earliest as I'm away next week. Peter, if meanwhile you want to submit an updated patch feel free to do so

        Thanks,
        Vincent

        Show
        Vincent Hennebert added a comment - I've had a more detailed look at the patch and it looks good to me. A few other comments: the definition and parsing of the extension properties should not be put in ExtensionElementMapping, but in a class (and a sub-package) of its own. Where exactly it should be put is not entirely clear to me yet. Probably in a new o.a.f.render.extensions package. How to name the new sub-package also is an open question (scalingcropbleedtrim? rather ugly... pageboundaries (taken from the PDF spec)? prepress?). amendment to what I said about the event notification mechanism: the extension should not use it, rather throw appropriate exceptions, which would be passed over by the PDF library (the classes in o.a.f.pdf) to the PDFDocumentHandler. The former two should remain independent of the notification mechanism to allow later extraction from the FOP codebase and modularization. Only the PDFDocumentHandler must be aware of the notification mechanism. the regexp parsing properties should be made slightly more robust and unit-tested. I have a concern about when the properties are actually parsed. Like it is now they will be parsed only at rendering stage, so if there is a mistake in them the error will be thrown rather 'late' in the process (only after layout has been performed). May that be a problem? Do we want a 'fail-fast' behaviour instead? Open question. I'm happy to do all the mentioned changes myself, but this will be in one week at the earliest as I'm away next week. Peter, if meanwhile you want to submit an updated patch feel free to do so Thanks, Vincent
        Hide
        Vincent Hennebert added a comment -

        Hi Peter,

        (In reply to comment #2)
        > (In reply to comment #1)
        > > Thanks for this patch, Peter!
        > >
        > > I know this is going to be very useful, as questions regarding that
        > > functionality pop up on the user list from time to time. We'll look into it,
        > > and will keep you informed. I currently have other priorities, but as soon as I
        > > see an available slot of time, if no one beats me to it, I'll go into it in
        > > more detail.
        >
        >
        > I was wondering whether there is anything I can do to make this move forward.

        Not much until we have reviewed the patch and provided comments. Then, if modifications are needed you may want to do them yourself and provide an updated patch, which would speed up its integration in the code base.

        I've only had a quick look so far and can't give much feedback yet. I'll try to have a deeper look in the next days. A few things I noticed, though:

        • the IllegalArgumentException in ExtensionElementMapping will have to be replaced by a call to FOP's event notification mechanism (so that the message can be localized, among others things). Have a look in, e.g., org.apache.fop.fo.flow.Table.java to see how it is used (TableEventProducer in that case)
        • the new features will have to be documented on the website. The corresponding source files may be found in the src/documentation/content/xdocs/trunk directory. The ideal place probably is extensions.xml, with a link in output.xml.
        • there are Checkstyle warnings in the new code. You can set up Checkstyle using the checkstyle-4.0.xml at the root of the project.

        Those are things that we will have to do before applying the patch anyway, so if you want to speed up the process you can have a go at them.

        Back later, hopefully, for comments on the functionalities themselves.

        Thanks!
        Vincent

        Show
        Vincent Hennebert added a comment - Hi Peter, (In reply to comment #2) > (In reply to comment #1) > > Thanks for this patch, Peter! > > > > I know this is going to be very useful, as questions regarding that > > functionality pop up on the user list from time to time. We'll look into it, > > and will keep you informed. I currently have other priorities, but as soon as I > > see an available slot of time, if no one beats me to it, I'll go into it in > > more detail. > > > I was wondering whether there is anything I can do to make this move forward. Not much until we have reviewed the patch and provided comments. Then, if modifications are needed you may want to do them yourself and provide an updated patch, which would speed up its integration in the code base. I've only had a quick look so far and can't give much feedback yet. I'll try to have a deeper look in the next days. A few things I noticed, though: the IllegalArgumentException in ExtensionElementMapping will have to be replaced by a call to FOP's event notification mechanism (so that the message can be localized, among others things). Have a look in, e.g., org.apache.fop.fo.flow.Table.java to see how it is used (TableEventProducer in that case) the new features will have to be documented on the website. The corresponding source files may be found in the src/documentation/content/xdocs/trunk directory. The ideal place probably is extensions.xml, with a link in output.xml. there are Checkstyle warnings in the new code. You can set up Checkstyle using the checkstyle-4.0.xml at the root of the project. Those are things that we will have to do before applying the patch anyway, so if you want to speed up the process you can have a go at them. Back later, hopefully, for comments on the functionalities themselves. Thanks! Vincent
        Hide
        Peter Coppens added a comment -

        (In reply to comment #1)
        > Thanks for this patch, Peter!
        >
        > I know this is going to be very useful, as questions regarding that
        > functionality pop up on the user list from time to time. We'll look into it,
        > and will keep you informed. I currently have other priorities, but as soon as I
        > see an available slot of time, if no one beats me to it, I'll go into it in
        > more detail.

        I was wondering whether there is anything I can do to make this move forward.

        Thanks,

        Peter

        Show
        Peter Coppens added a comment - (In reply to comment #1) > Thanks for this patch, Peter! > > I know this is going to be very useful, as questions regarding that > functionality pop up on the user list from time to time. We'll look into it, > and will keep you informed. I currently have other priorities, but as soon as I > see an available slot of time, if no one beats me to it, I'll go into it in > more detail. I was wondering whether there is anything I can do to make this move forward. Thanks, Peter
        Hide
        Andreas L. Delmelle added a comment -

        Thanks for this patch, Peter!

        I know this is going to be very useful, as questions regarding that functionality pop up on the user list from time to time. We'll look into it, and will keep you informed. I currently have other priorities, but as soon as I see an available slot of time, if no one beats me to it, I'll go into it in more detail.

        Show
        Andreas L. Delmelle added a comment - Thanks for this patch, Peter! I know this is going to be very useful, as questions regarding that functionality pop up on the user list from time to time. We'll look into it, and will keep you informed. I currently have other priorities, but as soon as I see an available slot of time, if no one beats me to it, I'll go into it in more detail.
        Peter Coppens created issue -
        Hide
        Peter Coppens added a comment -

        Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v4.patch has been added with description: Patch adding support for bleed, trim and crop box and scaling

        Show
        Peter Coppens added a comment - Attachment FOP_scalePage_CropBox_BleedBox_TrimBox_v4.patch has been added with description: Patch adding support for bleed, trim and crop box and scaling

          People

          • Assignee:
            Unassigned
            Reporter:
            Peter Coppens
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development