D7962: Implement several new print scaling options

[Oliver Sander]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R223:2e97d587508d: Implement several new print scaling options 
(authored by sander).

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7962?vs=47981=48037

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

AFFECTED FILES
  generators/poppler/generator_pdf.cpp

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  Thanks for testing!

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  The UI looks good now and all three options work great with my printer and a 
few assorted documents!

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham removed a dependency: D7949: Allow to print pdf doc directly into a 
QPrinter.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  The patch here does not depend on D7949  
anymore.  It should fit directly onto the current master.  D7949 
 does indeed need rebasing, which I will do 
as soon as this patch here has its final form.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Oliver Sander]

sander edited the summary of this revision.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  Thanks! Would you mind re-basing D7949  on 
current master and this patch on D7949 ? I 
can't seem to get both to apply, only one at a time.
  
  Alternatively, we could quickly review and land D7949 
 if it makes sense on its own.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham edited the summary of this revision.
ngraham added a dependency: D7949: Allow to print pdf doc directly into a 
QPrinter.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Oliver Sander]

sander updated this revision to Diff 47981.
sander added a comment.


  Changed the GUI as suggested by Nathan.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7962?vs=43948=47981

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

AFFECTED FILES
  generators/poppler/generator_pdf.cpp

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  Awesome, thanks so much! Let me know if you need any clarification from me.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  Yes and no. :-) I was ready to implement whatever you wish without any 
further arguments, but then I got sidetracked with other things.  It is still 
in the back of my head, though.  Let me see if I get around to implementing 
your GUI before the weekend.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  I think I scared @sander away. :(
  
  Maybe we should land this as-is and work in improving the UI in a later 
patch? Or is the internal representation of these options so tied to the UI 
that it would be a huge pain in the butt?

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: fvogt, bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid, 
darcyshen

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  In D7962#346063 , @sander wrote:
  
  > This is accurate.
  
  
  Great, that's what I thought. In this case, we can actually remove one of the 
comboboxes entirely, winding up with a substantially clearer and better user 
interface:
  
Scaling:  () None; print original size
  () Fit to full page
  () Fit to printable area
  
  Why? Well, there is actually no need to expose FitToPage and ShrinkToPage in 
the UI at all; we can simply always use FitToPage functionality. Consider the 
following use cases:
  
  - Document is is larger than printable area or full page and has no built-in 
margins: choose Fit to printable area
  - Document is is larger than printable area or full page and has built-in 
margins that you know are larger than the printer's own margins: choose Fit to 
full page
  - Document is smaller than printable area or full page and you don't want it 
to be scaled at all: choose None; print original size
  - Document is smaller than printable area or full page and has no built-in 
margins and you want to scale it up as big as possible: choose Fit to printable 
area
  - Document is smaller than printable area or full page and has built-in 
margins and you want to scale it up as big as possible: choose Fit to full page
  
  In no case does anyone actually want to use `ShrinkToPage`; if you don't want 
your document to be scaled up at all, you simply choose the None; print 
original size option.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  In D7962#346063 , @sander wrote:
  
  > This is accurate.
  
  
  Great, that's what I thought. In this case, we can actually remove one of the 
comboboxes entirely, winding up with this:
  
Scaling:  () None; print original size
  () Fit to full page
  () Fit to printable area
  
  Why? Consider the following use cases:
  
  - Document is is larger than printable area or full page and has no built-in 
margins, and you want to make sure nothing gets cut off: choose Fit to 
printable area

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  This is accurate.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  In D7962#339567 , @sander wrote:
  
  > ScaleMode:
  >
  > - None: Do not apply scaling at all
  > - FitToPage: Scale the page to be printed such that it fits the target area 
as good as possible, without changing the aspect ratio
  > - ShrinkToPage: Like FitToPage, but only scale down.
  >
  >   ScaleTo:  What is the actual target area that we are printing onto?
  > - FullPage: The complete physical piece of paper
  > - PrintableArea: Only that part of the physical paper that the printer can 
print onto.
  
  
  Ah, now I get it. One more question: is the following accurate?
  
  - Document is larger than printable area or full page and `ScaleTo` is not 
`None`: `FitToPage` and `ShrinkToPage` do the same thing
  - Document is smaller than printable area or full page and `ScaleTo` is not 
`None`: `FitToPage` makes it grow to fit the target, and `ShrinkToPage` has no 
effect

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  ScaleMode:
  
  - None: Do not apply scaling at all
  - FitToPage: Scale the page to be printed such that it fits the target area 
as good as possible, without changing the aspect ratio
  - ShrinkToPage: Like FitToPage, but only scale down.
  
  ScaleTo:  What is the actual target area that we are printing onto?
  
  - FullPage: The complete physical piece of paper
  - PrintableArea: Only that part of the physical paper that the printer can 
print onto.
  
  As the printable area typically has margins on all four sides, the two 
ScaleTo options not only influence the size of the print output when scaling is 
enabled, they also influence the output position.  For example, when printing 
an A5 page onto an A4 sheet of paper without scaling, the A5 page will appear 
in the very upper left corner of the physical page if FullPage is selected.  
Switching to PrintableArea will move it a little bit to the right and 
downwards, to account for the upper and left printer margins.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  I'm afraid I cannot agree that this should be fixed with documentation. Users 
don't read documentation. And they shouldn't have to read documentation to 
access basic features like print output scaling. The interface needs to hide 
unnecessary implementation details and be as self-documenting as possible. This 
current patch does neither: it demands that users understand details of Qt's 
PDF rendering implementation ("You need to have "Force Rasterization" turned 
on) and offers no clues that this is connected to the hidden scaling features.
  
  I'm very happy that the scaling features are being implemented, and I applaud 
the work you've done so far! But features that are not discoverable because 
they are hidden behind technical proficiency tests might as well not exist for 
99% of users since they will never be discovered. I think we can do a better 
job on the user interface. I'm happy to work with you on this, or if you're 
thoroughly sick of me by now (), we can involve other members of #VDG 
. Offering such guidance is one of the 
big reasons why VDG exists, in fact.
  
  Probably the minimum change I would want to approve this would be a label 
beneath the disabled scaling controls that says "Turn on 'Force Rasterization' 
to enable scaling options" when Force Rasterization is turned off. But I still 
think we can do even better: in particular, I still don't really understand 
what the Scale Mode options do.
  
  I know it's been a really long road for this patch, but I think we can get 
all the way to a nice UI without too much more effort!

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  > Is there a good UI reason to force users to manually click "force 
rasterization", or is it just technical difficulty?
  
  My current approach is the most simple one; I am not a fan of buttons etc 
that change their value depending on other buttons (other than enabledness).
  
  Also, I'm a pretty sure that people will be surprised when selecting a new 
scaling option automagically turns on rasterization.
  
  All your objections are valid in a sense, but I think they should be covered 
in the documentation.  I have tried to find out how to properly extend the 
docbook file that comes with Okular, and failed.  Is there any good 
documentation available on how to locally build those files into something 
legible?
  
  Given the promise of future documentation improvements, can this patch be 
approved?

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: bruns, okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  >> Since the image always needs to be rasterized before the new scaling 
options work... why don't we keep the scaling options enabled and turn on 
rasterization automatically when they're used? Basically we just remove the 
manual step of making the user check Force rasterization if they want to scale 
the document. We assume that if they want to scale the document, they're 
willing to accept whatever technical changes are required.
  > 
  > To be honest, this sounds like even more magic than what is happening right 
now.  And it will become even more confusing if we ever merge 
https://phabricator.kde.org/D7949 . Alternative suggestion: I'll let the 
tooltip hint at the rasterization option.
  
  Is there a good UI reason to force users to manually click "force 
rasterization", or is it just technical difficulty?
  
  > 
  > 
  >> When using Scale to: Full page, it might be nice if some warning text 
could appear below the combobox notifying the user that the document may get 
cut off at the edges if it does not include its own margins.
  > 
  > Isn't that obvious?  Besides, there are other ways to cut off your document 
too (e.g. print A3 document onto A4 paper with scaling turned off).
  
  Hm, it wasn't obvious to me.
  
  >> What is the relationship between the two options? For example, if I choose 
Scale Mode: None and Scale to: Printable Area, I don't have a clear picture of 
what will happen. The first one seems to imply that there will be no scaling, 
but the second one allows me to choose what the scaling target will be. It 
seems like Scale Mode: None should disable the second one, but it doesn't.
  > 
  > The difference between 'none, printable area'  and 'none, full page' is the 
position on the page where the document is printed too.
  
  I'm afraid that still doesn't help me understand what will happen. :) I would 
