Add option to ignore print margins for non-PDF generators
Needs ReviewPublic

Authored by michaelweghorn on Mar 2 2018, 4:47 PM.

Details

Reviewers
None
Group Reviewers
Okular
Summary

This adds a combobox in the print dialog of the non-PDF
generators to allow selecting whether or not to take
print margins into account.

For the PDF case and rasterized printing, new print otions have
been implemented in commit 2e97d587508dff08aaf86ff149c8ed6b7658950d
already, which adds an additional option to do no scaling at all.

For consistency reasons, the same terms also used for the PDF
case are used in the combobox (i.e. the two of the three that
apply).

This adds a new abstract class 'PrintOptionsWidget' with a
'ignorePrintMargins()' method to indicate whether print margins
should be ignored or not, and a default implementation.
The existing widget for the PDF generator now derives from this
class.

In order to avoid an ABI breakage, the return value of
'Document::printConfigurationWidget' is left as a 'QWidget *'
and a dynamic_cast is done on use.

FilePrinter is adapted to take into account the value set by
'QPrinter::setFullPage()' and the margin options
are now passed accordingly (either the values set in the dialog or '0').

A big thanks to Albert Astals Cid <aacid@kde.org> for showing how
to extend the initial implementation to cover more generators.

Test Plan
  1. Open a PostScript file in Okular (using a document size that matches a paper size available on the printer used later makes it easier to see things behave as expected)
  1. open print dialog, go to "Print options" and notice that there is a new "Scale mode" combobox whose value is set to "Fit to printable area" by default.
  1. don't change any options, print to a printer that has hardware margins

