PDF: Implement scaling options for non-rasterized printing
ClosedPublic

Authored by michaelweghorn on Jan 11 2019, 10:32 AM.

Details

Summary

This adds another 'FilePrinter::printFile' method that
accepts an additional parameter to specify whether or not to
do scaling and passes the 'fit-to-page' to CUPS dependent
on what is specified.

If FilePrinter is used, The PDF generator now passes this
option depending on the scaling mode that 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.

Diff Detail

Repository
R223 Okular
Branch
michaelweghorn/UPDATE_D18179
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7216
Build 7234: arc lint + arc unit
Restricted Application added a project: Okular. · View Herald TranscriptJan 11 2019, 10:32 AM
michaelweghorn requested review of this revision.Jan 11 2019, 10:32 AM
fvogt added a subscriber: fvogt.Jan 11 2019, 10:39 AM
aacid added a subscriber: aacid.Jan 11 2019, 9:04 PM

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?

core/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

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.

core/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?

aacid added a comment.Jan 12 2019, 6:15 PM

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.

core/fileprinter.h
82

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.

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.

Undo accidental whitespace changes

michaelweghorn marked an inline comment as done.Jan 15 2019, 12:35 PM

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

michaelweghorn marked 2 inline comments as done.Jan 15 2019, 12:35 PM

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

core/fileprinter.h
58

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

michaelweghorn retitled this revision from PDF: Implement scaling for non-rasterized printing to PDF: Implement scaling options for non-rasterized printing.
michaelweghorn edited the summary of this revision. (Show Details)

Changes in this revision:

  • add '@since' markes and TODO comments to merge functions when a binary incompatible change is done elsewhere
  • adapt enum to prepare for use of binary AND/OR
michaelweghorn marked an inline comment as done.
  • Adapt order of ScaleMode enum values as implicitly suggested in Albert's comment (I missed that at first)

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

I added these. I did not add a TODO comment to merge for the new printFiles() method, though. In the current constellation, merging them would break the API, since the position of the 'scaleMode' parameter is another one (can't be the last param for the new version, since it has no default value) and I guess this is undesirable, even when an ABI change appears?.

If you have any other idea on how to avoid that issue with the printFiles() methods, I'd be thankful to hear about this.

aacid added a comment.Jan 16 2019, 9:21 PM

If you have any other idea on how to avoid that issue with the printFiles() methods, I'd be thankful to hear about this.

The only way i can think of is making the long form not havve default values

https://paste.kde.org/p55lx6vrh

Changes in this revision:

  • make new 'printFiles()' have no default values for parameters, so that param order can be the same as in the original one (thanks Albert!)
  • add "TODO" comment to merge both versions

The only way i can think of is making the long form not havve default values

https://paste.kde.org/p55lx6vrh

Thanks, that looks good. I have done it this way now.

(Note to self: Make sure not to have strange settings in $HOME/.cups/lpoptions while testing ...)

This patch no longer applies cleanly to master:

Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Already up to date.
Deleted branch arcpatch-D18179 (was 8bf1a9117).
 INFO  Base commit is not in local repository; trying to fetch.
Created and checked out branch arcpatch-D18179.
Created and checked out branch arcpatch-D10974.
Checking patch part.cpp...
Checking patch interfaces/printinterface.h...
Checking patch generators/poppler/generator_pdf.h...
Checking patch generators/poppler/generator_pdf.cpp...
Checking patch core/printoptionswidget.h...
Checking patch core/printoptionswidget.cpp...
Checking patch core/fileprinter.cpp...
Checking patch core/document.h...
Checking patch CMakeLists.txt...
Applied patch part.cpp cleanly.
Applied patch interfaces/printinterface.h cleanly.
Applied patch generators/poppler/generator_pdf.h cleanly.
Applied patch generators/poppler/generator_pdf.cpp cleanly.
Applied patch core/printoptionswidget.h cleanly.
Applied patch core/printoptionswidget.cpp cleanly.
Applied patch core/fileprinter.cpp cleanly.
Applied patch core/document.h cleanly.
Applied patch CMakeLists.txt cleanly.

 Cherry Pick Failed!
 Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D10974'

STDOUT
On branch arcpatch-D18179
You are currently cherry-picking commit 4cc9a85f0.

nothing to commit, working tree clean


STDERR
The previous cherry-pick is now empty, possibly due to conflict resolution.
If you wish to commit it anyway, use:

    git commit --allow-empty

Otherwise, please use 'git reset'

(Run with `--trace` for a full exception trace.)

Could you rebase on master?

Never mind, removing the dependency made it work. Stupid Arc. :( Testing now...

Never mind, removing the dependency made it work. Stupid Arc. :( Testing now...

Yet, it also took me a while to figure this out... Looking forward to the GitLab migration :)

ngraham accepted this revision.Wed, Apr 3, 4:48 PM

All options work perfectly in my testing, both with and without rasterized printing enabled. If @aacid is good with the code, I think this could land.

This revision is now accepted and ready to land.Wed, Apr 3, 4:48 PM
aacid added inline comments.Wed, Apr 3, 8:24 PM
core/fileprinter.h
58

the since will need to be 1.8 (and all the other 1.7 you added)

61

Make this no Scale or something similar since otherwise

FilePrinter::None seems like you're saying "none printer"

Changes in this revision:

  • Update the '@since' comments from version 1.7 to 1.8
  • Rename 'ScaleMode::None' to 'ScaleMode::NoScaling'
michaelweghorn marked 2 inline comments as done.Thu, Apr 4, 7:44 AM

i have not tested it, if you think it works, i guess it's ok from the pure code point of view.

OK, let's go for it!

@michaelweghorn I hope you know this means you're on the hook to fix any bugs that crop up, right? :)

This revision was automatically updated to reflect the committed changes.

OK, let's go for it!

@michaelweghorn I hope you know this means you're on the hook to fix any bugs that crop up, right? :)

@ngraham: Thanks for landing this! Yes, I'm aware. :)