expect that Scale Mode: None would result in no scaling at all, which is why 
it's confusing to then have another control to change the type of scaling that 
I'm under the impression won't be applied.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  > because gating these scaling options behind Force rasterization is not very 
user friendly
  
  Indeed.
  
  > Since the image always needs to be rasterized before the new scaling 
options work... why don't we keep the scaling options enabled and turn on 
rasterization automatically when they're used? Basically we just remove the 
manual step of making the user check Force rasterization if they want to scale 
the document. We assume that if they want to scale the document, they're 
willing to accept whatever technical changes are required.
  
  To be honest, this sounds like even more magic than what is happening right 
now.  And it will become even more confusing if we ever merge 
https://phabricator.kde.org/D7949 . Alternative suggestion: I'll let the 
tooltip hint at the rasterization option.
  
  > When using Scale to: Full page, it might be nice if some warning text could 
appear below the combobox notifying the user that the document may get cut off 
at the edges if it does not include its own margins.
  
  Isn't that obvious?  Besides, there are other ways to cut off your document 
too (e.g. print A3 document onto A4 paper with scaling turned off).
  
  > What is the relationship between the two options? For example, if I choose 
Scale Mode: None and Scale to: Printable Area, I don't have a clear picture of 
what will happen. The first one seems to imply that there will be no scaling, 
but the second one allows me to choose what the scaling target will be. It 
seems like Scale Mode: None should disable the second one, but it doesn't.
  
  The difference between 'none, printable area'  and 'none, full page' is the 
