D18179: PDF: Implement scaling for non-rasterized printing

[Albert Astals Cid]

aacid added a comment.


  We would need a @since marker for the enum and for the new methods and a 
  // TODO merge with function above when a BIC change happens somehwere else
  would also make sense

INLINE COMMENTS

> fileprinter.h:58
> + */
> +enum ScaleMode { FitToPrintArea, None };
> +

if this is going to be flags, it should be like this

enum ScaleMode { 
None = 0,
FitToPrintArea = 1
 };

so the next flags can be 2, 4, 8, etc, meaning you can use binary or and binary 
and on them and it'll all work

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, tfella, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn marked an inline comment as done.
michaelweghorn added a comment.


  Thanks for the feedback and info. I've update the change accordingly now.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, tfella, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn marked 2 inline comments as done.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, tfella, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn updated this revision to Diff 49519.
michaelweghorn added a comment.


  Undo accidental whitespace changes

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18179?vs=49518&id=49519

BRANCH
  michaelweghorn/UPDATE_20191115_D18179_allowScaledPrintingWithoutRasterization

REVISION DETAIL
  https://phabricator.kde.org/D18179

AFFECTED FILES
  core/fileprinter.cpp
  core/fileprinter.h
  generators/poppler/generator_pdf.cpp

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, tfella, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn updated this revision to Diff 49518.
michaelweghorn added a comment.


  Implement improvements suggested by Albert:
  
  - use an enum rather than a bool for scale mode
  - avoid ABI breakage by adding extra methods that take an additional 
parameter for scale mode and make existing methods call those
  
  What's a bit ugly here is that 'scaleMode' cannot be the last
  parameter in the new 'FilePrinter::printFile' method, as it
  has to come before those params that have a default value.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18179?vs=49225&id=49518

BRANCH
  michaelweghorn/UPDATE_20191115_D18179_allowScaledPrintingWithoutRasterization

REVISION DETAIL
  https://phabricator.kde.org/D18179

AFFECTED FILES
  core/fileprinter.cpp
  core/fileprinter.h
  generators/poppler/generator_pdf.cpp

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, tfella, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Albert Astals Cid]

aacid added a comment.


  In D18179#391915 , @michaelweghorn 
wrote:
  
  > In D18179#391664 , @aacid wrote:
  >
  > > Maybe an enum is better than a bool so if in the future more scaling 
options are implemented we don't need to change the signature again?
  >
  >
  > Sounds reasonable. I'll change that in the next version.

INLINE COMMENTS

> michaelweghorn wrote in fileprinter.h:82
> I don't really feel up to the task of convincing you. :D
> The only point I can come up with is that your comment in 
> ab96e0c07def2f572a8bb3e32f90e597e6761627 
>  
> indicates that that commit //might// have broken binary compatibility already 
> and thus it might be worth considering bumping the so version anyway (in 
> which case I think this here should no longer be any problem, but I haven't 
> really had to deal with binary compatibility in the past).
> 
> Otherwise, I don't mind adding the suggested extra methods.
> 
> Will those extra methods then stay there forever or can the methods then be 
> "merged" again once another ABI-incompatible change is done?
> 
> Just out of curiosity: Do you know of specific applications/third-party 
> generators,... that use libokularcore?

I'm almost sure that doesn't break binary compatibility since it's a template 
and thus the code would end up on the app side and not the library side, but i 
didn't want to lose sleep making sure.

Once we break ABI we could merge them back, if people realize :D

calligra has a few generators for odt, odp using calligra libs.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn added a comment.


  In D18179#391664 , @aacid wrote:
  
  > Maybe an enum is better than a bool so if in the future more scaling 
options are implemented we don't need to change the signature again?
  
  
  Sounds reasonable. I'll change that in the next version.

INLINE COMMENTS

> aacid wrote in fileprinter.h:82
> this is not binary compatible, we try not to break the binary compatibility 
> of the okular public classes.
> 
> Add another method (without the default values) and make this one call the 
> new one.
> 
> Same for all the functions you're adding new parameters to.
> 
> Or you can try to convince me that maintainting binary compatibilty is not 
> needed :D

I don't really feel up to the task of convincing you. :D
The only point I can come up with is that your comment in 
ab96e0c07def2f572a8bb3e32f90e597e6761627 
 
indicates that that commit //might// have broken binary compatibility already 
and thus it might be worth considering bumping the so version anyway (in which 
case I think this here should no longer be any problem, but I haven't really 
had to deal with binary compatibility in the past).

Otherwise, I don't mind adding the suggested extra methods.

Will those extra methods then stay there forever or can the methods then be 
"merged" again once another ABI-incompatible change is done?

Just out of curiosity: Do you know of specific applications/third-party 
generators,... that use libokularcore?

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Albert Astals Cid]

aacid added a comment.


  Maybe an enum is better than a bool so if in the future more scaling options 
are implemented we don't need to change the signature again?

INLINE COMMENTS

> fileprinter.h:82
>PageSelectPolicy pageSelectPolicy = 
> FilePrinter::ApplicationSelectsPages,
> -  const QString &pageRange = QString() );
>  

this is not binary compatible, we try not to break the binary compatibility of 
the okular public classes.

Add another method (without the default values) and make this one call the new 
one.

Same for all the functions you're adding new parameters to.

Or you can try to convince me that maintainting binary compatibilty is not 
needed :D

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: aacid, fvogt, okular-devel, ngraham, darcyshen

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn added a dependency: D10974: Add option to ignore print margins 
for non-PDF generators.

REPOSITORY
  R223 Okular

REVISION DETAIL
  https://phabricator.kde.org/D18179

To: michaelweghorn, #okular, ngraham, sander
Cc: okular-devel, ngraham, darcyshen, aacid

D18179: PDF: Implement scaling for non-rasterized printing

[Michael Weghorn]

michaelweghorn created this revision.
michaelweghorn added reviewers: Okular, ngraham, sander.
Herald added a project: Okular.
michaelweghorn requested review of this revision.

REVISION SUMMARY
  This extends 'FilePrinter::printFile' by an optional
  parameter to specify whether or not to do scaling and passes
  the 'fit-to-page' to CUPS dependent on what is specified.
  If the parameter is not specified, the behaviour remains
  unchanged.
  
  If FilePrinter is used, The PDF generator now passes this
  option depending on the scaling mode was selected in the
  custom print options widget, which is therefore now enabled
  for non-rasterized printing as well.

TEST PLAN
  1. open a PDF document in Okular and open the print dialog
  2. go to the "PDF Options" tab
  3. verify that "Force rasterisation" is disabled, but the "Scale mode" 
combobox is active.
  4. test all the three options available in the "Scale mode" combobox do what 
they say
  5. Make sure the three options still work as expected for the "Force 
rasterisation" case.

REPOSITORY
  R223 Okular

BRANCH
  michaelweghorn/WIP_allowScaledPrintingWithoutRasterization

REVISION DETAIL
  https://phabricator.kde.org/D18179

AFFECTED FILES
  core/fileprinter.cpp
  core/fileprinter.h
  generators/poppler/generator_pdf.cpp

To: michaelweghorn, #okular, ngraham, sander
Cc: okular-devel, ngraham, darcyshen, aacid