<table><tr><td style="">rkflx added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D7949">View Revision</a></tr></table><br /><div><div><p>Thanks for the update. Still found some minor things, otherwise LGTM.</p>
<hr class="remarkup-hr" />
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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.</p></blockquote>
<p>Has this been worked on? If not, there should be a hint in the tooltip.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>being able to use the Arthur backend in my daily work.</p></blockquote>
<p>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.</p>
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D7949#155133" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D7949#155133</a>, <a href="https://phabricator.kde.org/p/rkflx/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@rkflx</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>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?</p></blockquote>
<p>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 <tt style="background: #ebebeb; font-size: 13px;">\n</tt> 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).</p></div>
</blockquote>
<p>There is still no way to get to the longer explanations. I'd suggest:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">For the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">CUPS</span></span></span> entry: Use what you currently have for <tt style="background: #ebebeb; font-size: 13px;">WhatsThisRole</tt> for the text of <tt style="background: #ebebeb; font-size: 13px;">ToolTipRole</tt>.</li>
<li class="remarkup-list-item">For the <span><span class="phui-tag-view phui-tag-type-shade phui-tag-grey phui-tag-shade "><span class="phui-tag-core ">QPrinter</span></span></span> entry: Only set the text of <tt style="background: #ebebeb; font-size: 13px;">ToolTipRole</tt>, which should be fine as-is. Possibly move the rest of the text to a comment in the code.</li>
</ul>
<hr class="remarkup-hr" />
<blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D7949#285825" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D7949#285825</a>, <a href="https://phabricator.kde.org/p/sander/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@sander</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>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:</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">If there is agreement that QPrinter printing is desirable then the patch should be pushed, possibly after further improvements.</li>
<li class="remarkup-list-item">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.</li>
<li class="remarkup-list-item">As a compromise, I could modify the diff to be 'windows only'. I would need a bit of help with testing, though.
<br /><br />
Opinions?</li>
</ul></div>
</blockquote>
<p>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.</p>
<p><a href="https://phabricator.kde.org/p/aacid/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@aacid</a> <a href="https://phabricator.kde.org/p/michaelweghorn/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@michaelweghorn</a> Any further comments?</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7949#inline-73446">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:105</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_printBackend</span><span style="color: #aa2211">-></span><span class="n">setItemData</span><span class="p">(</span><span class="n">CUPSBackend</span><span class="p">,</span> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Print with CUPS"</span><span class="p">),</span> <span class="n">Qt</span><span style="color: #aa2211">::</span><span class="n">ToolTipRole</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_printBackend</span><span style="color: #aa2211">-></span><span class="n">setItemData</span><span class="p">(</span><span class="n">CUPSBackend</span><span class="p">,</span> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Print using the CUPS printing system. This will convert your PDf file to PostScript first, and then send the PostScript file to the printer."</span><span class="p">),</span> <span class="n">Qt</span><span style="color: #aa2211">::</span><span class="n">WhatsThisRole</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#endif</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Remove extra space before "This", add linebreak with <tt style="background: #ebebeb; font-size: 13px;"><nl/></tt> (needs <tt style="background: #ebebeb; font-size: 13px;">xi18n</tt> instead of <tt style="background: #ebebeb; font-size: 13px;">i18n</tt>).</p>
<p style="padding: 0; margin: 8px;">PDf → PDF</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7949#inline-73447">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:118</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">QFormLayout</span> <span style="color: #aa2211">*</span><span class="n">printBackendLayout</span> <span style="color: #aa2211">=</span> <span style="color: #aa4000">new</span> <span class="n">QFormLayout</span><span class="p">(</span><span class="n">formWidget</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">printBackendLayout</span><span style="color: #aa2211">-></span><span class="n">addRow</span><span class="p">(</span><span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Print backend"</span><span class="p">),</span> <span class="n">m_printBackend</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">layout</span><span style="color: #aa2211">-></span><span class="n">addWidget</span><span class="p">(</span><span class="n">formWidget</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Add colon.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D7949#inline-73448">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:1320</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span class="bright"></span><span style="color: #304a96"><span class="bright">#ifdef Q_OS_WIN</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #74777d">// Windows can only print by rasterization, because that is</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #74777d">// currently the only way Okular implements printing without using UNIX-specific</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #74777d">// tools like 'lpr'.</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">forceRasterize</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">true</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#ifndef HAVE_POPPLER_0_60</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #74777d">// The Document::HideAnnotations flags was introduced in poppler 0.60</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">printAnnots</span> <span style="color: #aa2211">=</span> <span style="color: #304a96">true</span><span class="p">;</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#endif</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#endif</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);">
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#ifdef HAVE_POPPLER_0_60</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">if</span> <span class="p">(</span> <span class="n">forceRasterize</span> <span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">pdfdoc</span><span style="color: #aa2211">-></span><span class="n">setRenderHint</span><span class="p">(</span><span class="n">Poppler</span><span style="color: #aa2211">::</span><span class="n">Document</span><span style="color: #aa2211">::</span><span class="n">HideAnnotations</span><span class="p">,</span> <span style="color: #aa2211">!</span><span class="n">printAnnots</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"><span style="color: #304a96">#else</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span style="color: #aa4000">if</span> <span class="p">(</span> <span class="n">forceRasterize</span> <span style="color: #aa2211">&&</span> <span class="n">printAnnots</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span class="bright"> </span><span style="color: #aa4000"><span class="bright">if</span></span><span class="bright"> </span><span class="p"><span class="bright">(</span></span><span class="bright"> </span><span class="n"><span class="bright">printBackend</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">==</span></span><span class="bright"></span><span class="n"><span class="bright">PDFOptionsPage</span></span><span class="bright"></span><span style="color: #aa2211"><span class="bright">::</span></span><span class="bright"></span><span class="n"><span class="bright">QPrinterBackend</span></span><span class="bright"> </span><span class="p"><span class="bright">)</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Coding style: Add spaces around <tt style="background: #ebebeb; font-size: 13px;">==</tt>.</p></div></div></div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7949">https://phabricator.kde.org/D7949</a></div></div><br /><div><strong>To: </strong>sander, Okular<br /><strong>Cc: </strong>cfeck, ltoscano, rkflx, michaelweghorn, ngraham, aacid<br /></div>