<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/D7962" rel="noreferrer">View Revision</a></tr></table><br /><div><div><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>I'm not able to test real printing at the moment. Could you add a "Test Plan" to show what simulated/real testing you did on your side, at least?</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>Scale mode: fit-to-page vs. shrink-to-page vs. none<br />
Scale to: printable area vs. full page</p></blockquote>
<p>Having "page" in the names for the scale modes while "Scale to" has "page" in the name half of the time seems like it could confuse some users. "Mode" is a quite technical (and often meaningless) term, too. In addition, it's very hard without looking at the code and with no extra explanation available to know what the actual difference between "fit" and "shrink" is. Ideas (could be improved, though):</p>
<ul class="remarkup-list">
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">Scaling:</tt> <tt style="background: #ebebeb; font-size: 13px;">Shrink oversized pages only</tt> | <tt style="background: #ebebeb; font-size: 13px;">Expand or shrink to fit to page</tt> | <tt style="background: #ebebeb; font-size: 13px;">None</tt> (← Note how the first/default mode is different – I don't think users would expect their printouts getting scaled up by default like in your Diff.)</li>
<li class="remarkup-list-item"><tt style="background: #ebebeb; font-size: 13px;">Scale to:</tt> <tt style="background: #ebebeb; font-size: 13px;">Printable area</tt> | <tt style="background: #ebebeb; font-size: 13px;">Paper size</tt></li>
</ul>
<p>Optional improvement, not a blocker: The dialog would look nicer if all combo boxes had the same width. Maybe look at other code snippets to see how it's done there?</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>The patch requires <a href="https://phabricator.kde.org/D7949" class="remarkup-link" target="_blank" rel="noreferrer">https://phabricator.kde.org/D7949</a>.</p></blockquote>
<p>You could use "Depends on <a href="https://phabricator.kde.org/D7949" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D7949</a>" in the summary, this way <tt style="background: #ebebeb; font-size: 13px;">arc patch</tt> would automatically download both Diffs for the reviewer and Phab would add the info to the "Stack" tab.</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/D7962#inline-33455" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:115</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_scaleMode</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_scaleMode</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">"Fit to page"</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">m_scaleMode</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">"Shrink to page"</span><span class="p">);</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Use the <tt style="background: #ebebeb; font-size: 13px;">enum</tt> like in the other Diff.</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/D7962#inline-33456" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:170</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_scaleMode</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;">Use the <tt style="background: #ebebeb; font-size: 13px;">return</tt> like in the other Diff.</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/D7962#inline-33457" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">generator_pdf.cpp:1217</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">// We use the smaller of the two for both directions, to keep the aspect ratio</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">scaling</span> <span style="color: #aa2211">=</span> <span class="n">std</span><span style="color: #aa2211">::</span><span class="n">min</span><span class="p">(</span><span class="n">horizontalScaling</span><span class="p">,</span><span class="n">verticalScaling</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;">Missing space: <tt style="background: #ebebeb; font-size: 13px;">g,v</tt>.</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/D7962" rel="noreferrer">https://phabricator.kde.org/D7962</a></div></div><br /><div><strong>To: </strong>sander, Okular<br /><strong>Cc: </strong>rkflx, michaelweghorn, ngraham, aacid<br /></div>