position on the page where the document is printed too.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


  I gave this a try today and it works nicely! Awesome work! I'd still like 
some user interface improvements though, because gating these scaling options 
behind Force rasterization is not very user friendly: the connection between 
that setting and the scaling options is not likely to be apparent to any user 
who does not also happen to be a PDF expert or an Okular developer. :)
  
  Here are some ideas to make the presentation a bit more user-friendly:
  
  - Since the image always needs to be rasterized before the new scaling 
options work... why don't we keep the scaling options enabled and turn on 
rasterization automatically when they're used? Basically we just remove the 
manual step of making the user check Force rasterization if they want to scale 
the document. We assume that if they want to scale the document, they're 
willing to accept whatever technical changes are required.
  - When using Scale to: Full page, it might be nice if some warning text could 
appear below the combobox notifying the user that the document may get cut off 
at the edges if it does not include its own margins.
  - What is the relationship between the two options? For example, if I choose 
Scale Mode: None and Scale to: Printable Area, I don't have a clear picture of 
what will happen. The first one seems to imply that there will be no scaling, 
but the second one allows me to choose what the scaling target will be. It 
seems like  Scale Mode: None should disable the second one, but it doesn't.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg, ngraham
Cc: okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a reviewer: VDG.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid, #vdg
Cc: okular-devel, cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander updated this revision to Diff 42181.
sander added a comment.


  I updated the diff.  Apologies for the last version---it was really messed up.
  
  The new version brings a few changes:
  
  - The diff does *not* depend on https://phabricator.kde.org/D7949 anymore! 
Pro: the trusted Poppler Splash backend does the rendering, with very good 
results.  Con: Works only if 'force rasterization' is enabled.
  - It actually does work if 'force rasterization' is enabled. Previous diffs 
only claimed that...
  - The new options are now only disabled when 'force rasterization' is 
deselected, not hidden.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7962?vs=38128=42181

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

AFFECTED FILES
  generators/poppler/generator_pdf.cpp

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

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  No problem, and don't let me rush you!
  
  If these new options are hidden or enabled conditionally on the basis that 
they are not yet ready for prime time or are a tech preview, then what I would 
like to see is for this to be made explicit, probably in the Settings window:
  
[] Enable experimental printing features
   Turns on experimental support for native Qt printing.
   This offers more options, but may be buggy. Use at your own risk.
  
  Otherwise I worry that the conneciton between "force rasterization" and "show 
me all the print scaling options" will only be apparently to the developers 
(i.e. you :) ).

REPOSITORY
  R223 Okular

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

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

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  Sorry for the delay, I have been wanting to fix this for quite a while now.  
Now that you nudge me about it I'll promise to update the patch before the end 
of the weekend.
  
  Actually, I was going to remove the dependence on 
