michaelweghorn added a comment.
In general, I really like the idea of providing this new option.
Two notes after having a quick look (besides the two comments directly on the
code):
- Depending on what the use case for the option is (Will users who want to
use this experimental feature have it enabled "all of the time" or is it
actually an option that is meant to be changed per print job?), it might
possibly be an alternative to move it to the config options of the PDF backend
(which are available at "Options" -> "Configure backends" -> "PDF"), which
would even further "hide" it from users who don't know about it. (Just to
mention it, I don't have a strong opinion on this at the moment.)
- As far as I can see, this also changes how the "Force rasterization"
behaves for the CUPS case, since it now actually no longer uses QPrinter for
this case now. This is possibly intended. (Just to mention that this will
change the current behaviour. here also when the new experimental option is not
selected.)
(I did not find the time to actually test or look at the change more
intensely yet.)
INLINE COMMENTS
> generator_pdf.cpp:99
> +
> + m_printBackend = new QComboBox;
> + // Windows can only print with the QPrinter backend, because the
> CUPS backend
Should "this" be passed in the constructor to have this auto-destroyed and
avoid a memory leak?
> generator_pdf.cpp:1349
> {
> + if ( forceRasterize )
> + {
The indentation here is a little odd in my eyes, since the "if" is indented
further than the following lines inside of the "if" block. I think moving the
"if" (and "else" below) one indentation level to the "left" and the block one
indentation level to the "right" would be the usual way to do it.
REPOSITORY
R223 Okular
REVISION DETAIL
https://phabricator.kde.org/D7949
To: sander, #okular, rkflx
Cc: okular-devel, asturmlechner, cfeck, ltoscano, rkflx, michaelweghorn,
ngraham, aacid