Expected result: the document is scaled to the printable area (e.g.
scaled down so that the printer's hardware margins remain empty) as it
has been without this change.

  1. Set the value of the "Print Options" -> "Scale mode" combobox to "Fit to full page" and print again

Expected result: The document is scaled to the full page size, i.e. ignoring
the printer's hardware margins.

  1. Try steps 1-4 with other document formats supported by Okular and observe that they behave the same (except for the PDF case, where there's a combobox with three options that has been implemented independent of this change).

Diff Detail

Repository
R223 Okular
Branch
michaelweghorn/UPDATE_D10974
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7205
Build 7223: arc lint + arc unit
michaelweghorn created this revision.Mar 2 2018, 4:47 PM
Restricted Application added a project: Okular. · View Herald TranscriptMar 2 2018, 4:47 PM
michaelweghorn requested review of this revision.Mar 2 2018, 4:47 PM

In general, this option is not only relevant for the PDF generator, but (as Albert mentioned on the bug report) for all generators that use FilePrinter.
This change currently only addresses the PDF case.

I have created a separate class PrintScalingOptionWidget that could in general be used by other generators as well, if it were put to a more appropriate place (e.g. an own source file, but I have no idea what would be the right place).
Otherwise, the functionality should probably just be moved to the PDFOptionsPage class (which would however result in code duplication when you want to add it for other generators as well).
I'd be happy about direction on what is best here.

In addition, the alignment of the checkboxes in the "PDF options" tab is currently non-optimal. The checkbox from the nested PrintScalingOptionWidget is a little bit more on the right. I guess that can probably be fixed by setting some option in one of the widgets, but I couldn't figure that out quickly ignored it for this first revision.

Once https://phabricator.kde.org/D7962 should make it to Okular, it might make sense to reconsider using the combobox for the scaling options that this adds for the PDF generator case (or how to properly merge the two approaches for the QPrinter and the lpr approaches).

ngraham added a subscriber: ngraham.Mar 2 2018, 8:40 PM
aacid added a subscriber: aacid.Mar 3 2018, 6:30 PM

so you worded point 4 and point 6 (second point 5) differently "the document is not scaled at all now." vs "The document is scaled to the full page size", i would not expect Force Rasterize to cause different behaviour.

Is it just that you wrote it differently or does Force Rasterize actually change the behaviour of "Fit to printable area"?

so you worded point 4 and point 6 (second point 5) differently "the document is not scaled at all now." vs "The document is scaled to the full page size", i would not expect Force Rasterize to cause different behaviour.

Is it just that you wrote it differently or does Force Rasterize actually change the behaviour of "Fit to printable area"?

As of now, it actually does behave differently. The "Force rasterization" option does not use FilePrinter, but Qt's "normal way of printing" (which is why e.g. n-up-printing also behaves a little different there). For the "Force rasterization" case, the only thing that changes is whether the printer margin is taken into account or not. If you print an A4 document to A3 paper and unselecting the "Fit to printable area", currently

  • FilePrinter would not scale the document at all, thus leaving a lot of blank space on the printout.
  • The "force rasterization" option would scale the document to A3, ignoring the printer hardware margins.

Thinking about this again, I think FilePrinter should react to the value returned by QPrinter::fullPage() in the same way as the "force rasterization" option, since this is what the option actually means [1]:

void QPrinter::setFullPage(bool fp)
If fp is true, enables support for painting over the entire page; otherwise restricts painting to the printable area reported by the device.

Rather than not setting the "fit-to-page" option, the margins should not be set in FilePrinter in this case.

When document page size and the printer's page size is the same, this does not make any difference as compared to not setting the fit-to-page option. But it does make a diference when the two page sizes do not match (as described above).

What do you think?

1: http://doc.qt.io/qt-5/qprinter.html#fullPage

(Don't) Set margins based on QPrinter::fullPage()

This modifies the draft solution as I described in comment
https://phabricator.kde.org/D10974#217969 .

michaelweghorn retitled this revision from PDF: Make the "fit-to-page" print option configurable to PDF: Allow to ignore print margins.Mar 9 2018, 3:49 PM
michaelweghorn edited the summary of this revision. (Show Details)
michaelweghorn edited the test plan for this revision. (Show Details)
aacid added a comment.May 18 2018, 1:56 PM

Looks good, i'm going to try to make the PrintScalingOptionWidget be shown for all the formats and not only for PDF before commiting, should "hopefully" not be very hard.

Restricted Application added a subscriber: okular-devel. · View Herald TranscriptMay 18 2018, 1:56 PM
aacid added a comment.May 18 2018, 2:01 PM

Actually on a second read of the texts i'm not sure i like them.

The feature it's not really "Fit to printable area", it's more "Take Margins into account" or if we negate it "Ignore printer margins", because "printable area" is the area of the page where the printer can print, but what we're doing here is ignore the printer margins, that default to the printable area but the user can change at will.

What do you think?

aacid added a comment.May 18 2018, 2:45 PM

My suggestion to make this avaialble to all generators https://paste.kde.org/pdnznhh7m

[...]
The feature it's not really "Fit to printable area", it's more "Take Margins into account" or if we negate it "Ignore printer margins", because "printable area" is the area of the page where the printer can print, but what we're doing here is ignore the printer margins, that default to the printable area but the user can change at will.

What do you think?

I agree. I personally like "Ignore printer margins" best.
Thanks also for your suggestion on how to make it available for all generators.! I'll try to have a closer look sometime next week, since I'm away for this weekend.

michaelweghorn retitled this revision from PDF: Allow to ignore print margins to Add option to ignore print margins.
michaelweghorn edited the summary of this revision. (Show Details)
michaelweghorn edited the test plan for this revision. (Show Details)

Changes to previous version:

  1. take over Albert's suggestion to make option available to all generators "as is" (except for fixing a minor typo in a comment ("cad" -> "can") (A big thanks to Albert!)
  2. rename dialog option from "Fit to printable area" to "Ignore printer margins" and adapt variable names etc. accordingly
  3. rebase onto current git master
  4. Fix issue that hardware printer margins were still taken into account when option was set not to do so.

Detail on 4): Further testing with more real printers showed that just passing
the option "fit-to-page" to CUPS without explicitly setting any margins causes
the defaults from the PPD to be used, which is not what is excpected.
This is fixed now by explicitly passing '0'.

(Side note: I missed this at first, since values for the margins in the Qt print
dialog currently don't seem to be initialized with the PPD default values.)

@aacid: What's the right way to properly indicate that an important part of this
was actually done by you (add you in the copyright, mention in the commit message,...)?

[....]
(Side note: I missed this at first, since values for the margins in the Qt print
dialog currently don't seem to be initialized with the PPD default values.)

Or, to be more precise after a glance: A minimum of 10 pt is used for the initialization and printer defaults for the print margins are only used when they are greater, which does make some sense. (Margins smaller than 10pt can still be set manually if needed). But this is rather off-topic for this change, anyway...

From a user perspective, I'm not sure I would know what "Ignore printer margins" means. What's a printer margin? If I turn this on or off, what will happen? It's not very clear at the moment, IMHO.

aacid added a comment.May 24 2018, 5:42 PM

@aacid: What's the right way to properly indicate that an important part of this
was actually done by you (add you in the copyright, mention in the commit message,...)?

Just add a line to the commit message and it's fine for me

From a user perspective, I'm not sure I would know what "Ignore printer margins" means. What's a printer margin? If I turn this on or off, what will happen? It's not very clear at the moment, IMHO.

Thanks for your opinion on this. Do you have a suggestion on how to improve this?

@aacid: What's the right way to properly indicate that an important part of this
was actually done by you (add you in the copyright, mention in the commit message,...)?

Just add a line to the commit message and it's fine for me

OK, will do with the next revision.

Stepping back a bit, I can identify the following two use cases:

  • The document has its own margins that will make it fit within the printer's printable area without any scaling (e.g. a scientific paper or an eBook)
  • The document will not fit within the printer's printable area without some scaling, either because it has no margins, or because its own margins are not big enough (e.g a flyer, magazine article, or advertisement)

In the first case, you never need additional margins and always want the document printed with no scaling.

In the second case, you always need to scale the document down to fit within the printable area, or else the edges will be clipped and you'll lose some of the content.

If this analysis is complete, then we don't actually need a user-facing setting here at all; we should instead infer the correct setting for ourselves by seeing whether the document to be printed has its own margins that make it fit within the printer's printable area: if it does, print as-is; if it doesn't, then scale it until it fits.

Am I on the right track, or is this an incomplete assessment?

I took the time to read the attached bug report and I take back what I said in the prior comment. I see now why one would want to manually trigger this behavior.

This is basically a scaling setting, but we also want it to be really obvious what each state is for, because it's a somewhat technical feature.

If we use a checkbox, the string I would recommend is "Scale down to fit within printable area".

Another option is to use a radio button:

Scaling: (o) None
         ( ) Auto-fit within printable area

A nice side-effect of the radio button approach is that when we add fixed percentage scaling, it naturally fits in the same group:

Scaling: (o) None
         ( ) Auto-fit within printable area
         ( ) Scale to fixed percentage: [100%]

Or even:

Scaling: (o) [100%]
         ( ) Auto-fit within printable area

A further enhancement would be to automatically select an appropriate option by default according to the logic I outlined above: if the document actually does fit within the printable area due to its own margins, or because the printer supports borderless printing, don't scale it down by default unless the user deliberately chooses that option.

Does that make sense?

aacid added a comment.May 25 2018, 2:15 PM

If we use a checkbox, the string I would recommend is "Scale down to fit within printable area".

I don't like the wording "printable area" because it's not clear what it means.

What does "printable area" mean for you? It's the area that your printer can actually print on (i.e. page minus hardware margins) or it is the area defined by the margins on the print configuration dialog?

For me "printable area" means the first, but what we're doing is the second, which sometimes happen to match the first value, but the user can change them (to bigger not to smaller).

That's why i think it makes much more sense to use the word margins, which is something the user has seen on the UI and can relate to.

if the document actually does fit within the printable area due to its own margins

That's very difficult to know.

aacid added a comment.May 25 2018, 2:18 PM

Stepping back a bit, I can identify the following two use cases:

  • The document has its own margins that will make it fit within the printer's printable area without any scaling (e.g. a scientific paper or an eBook)
  • The document will not fit within the printer's printable area without some scaling, either because it has no margins, or because its own margins are not big enough (e.g a flyer, magazine article, or advertisement)

    In the first case, you never need additional margins and always want the document printed with no scaling.

    In the second case, you always need to scale the document down to fit within the printable area, or else the edges will be clipped and you'll lose some of the content.

    If this analysis is complete, then we don't actually need a user-facing setting here at all; we should instead infer the correct setting for ourselves by seeing whether the document to be printed has its own margins that make it fit within the printer's printable area: if it does, print as-is; if it doesn't, then scale it until it fits.

    Am I on the right track, or is this an incomplete assessment?

Your assessment is incomplete. There's times you want more margins than the ones the document has, for wathever reasons you may have, there's time in which you want an exact 1:1 print of the document and will be happy to get lost content if the important content which is usually at the center is correctly to scaled.

This is something you can't solve by not having an option if you want to do right.

[...]
Another option is to use a radio button:

Scaling: (o) None
         ( ) Auto-fit within printable area

[...]

Maybe a radio button might actually help to make clearer what happens; it also makes it possible to explicitly describe both options (taking margins into account or not). Proper labels would still be needed, though.
Note that with the current implementation, the option that does not consider margins still does scaling in case the page size of the document and the printout do not match, so "Scaling: None" does not really describe what happens in all use cases, s. comment https://phabricator.kde.org/D10974#217969 above for more details. Maybe something like "Scale to full page" or "Auto-fit to full page" is more appropriate. I currently don't have any good idea on how to call both options so that they're both correct and easily understandable.

Just to mention it, the current revision uses the term "print margin", not "printer margin", basically for the reasoning Albert described. (In my ears, "printer margins" sounds more like "hardware-specific margins" than what the user set in the page setup tab.)

I'm not really sure by myself yet, but does it help to say "print area" instead of "printable area" (i.e. "Fit to print area") to take into account that the margins being used are not necessarily the hardware margins of the printer (which define the "printable area")? Would it be clear for users what this refers to?

Maybe it's not going to help, but how do other printing interfaces (free - Gtk+, libreoffice, mozilla,...or not - windows, macOS) deal with this? A bit of consistency (if it exists) *may* help (even if not 100% precise).

Maybe it's not going to help, but how do other printing interfaces (free - Gtk+, libreoffice, mozilla,...or not - windows, macOS) deal with this? A bit of consistency (if it exists) *may* help (even if not 100% precise).

This feature is important to me (and is in fact why I'm still using evince even on KDE). So here are some screen shots of how this is handled in applications I have on hand. There doesn't appear to be much consistence. However, I think the Gnome version (3 options in a drop down) is pretty clear. I think anyone who cares about this will know what the "printable area" is and no one else will need to see this option.

Gnome (Evince):

Firefox:

These shots were taken under KDE, but I don't think that will affect anything other than themeing.

@aacid , @ngraham : Any chance we can reach a concensus here?
I personally don't have a strong opinion on what the best term to use in the dialog is and could live with all suggestions made so far.
Looking at the Firefox screenshot, the option "Ignore print margins and scale to full page" came to my mind in addition (which is quite long, but might be clear on what is happening?).

ngraham added a comment.EditedJun 7 2018, 5:20 PM

I personally don't think "printable area" is a problematic or confusing string, but I'm willing to compromise on that.

macOS uses "Scale to fit paper size" BTW.

I like the GTK dialog's "Page scaling" combobox with multiple options because this really is a scaling setting. I would support using a combobox, (or radio buttons if we have the space). For the string in question let me throw out some more options:

  • Shrink to fit if necessary
  • Fit on page, shrinking if necessary
  • Scale to fit

Yeah, maybe having two options in a combo/radio is better than just having one checkbox. that way since we have two options it's easier to explain what happens when the checkbox is not checked since we have the alternate option text.

I do like trying to have similar text to the GTK/Evince options too (Even though it's three options and we would only have two).

Do you think we can agree on those?

Do you think we can agree on those?

+1 no objection so we can move this forward.

I might have missed something, but I'm not yet sure what the "correct" wording for the two options in the radio button or combobox would be. :-)

Trying to stick with what Evince uses, "Fit to printable area" might be one. Is this one OK?
Should the other one be called something like "Fit to full page", "Fit to whole page", "Ignore print margins and fit to full page" or "Fit to full page (ignore print margins)" then? (since "No Scaling" or "None" does not really fit for all cases, s. https://phabricator.kde.org/D10974#268328 )? Evince does not seem to have an equivalent for this.

arthurpeters added a comment.EditedJun 29 2018, 5:32 PM

I like "Fit to printable area". I think the idea of the area that is "printable" is accurate and clear. A tooltip specifying that the area we are talking about is the area that the printer can print might be useful.

The "opposite" is more complex. The problem that there are actually multiple options people have mentioned. Some of which I hadn't thought of. The three other options that I've seen are:

  1. Shrink to printable area: meaning only rescale if too large, do not enlarge to printable area.
  2. Scale to full/physical/whole page: Scale the file, but to the phyical page ignoring the printable area limitations of the printer.
  3. No scale/100%: Print it at exactly the size represented in the file. Do not scale at all. Presumably, centered with respect to the output page.

I think we should include 3 options: "Fit to printable area", "Fit to paper size", "Do not fit" (where "do not fit" means fix scaling at exactly 100%). "Fit to printable area" is important for PDFs without margins where absolute size does not matter. "Fit to paper size" is important for PDFs with margins but with the wrong paper size (like A4 vs Letter). "Do not fit" is important for things where absolute size matters (like cutting templates or PCB masks).

If we want to have only two options, I would prefer dropping "Fit to paper size", since in many cases "Do not fit" (100% scale) can often simulate "Fit to paper size", but the opposite is never true.

[...]
Trying to stick with what Evince uses, "Fit to printable area" might be one. Is this one OK?
Should the other one be called something like "Fit to full page", "Fit to whole page", "Ignore print margins and fit to full page" or "Fit to full page (ignore print margins)" then? (since "No Scaling" or "None" does not really fit for all cases, s. https://phabricator.kde.org/D10974#268328 )? Evince does not seem to have an equivalent for this.

rkflx added a subscriber: rkflx.

FWIW, earlier in D7962 there was this suggestion:

In D7962#150260, @rkflx wrote:

Scale mode: fit-to-page vs. shrink-to-page vs. none
Scale to: printable area vs. full page

Having "page" in the names for the scale modes while "Scale to" has "page" in the name half of the time seems like it could confuse some users. "Mode" is a quite technical (and often meaningless) term, too. In addition, it's very hard without looking at the code and with no extra explanation available to know what the actual difference between "fit" and "shrink" is. Ideas (could be improved, though):

  • Scaling: Shrink oversized pages only | Expand or shrink to fit to page | None (← Note how the first/default mode is different – I don't think users would expect their printouts getting scaled up by default like in your Diff.)
  • Scale to: Printable area | Paper size
  • Scale to: Printable area | Paper size

+1, I would be fine with this.

@arthurpeters: Thanks for your ideas on this. For now, I'd like to stick with the two options that have been discussed so far in this change (see reasoning in D10974#217969) and suggest to handle potential further improvements separately.

  • Scale to: Printable area | Paper size

+1, I would be fine with this.

@rkflx , @ngraham: Thanks for your feedback. Unless there's any objection to this, I'll try to have a closer look at this sometime next week (also after looking more closely at Oliver's updated printing-related changes D7949 and D7962).

[...] Unless there's any objection to this, I'll try to have a closer look at this sometime next week (also after looking more closely at Oliver's updated printing-related changes D7949 and D7962).

Currently, I think it makes sense to first wait a bit how D7949 and D7962 proceed, in particular since some code between D7962 and this diff could be shared (and working on both in parallel will make it necessary to fix merge conflicts...).

fvogt added a subscriber: fvogt.Dec 4 2018, 8:48 PM

D7962 just landed! :)

D7962 just landed! :)

Yes, thanks for driving this forward. I'll have a closer look again once I find the time.

Is this even still necessary now that we have D7962? The new scaling combobox has an option to "Fit to full page" which ignores the margins (To respect them, you use "Fit to Printable area")

Is this even still necessary now that we have D7962? The new scaling combobox has an option to "Fit to full page" which ignores the margins (To respect them, you use "Fit to Printable area")

I think it still makes sense in general, since this change here also provides the option for other backends (not only PDF) and rasterized printing of PDF files can create huge files and be very slow, depending on the PDF files and printers. And I think it would also be desirable from a usability point of view to not have the combobox for scaling options greyed out unless rasterized printing is used.
I'll think about this once again, and what might be a good way to go ahead...

I agree that the current UI that makes you click "Force Rasterization" first is not ideal. Kudos if you find a technically acceptable way to improve that! :)

michaelweghorn retitled this revision from Add option to ignore print margins to Add option to ignore print margins for non-PDF generators.
michaelweghorn edited the summary of this revision. (Show Details)
michaelweghorn edited the test plan for this revision. (Show Details)

Changes in this version:

  • rebase to current master, including Oliver's printing changes for PDF
  • adapt GUI elements to match those used for the PDF case (only "Fit to printable area" and "Fit to full page" though)
  • only cover non-PDF generators in this change (PDF case will be dealt with in a subsequent change)

I agree that the current UI that makes you click "Force Rasterization" first is not ideal. Kudos if you find a technically acceptable way to improve that! :)

My approach to do so is in https://phabricator.kde.org/D18179, which depends on this one here.

I've split this change into two now, with this one now covering non-PDF generators (so exactly the opposite of the initial version of my patch...), and the other one (D18179) allowing the FilePrinter case to handle the additional option in the PDF generator's print dialog as well.

aacid added inline comments.Jan 11 2019, 9:08 PM
core/document.h
731

this is binary incompatible :/

We could just let it be a QWidget but document it has to be a PrintOptionsWidge subclass, and then on the user do a dynamic_cast and complain if it is not a PrintOptionsWidget, but maybe we can just say meh and break the BC.

generators/poppler/generator_pdf.cpp
141

remove the virtual

michaelweghorn edited the summary of this revision. (Show Details)

Adapt according to Albert's feedback:

  • undo ABI breakage by changing back signature of 'Document::printConfigurationWidget()' to what it is like without this change, and use a 'dynamic_cast' in 'Part::slotPrint'.
  • drop 'virtual' for 'PDFOptionsPage::ignorePrintMargins()'
michaelweghorn marked an inline comment as done.Jan 15 2019, 10:43 AM
michaelweghorn added inline comments.
core/document.h
731

I have changed the return type to a QWidget* again, but only for the Document class itself, not in the other places. As far as I understand, this place here is the only one relevant for the ABI. Is this correct?

Currently, there is no complaint if a plain QWidget* is returned, it's just handled as it used to be then. Should I change that (e.g. emit a 'qWarning()')?

michaelweghorn edited the summary of this revision. (Show Details)Jan 15 2019, 10:45 AM
aacid added inline comments.Jan 15 2019, 10:04 PM
core/document.h
731

interfaces/ is also installed when doing make install so compatibility there is also important.

Since you have a dynamic cast in part.cpp i guess it makes sense to have a warning when the dynamic cast fails? Telling the "developer" that he should update her implementation of the method?

Changes in this revision:

  • undo changes in 'interfaces/'
  • emit a qWarning() when 'dynamic_cast' fails
aacid added a comment.Jan 16 2019, 9:07 PM

I don't really have the time to spend print stuff to check if this does what it says it does, so i'm just doing code review from the pure formal way.

core/document.cpp
83 ↗(On Diff #49608)

i guess you don't need this include?

core/printoptionswidget.h
47

i think best practices say to not include virtual when it's an override

Adapted according to Albert's feedback:

  • Remove (now) unneeded imclude
  • remove 'virtual' keyword in derived class
michaelweghorn marked 4 inline comments as done.Jan 17 2019, 10:16 AM

I don't really have the time to spend print stuff to check if this does what it says it does, so i'm just doing code review from the pure formal way.

Many thanks for taking the time to look at this anyway!