https://phabricator.kde.org/D7949 from this patch, and have the new options 
appear only when 'rasterization' is set. It is my hope that this makes the 
patch less contentious. Any opinions on this?

REPOSITORY
  R223 Okular

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

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

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  Any movement on Henrik's review comments?

REPOSITORY
  R223 Okular

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

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

D7962: Implement several new print scaling options

[Henrik Fehlauer]

rkflx added inline comments.

INLINE COMMENTS

> generator_pdf.cpp:80-89
> +   bool isVisible = (printBackend() == QPrinterBackend || 
> m_forceRaster->isChecked());
> +   m_scaleMode->setVisible ( isVisible );
> +   m_scaleTo->setVisible ( isVisible );
> +   if ( isVisible ) {
> +   m_printBackendLayout->labelForField(m_scaleMode)->show();
> +   m_printBackendLayout->labelForField(m_scaleTo)->show();
> +   } else {

In general it's preferred to only "disable" options which are unavailable or do 
not apply in a given context instead of hiding them, e.g. with `setEnabled(…)`. 
This should result in less confusion for users.

Apart from that, nothing to complain about your approach ;)

> michaelweghorn wrote in generator_pdf.cpp:151
> Should this be `m_scaleMode->insertItem(FitToPage, i18n("Fit to page"), 
> FitToPage);` (and likewise for all other calls to `insertItem` below? 
> Otherwise a `QVariant()` is inserted.

Indeed, compared to a similar change in D7949 
 the third parameter (i.e. where the magic 
happens) is missing.

I'm not sure using `enum class` would gain us much, because then for the first 
parameter we'd have to provide `int index` ourselves (e.g. `0, 1, 2`) and keep 
track of duplicate numbers. However, this would not yet fix the issue of 
decoupling the implementation of `scaleMode()` from the actual position in the 
combobox. For that, setting `userData` in the third parameter is still required.

> generator_pdf.cpp:154
> +   m_scaleMode->insertItem(None, i18n("None"));
> +   m_printBackendLayout->addRow(i18n("Scale mode"), m_scaleMode);
> +

I'd suggest to append a colon after each label (if it belongs to another item 
like a combobox), just like you did for Print backend:.

> generator_pdf.cpp:159
> +   m_scaleTo->insertItem(FullPage, i18n("Full page"));
> +   m_printBackendLayout->addRow(i18n("Scale to"), m_scaleTo);
> +

Another missing colon…

> generator_pdf.cpp:165-170
> +   // Show scaleMode and scaleTo only if experimental QPrinter 
> backend is selected
> +   // or if the file is to be rasterized before printing
> +   m_scaleMode->hide();
> +   m_scaleTo->hide();
> +   m_printBackendLayout->labelForField(m_scaleMode)->hide();
> +   m_printBackendLayout->labelForField(m_scaleTo)->hide();

I'd use `setEnabled(false)`, see other comment.

> rkflx wrote in generator_pdf.cpp:1217
> Missing space: `g,v`.

Not really important, but this is marked as "done" even though  the space after 
the comma is still missing:

  horizontalScaling,verticalScaling

REPOSITORY
  R223 Okular

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

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

D7962: Implement several new print scaling options

[Michael Weghorn]

michaelweghorn added a comment.


  While testing, the options did not seem to have any effect for me;  changing 
them did not affect the printout.
  
  As far as I can see, the issue is in the calls to `QComboBox::insertItem`, 
e.g. the one I added an inline note to. When not giving a third parameter, a 
`QVariant()` is inserted, which is then probably always casted to '0', i.e. the 
first value of the `ScaleMode` and `ScaleTo` enums.
  (I quickly changed one occurence for the scale mode and that seemed to work.)

INLINE COMMENTS

> generator_pdf.cpp:151
> +   m_scaleMode = new QComboBox;
> +   m_scaleMode->insertItem(FitToPage, i18n("Fit to page"));
> +   m_scaleMode->insertItem(ShrinkToPage, i18n("Shrink to page"));

