<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" rel="noreferrer">View Revision</a></tr></table><br /><div><div><p>Great work, Oliver. If you want to spend your time on Arthur and make users happy that way, I'm all for it.</p>
<p>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).</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>Summary</p></blockquote>
<p>Please add a test plan for future Diffs, it took me ages to find where I could activate this in the GUI.</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>I tagged this option as 'experimental' in the GUI.</p></blockquote>
<p>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.</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>improving Arthur is easy: you have seen me do it.</p></blockquote>
<p>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 <tt style="background: #ebebeb; font-size: 13px;">#ifdef</tt>).</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>what specific issues do you want to see fixed in Arthur</p></blockquote>
<p>I took <a href="https://arxiv.org/pdf/1708.01455.pdf" class="remarkup-link" target="_blank" rel="noreferrer">https://arxiv.org/pdf/1708.01455.pdf</a> and printed to a "PDF file" (can be observed in an Arthur based Okular, too):</p>
<ul class="remarkup-list">
<li class="remarkup-list-item">page 20: bad smoothing of scaled raster image</li>
<li class="remarkup-list-item">page 21: parts of vector (?) illustration missing</li>
<li class="remarkup-list-item">large filesize (5x), because of letters being vectors instead of fonts (might lead to problems regarding the printer's cpu/memory resources?)</li>
</ul>
<p>Those are not blockers for this patch, just some issues I found with limited testing already.</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-33446" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:80</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="p">};</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">
</div><div style="padding: 0 8px; margin: 0 4px; "> <span class="n">PDFOptionsPage</span><span class="p">()</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Add <tt style="background: #ebebeb; font-size: 13px;">Q_ENUM(PrintBackend)</tt>…</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-33447" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:95</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 style="color: #aa4000">new</span> <span class="n">QComboBox</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">insertItem</span><span class="p">(</span><span style="color: #601200">0</span><span class="p">,</span> <span style="color: #766510">"CUPS"</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 style="color: #601200">0</span><span class="p">,</span> <span style="color: #766510">"Print directly with CUPS (UNIX only)"</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></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">m_printBackend->insertItem(PrintBackend::CUPSBackend, "CUPS");</tt>. ← Do it like this everywhere you currently have <tt style="background: #ebebeb; font-size: 13px;">0</tt>. This way you don't risk mixing up magic numbers all over the code in the future.</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-33450" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:96</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">insertItem</span><span class="p">(</span><span style="color: #601200">0</span><span class="p">,</span> <span style="color: #766510">"CUPS"</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 style="color: #601200">0</span><span class="p">,</span> <span style="color: #766510">"Print directly with CUPS (UNIX only)"</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">insertItem</span><span class="p">(</span><span style="color: #601200">1</span><span class="p">,</span> <span style="color: #766510">"QPrinter"</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Remove <tt style="background: #ebebeb; font-size: 13px;">UNIX only</tt> 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).</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-33453" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:101</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 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;"><tt style="background: #ebebeb; font-size: 13px;">i18n</tt> everywhere</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-33449" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:109</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; "><span style="color: #304a96">#endif</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">#if defined(Q_OS_WIN)</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">setCurrentIndex</span><span class="p">(</span><span style="color: #601200">1</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Remove all three lines. Could always be brought back if there were actually multiple options on Windows.</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-33448" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:138</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="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">if</span> <span class="p">(</span><span class="n">m_printBackend</span><span style="color: #aa2211">-></span><span class="n">currentIndex</span><span class="p">()</span><span style="color: #aa2211">==</span><span style="color: #601200">0</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="p">{</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">…then you can just do <tt style="background: #ebebeb; font-size: 13px;">return m_printBackend->currentData().value<PrintBackend>();</tt></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-33451" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:1116</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 style="color: #74777d">// <span class="bright">currently the only way Okular implements printing without using </span>UNIX-specific</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="bright"></span><span style="color: #74777d"><span class="bright">// tools like 'lpr'.</span></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(151, 234, 151, .6);"> <span style="color: #74777d">// <span class="bright">is </span>UNIX-specific<span class="bright">.</span></span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <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 style="color: #304a96">#endif</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">As you depend on <tt style="background: #ebebeb; font-size: 13px;">pdfOptionsPage</tt> 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.</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-33452" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:1162</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 style="color: #74777d">// Switch back to old render backend</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">pdfdoc</span><span style="color: #aa2211">-></span><span class="n">setRenderBackend</span><span class="p">(</span> <span class="n">renderBackend</span> <span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Just rename your variable to <tt style="background: #ebebeb; font-size: 13px;">oldRenderBackend</tt>, then the initial comment should suffice and this one could go.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R223 Okular</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D7949" rel="noreferrer">https://phabricator.kde.org/D7949</a></div></div><br /><div><strong>To: </strong>sander, Okular<br /><strong>Cc: </strong>rkflx, michaelweghorn, ngraham, aacid<br /></div>