rkflx added a comment.
Thanks for the update. Still found some minor things, otherwise LGTM. --- > Unlike for CUPS printing, Qt printing prints on the entire page and does not scale to the printable area yet. This is only because it is the easiest way. I plan to implement a few standard scaling methods in a subsequent patch, which will only be a few additional lines of code. Has this been worked on? If not, there should be a hint in the tooltip. > being able to use the Arthur backend in my daily work. I guess you don't print annotations then ;) For me, highlighter annotations are rendered opaque. Not a blocker, of course, but maybe worth to file a bug for. In D7949#155133 <https://phabricator.kde.org/D7949#155133>, @rkflx wrote: > > I implemented a longer "What's this" help, but I couldn't make it appear in the GUI. What's the magic button for that? > > Normally you can right-click or use the question mark tool in the dialog's title bar. However, this does not seem to work with combobox popups (some focus issue, it does work with normal menu popups, though). The easiest solution for this would be to move all of the text to the tooltip, with `\n` for line breaks. If you want to get fancy, you could switch to radio buttons or add a text area next to the combobox with the explanation (but not sure if worth it). There is still no way to get to the longer explanations. I'd suggest: - For the CUPS entry: Use what you currently have for `WhatsThisRole` for the text of `ToolTipRole`. - For the QPrinter entry: Only set the text of `ToolTipRole`, which should be fine as-is. Possibly move the rest of the text to a comment in the code. --- In D7949#285825 <https://phabricator.kde.org/D7949#285825>, @sander wrote: > At this point, I am unlikely to fix further Arthur bugs just for the fun of it. This raises the questions about the future of this diff. I see basically three options: > > - If there is agreement that QPrinter printing is desirable then the patch should be pushed, possibly after further improvements. > - If there is no such agreement we may as well close this diff now. Arthur will not improve further unless there is a real need. > - As a compromise, I could modify the diff to be 'windows only'. I would need a bit of help with testing, though. > > Opinions? IMO the current state is good enough to ship it as a non-default option. As it is quite hidden behind buttons and tabs, I guess it's quite unlikely that there will be many bug reports about problems created through accidental activation of the option. @aacid @michaelweghorn Any further comments? INLINE COMMENTS > generator_pdf.cpp:105 > + m_printBackend->setItemData(CUPSBackend, i18n("Print with CUPS"), > Qt::ToolTipRole); > + m_printBackend->setItemData(CUPSBackend, i18n("Print using the > CUPS printing system. This will convert your PDf file to PostScript first, > and then send the PostScript file to the printer."), Qt::WhatsThisRole); > +#endif Remove extra space before "This", add linebreak with `<nl/>` (needs `xi18n` instead of `i18n`). PDf → PDF > generator_pdf.cpp:118 > + QFormLayout *printBackendLayout = new QFormLayout(formWidget); > + printBackendLayout->addRow(i18n("Print backend"), m_printBackend); > + layout->addWidget(formWidget); Add colon. > generator_pdf.cpp:1320 > > -#ifdef Q_OS_WIN > - // Windows can only print by rasterization, because that is > - // currently the only way Okular implements printing without using > UNIX-specific > - // tools like 'lpr'. > - forceRasterize = true; > -#ifndef HAVE_POPPLER_0_60 > - // The Document::HideAnnotations flags was introduced in poppler 0.60 > - printAnnots = true; > -#endif > -#endif > - > -#ifdef HAVE_POPPLER_0_60 > - if ( forceRasterize ) > - { > - pdfdoc->setRenderHint(Poppler::Document::HideAnnotations, > !printAnnots); > -#else > - if ( forceRasterize && printAnnots) > + if ( printBackend==PDFOptionsPage::QPrinterBackend ) > { Coding style: Add spaces around `==`. REVISION DETAIL https://phabricator.kde.org/D7949 To: sander, #okular Cc: cfeck, ltoscano, rkflx, michaelweghorn, ngraham, aacid
