D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

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,

D7962: Implement several new print scaling options

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

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.

D7962: Implement several new print scaling options

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

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

D7962: Implement several new print scaling options

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,

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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,

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

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 (

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

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

D7962: Implement several new print scaling options

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,

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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

D7962: Implement several new print scaling options

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