<table><tr><td style="">dfaure requested changes to this revision.<br />dfaure 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/D19080">View Revision</a></tr></table><br /><div><div><p>Thanks for working on this, here's my review.</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/D19080#inline-106194">View Inline</a><span style="color: #4b4d51; font-weight: bold;">jobtest.cpp:1724</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: #aa4000">void</span> <span class="n">JobTest</span><span style="color: #aa2211">::</span><span class="n">safeOverwrite</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;">This test is called Overwrite, and passes the Overwrite flag, but it's not actually overwriting anything (destFile doesn't exist).</p>
<p style="padding: 0; margin: 8px;">I think both cases should be tested --> make a _data() method with two rows, destFileExists=false and destFileExists=true.<br />
Apart from creating destFile is that bool is true, I think all the code of the test remains the same.</p>
<p style="padding: 0; margin: 8px;">Another issue: the whole test needs to be skipped on Windows (use QSKIP in #ifdef), since you only implemented it in file_unix.cpp</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/D19080#inline-106196">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:288</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: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">!</span><span class="n">_dest_backup</span><span class="p">.</span><span class="n">isEmpty</span><span class="p">()</span> <span style="color: #aa2211">&&</span> <span style="color: #aa2211">!</span><span class="n">orig_delete_attempted</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span style="color: #aa4000">int</span> <span class="n">btnCode</span> <span style="color: #aa2211">=</span> <span class="n">messageBox</span><span class="p">(</span><span class="n">SlaveBase</span><span style="color: #aa2211">::</span><span class="n">WarningContinueCancel</span><span class="p">,</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">i18n</span><span class="p">(</span><span style="color: #766510">"Proceeding further will destroy the destination file. Do you want to continue?"</span><span class="p">));</span> <span style="color: #74777d">//TODO better message</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">This is all in the context of KIO::Overwrite being set, programatically. So the whole point *is* to overwrite without asking (this is often used after the user has already approved overwriting, or for cases where the user should not be involved like application-internal files).</p>
<p style="padding: 0; margin: 8px;">So it makes zero sense to me to ask the user about overwriting yet again. In fact the thing you're asking here is "do you agree that canceling before completion will no longer preserve the existing destination file, as it would do otherwise". In my opinion, this is too much detail, we shouldn't be asking a question "just in case the user wants to cancel". In all the cases where the user isn't going to cancel, this is really just an annoyance.</p>
<p style="padding: 0; margin: 8px;">I view the feature (no data loss during cancel-of-overwrite) as a nice bonus, but it's not a *bug* if an overwritten file is lost during an overwrite operation, by definition.</p>
<p style="padding: 0; margin: 8px;">So... I would suggest to proceed without asking.</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/D19080#inline-106197">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:422</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: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">::</span><span class="n">unlink</span><span class="p">(</span><span class="n">_dest_backup</span><span class="p">.</span><span class="n">data</span><span class="p">())</span> <span style="color: #aa2211">==</span> <span style="color: #aa2211">-</span><span style="color: #601200">1</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">qCWarning</span><span class="p">(</span><span class="n">KIO_FILE</span><span class="p">)</span> <span style="color: #aa2211"><<</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"Couldn't remove original dest '%1'"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">QString</span><span style="color: #aa2211">::</span><span class="n">fromLocal8Bit</span><span class="p">(</span><span class="n">_dest_backup</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;">qCWarning supports <<, no need for arg(). <br />
The use of a QString will add double quotes automatically, so having double quotes around the whole message will look strange.<br />
Just do the usual</p>
<div class="remarkup-code-block" style="margin: 12px 0;" data-code-lang="text" data-sigil="remarkup-code-block"><pre class="remarkup-code" style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; padding: 12px; margin: 0; background: rgba(71, 87, 120, 0.08);">qCWarning() << "Couldn't ..." << _dest_backup;</pre></div></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/D19080#inline-106198">View Inline</a><span style="color: #4b4d51; font-weight: bold;">file_unix.cpp:426</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: #aa4000">if</span> <span class="p">(</span><span style="color: #aa2211">::</span><span class="n">rename</span><span class="p">(</span><span class="n">_dest</span><span class="p">.</span><span class="n">data</span><span class="p">(),</span> <span class="n">_dest_backup</span><span class="p">.</span><span class="n">data</span><span class="p">())</span> <span style="color: #aa2211">==</span> <span style="color: #aa2211">-</span><span style="color: #601200">1</span><span class="p">)</span> <span class="p">{</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"> <span class="n">qCWarning</span><span class="p">(</span><span class="n">KIO_FILE</span><span class="p">)</span> <span style="color: #aa2211"><<</span> <span class="n">QStringLiteral</span><span class="p">(</span><span style="color: #766510">"Couldn't rename '%1' to '%2'"</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">dest</span><span class="p">).</span><span class="n">arg</span><span class="p">(</span><span class="n">QString</span><span style="color: #aa2211">::</span><span class="n">fromLocal8Bit</span><span class="p">(</span><span class="n">_dest_backup</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;">(same)</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/D19080">https://phabricator.kde.org/D19080</a></div></div><br /><div><strong>To: </strong>chinmoyr, dfaure, ngraham<br /><strong>Cc: </strong>kde-frameworks-devel, michaelh, ngraham, bruns<br /></div>