| aacid | |
| ngraham |
| Okular | |
| VDG |
This patch introduces three scaling option for printing
The new options only work when rasterization is enabled. This has technical reasons: (This implementation of ) scaling needs printing through a QPrinter object, and only rasterized printing does that.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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.
| generators/poppler/generator_pdf.cpp | ||
|---|---|---|
| 115 | i18n everywhere | |
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):
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 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.
| generators/poppler/generator_pdf.cpp | ||
|---|---|---|
| 115 | Use the enum like in the other Diff. | |
| 170 | Use the return like in the other Diff. | |
| 1217 | Missing space: g,v. | |
Does this resolve https://bugs.kde.org/show_bug.cgi?id=348172, or does it only implement preliminary support for these new options?
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.
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.
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.)
| generators/poppler/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. | |
| generators/poppler/generator_pdf.cpp | ||
|---|---|---|
| 151 | Or maybe using sth else as index might be a good option as well. I did not test, but using enum class instead of enum might have triggered a compile error right away. | |
| generators/poppler/generator_pdf.cpp | ||
|---|---|---|
| 80–89 | 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 ;) | |
| 151 | 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. | |
| 154 | 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:. | |
| 159 | Another missing colon… | |
| 165–170 | I'd use setEnabled(false), see other comment. | |
| 1217 | Not really important, but this is marked as "done" even though the space after the comma is still missing: horizontalScaling,verticalScaling | |
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?
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 :) ).
I updated the diff. Apologies for the last version---it was really messed up.
The new version brings a few changes:
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:
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.
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.
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?
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!
ScaleMode:
ScaleTo: What is the actual target area that we are printing 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.
Ah, now I get it. One more question: is the following accurate?
This new version of the diff adds tooltips to the new combo boxes. When the boxes are enabled, the tooltips hint at what they are good for. When the boxes are disabled, the tooltips tell how to enable them.
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 areaWhy? 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:
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.
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?
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.
The UI looks good now and all three options work great with my printer and a few assorted documents!