<table><tr><td style="">rkflx requested changes to this revision.<br />rkflx added a comment.<br />This revision now requires changes to proceed.
</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/D12328">View Revision</a></tr></table><br /><div><div><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/D12328#252246" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D12328#252246</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 #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/D12328#250028" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D12328#250028</a>, <a href="https://phabricator.kde.org/p/anemeth/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@anemeth</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Rebase on <a href="https://phabricator.kde.org/D12321" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D12321</a></p></div>
</blockquote>
<p>While the dependency tree is now looking good, the rebase did not work out correctly. While it might look correct on your local branch, on Phabricator you can clearly see that in this Diff the changes from the parent revision are visible, which should not be there. Please make sure to always check what ends up on Phabricator, as that's what your reviewers are seeing.</p>
<p><tt style="background: #ebebeb; font-size: 13px;">arc diff</tt> will upload the commit range between <tt style="background: #ebebeb; font-size: 13px;">HEAD</tt> and the upstream tracking branch. Here the upstream branch apparently is <tt style="background: #ebebeb; font-size: 13px;">master</tt>, while it should be your local branch where <a href="https://phabricator.kde.org/D12321" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D12321</a> is living (you have a separate branch per Diff as written in the wiki, right?). You may want to look at my advice in <a href="https://phabricator.kde.org/D12321#249646" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D12321#249646</a> again.</p></div>
</blockquote>
<p><a href="https://phabricator.kde.org/p/anemeth/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@anemeth</a> Ping. I won't be able to help you with <tt style="background: #ebebeb; font-size: 13px;">kconf_update</tt> or review the patch unless Phab shows me the proper patch!</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/D12328#inline-63773">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kdiroperator.cpp:2165</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: #aa4000">if</span> <span class="p">(</span><span class="n">d</span><span style="color: #aa2211">-></span><span class="n">inlinePreviewState</span> <span style="color: #aa2211">==</span> <span class="n">Private</span><span style="color: #aa2211">::</span><span class="n">NotForced</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(251, 175, 175, .7);"> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">showPreviews</span> <span style="color: #aa2211">=</span> <span class="n">configGroup</span><span class="p">.</span><span class="n">readEntry</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"Previews"</span><span class="p">),</span> <span class="bright"></span><span style="color: #304a96"><span class="bright">fals</span>e</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">showPreviews</span> <span style="color: #aa2211">=</span> <span class="n">configGroup</span><span class="p">.</span><span class="n">readEntry</span><span class="p">(</span><span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"Preview<span class="bright"> Thumbnail</span>s"</span><span class="p">),</span> <span class="bright"></span><span style="color: #304a96"><span class="bright">tru</span>e</span><span class="p">);</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">showPreviewsEnabledBeforeZoom</span> <span style="color: #aa2211">=</span> <span class="n">d</span><span style="color: #aa2211">-></span><span class="n">showPreviews</span><span class="p">;</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">There's still some confusion in the wording wrt. <tt style="background: #ebebeb; font-size: 13px;">Show Preview</tt>.</p>
<p style="padding: 0; margin: 8px;">How about naming your setting <tt style="background: #ebebeb; font-size: 13px;">Show Inline Previews</tt> (inspired by <tt style="background: #ebebeb; font-size: 13px;">_k_toggleInlinePreview()</tt>)?</p>
<p style="padding: 0; margin: 8px;">(The other option could be renamed to <tt style="background: #ebebeb; font-size: 13px;">Show Aside Preview</tt> in a followup patch.)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D12328">https://phabricator.kde.org/D12328</a></div></div><br /><div><strong>To: </strong>anemeth, Frameworks, VDG, rkflx<br /><strong>Cc: </strong>abetts, rkflx, ngraham, Frameworks, michaelh, bruns<br /></div>