Should this be `m_scaleMode->insertItem(FitToPage, i18n("Fit to page"), 
FitToPage);` (and likewise for all other calls to `insertItem` below? Otherwise 
a `QVariant()` is inserted.

REPOSITORY
  R223 Okular

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

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

D7962: Implement several new print scaling options

[Oliver Sander]

sander updated this revision to Diff 38128.
sander added a comment.
Restricted Application added a subscriber: okular-devel.


  Implemented all suggestions.  In particular, the new controls disappear now 
unless the experimental QPrinter option or 'force rasterization' is set.  The 
code for this is not particularly elegant, and I'd be interested to hear from 
the Qt pros about how it should really be done.
  
  I did not change the actual option names, because I still don't see a 
concensus on these names anywhere.  I still like my names, but I don't object 
to renaming in principle.

REPOSITORY
  R223 Okular

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D7962?vs=19846=38128

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

AFFECTED FILES
  generators/poppler/generator_pdf.cpp

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

D7962: Implement several new print scaling options

[Albert Astals Cid]

aacid requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R223 Okular

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

To: sander, #okular, aacid
Cc: cfeck, rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander added a comment.


  Hi guys, thanks for all the feedback.  I'll look into it, but I'll be busy 
with other things during the next 10 days.
  
  Nate, the support is "preliminary" in a certain sense.  You get and option to 
print with all the fancy scaling.  The scaling works, but you buy all the 
deficiencies of the poppler "Arthur" backend.  I have submitted various patches 
to this backend recently, so to test this you really really need a 
bleeding-edge version (read: git master) of poppler.

REPOSITORY
  R223 Okular

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

To: sander, #okular
Cc: rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Nathaniel Graham]

ngraham added a comment.


  Does this resolve https://bugs.kde.org/show_bug.cgi?id=348172, or does it 
only implement preliminary support for these new options?

REPOSITORY
  R223 Okular

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

To: sander, #okular
Cc: rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Henrik Fehlauer]

rkflx added a comment.


  > Summary
  
  I'm not able to test real printing at the moment. Could you add a "Test Plan" 
to show what simulated/real testing you did on your side, at least?
  
  > 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`
  
  Optional improvement, not a blocker: The dialog would look nicer if all combo 
boxes had the same width. Maybe look at other code snippets to see how it's 
done there?
  
  > The patch requires https://phabricator.kde.org/D7949.
  
  You could use "Depends on https://phabricator.kde.org/D7949; in the summary, 
this way `arc patch` would automatically download both Diffs for the reviewer 
and Phab would add the info to the "Stack" tab.

INLINE COMMENTS

> generator_pdf.cpp:115
> +   m_scaleMode = new QComboBox;
> +   m_scaleMode->insertItem(0, "Fit to page");
> +   m_scaleMode->insertItem(1, "Shrink to page");

Use the `enum` like in the other Diff.

> generator_pdf.cpp:170
> +   {
> +   if (m_scaleMode->currentIndex()==0)
> +   {

Use the `return` like in the other Diff.

> generator_pdf.cpp:1217
> +// We use the smaller of the two for both directions, to 
> keep the aspect ratio
> +scaling = std::min(horizontalScaling,verticalScaling);
> +}

Missing space: `g,v`.

REPOSITORY
  R223 Okular

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

To: sander, #okular
Cc: rkflx, michaelweghorn, ngraham, aacid

D7962: Implement several new print scaling options

[Albert Astals Cid]

aacid added a comment.


  If this is only useful with the experimental backend you should only make the 
combos visible when the experimental backend is selected, or you'll immediately 
have lots of bug reports about how printing sucks and the okular developers 
should kill themselves for being useless.

INLINE COMMENTS

> generator_pdf.cpp:115
> +   m_scaleMode = new QComboBox;
> +   m_scaleMode->insertItem(0, "Fit to page");
> +   m_scaleMode->insertItem(1, "Shrink to page");

i18n everywhere

REPOSITORY
  R223 Okular

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

To: sander, #okular
Cc: aacid

D7962: Implement several new print scaling options

[Oliver Sander]

sander created this revision.
sander added a reviewer: Okular.
Restricted Application added a project: Okular.

REVISION SUMMARY
  This patch introduces two new printing options:
  
  - Scale mode: fit-to-page vs. shrink-to-page vs. none
  - Scale to: printable area vs. full page
  
  The patch requires https://phabricator.kde.org/D7949 . The new options only 
work with the QPrinter backend.

REPOSITORY
  R223 Okular

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

AFFECTED FILES
  generators/poppler/generator_pdf.cpp

To: sander, #okular
Cc: aacid