<table><tr><td style="">michaelweghorn 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>In general, I really like the idea of providing this new option.</p>

<p>Two notes after having a quick look (besides the two comments directly on the code):</p>

<ul class="remarkup-list">
<li class="remarkup-list-item">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.)</li>
<li class="remarkup-list-item">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.)</li>
</ul>

<p>(I did not find the time to actually test or look at the change more intensely yet.)</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-73788">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:99</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 style="color: #74777d">// Windows can only print with the QPrinter backend, because the CUPS backend</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Should "this" be passed in the constructor to have this auto-destroyed and avoid a memory leak?</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-73787">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:1349</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 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">forceRasterize</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;">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.</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">https://phabricator.kde.org/D7949</a></div></div><br /><div><strong>To: </strong>sander, Okular, rkflx<br /><strong>Cc: </strong>okular-devel, asturmlechner, cfeck, ltoscano, rkflx, michaelweghorn, ngraham, aacid<br /></div>