D7949: Allow to print pdf doc directly into a QPrinter

[Henrik Fehlauer]

rkflx added a comment.


  Great work, Oliver. If you want to spend your time on Arthur and make users happy that way, I'm all for it.
  
  Idea: Add a task (or multiple tasks) to the Okular workboard to track all printing and/or Arthur related work (otherwise it's difficult for reviewers to get the full picture, with information scattered in Bugzilla, Reviewboard and Phabricator).
  
  > Summary
  
  Please add a test plan for future Diffs, it took me ages to find where I could activate this in the GUI.
  
  > I tagged this option as 'experimental' in the GUI.
  
  Having it only in the tooltip is definitely not enough, this should be directly in the combobox. The tooltip could contain additional information (Which features are missing? What rendering problems are expected? Are bug reports / contributions encouraged?). Optionally add a longer "What's this?" help.
  
  > improving Arthur is easy: you have seen me do it.
  
  When testing this Diff for the first time, I wanted to give it an "-1" because the text rendering was so bad. Then I discovered your patch to Poppler, which brought massive improvements. Therefore, I would highly recommend to only enable Arthur if Poppler is recent enough (add an `#ifdef`).
  
  > what specific issues do you want to see fixed in Arthur
  
  I took https://arxiv.org/pdf/1708.01455.pdf and printed to a "PDF file" (can be observed in an Arthur based Okular, too):
  
  - page 20: bad smoothing of scaled raster image
  - page 21: parts of vector (?) illustration missing
  - large filesize (5x), because of letters being vectors instead of fonts (might lead to problems regarding the printer's cpu/memory resources?)
  
  Those are not blockers for this patch, just some issues I found with limited testing already.

INLINE COMMENTS

> generator_pdf.cpp:80
> +       };
> +
>         PDFOptionsPage()

Add `Q_ENUM(PrintBackend)`…

> generator_pdf.cpp:95
> +           m_printBackend = new QComboBox;
> +           m_printBackend->insertItem(0, "CUPS");
> +           m_printBackend->setItemData(0, "Print directly with CUPS (UNIX only)", Qt::ToolTipRole);

`m_printBackend->insertItem(PrintBackend::CUPSBackend, "CUPS");`. ← Do it like this everywhere you currently have `0`. This way you don't risk mixing up magic numbers all over the code in the future.

> generator_pdf.cpp:96
> +           m_printBackend->insertItem(0, "CUPS");
> +           m_printBackend->setItemData(0, "Print directly with CUPS (UNIX only)", Qt::ToolTipRole);
> +           m_printBackend->insertItem(1, "QPrinter");

Remove `UNIX only` and only show the option on Unix in the first place. However, keep the combobox on Windows even if it only has the QPrinter option (clear communication and useful for users switching between Windows and Linux to not get confused debugging printing issues).

> generator_pdf.cpp:101
> +           QFormLayout *printBackendLayout = new QFormLayout(formWidget);
> +           printBackendLayout->addRow("Print backend", m_printBackend);
> +           layout->addWidget(formWidget);

`i18n` everywhere

> generator_pdf.cpp:109
>  #endif
> +#if defined(Q_OS_WIN)
> +           m_printBackend->setCurrentIndex(1);

Remove all three lines. Could always be brought back if there were actually multiple options on Windows.

> generator_pdf.cpp:138
> +       {
> +           if (m_printBackend->currentIndex()==0)
> +           {

…then you can just do `return m_printBackend->currentData().value<PrintBackend>();`

> generator_pdf.cpp:1116
> +    // is UNIX-specific.
> +    printBackend = PDFOptionsPage::QPrinterBackend;
>  #endif

As you depend on `pdfOptionsPage` where you already have this check anyway, this could go. Too easy to miss that you always have to change both places. The comment is nice though, just move it to the right place.

> generator_pdf.cpp:1162
> +
> +                    // Switch back to old render backend
> +                    pdfdoc->setRenderBackend( renderBackend );

Just rename your variable to `oldRenderBackend`, then the initial comment should suffice and this one could go.

REPOSITORY
  R223 Okular

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

To: sander, #okular
Cc: rkflx, michaelweghorn, ngraham, aacid
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/okular-devel/attachments/20170928/416ac58d/attachment.html>