<table><tr><td style="">dmitrio marked an inline comment as done.<br />dmitrio 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/D10663" rel="noreferrer">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/D10663#209708" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10663#209708</a>, <a href="https://phabricator.kde.org/p/meven/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@meven</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Add this to your commit message<br />
FIXED-IN: 5.44</p></div>
</blockquote>
<p>Thank you, added it to the description.</p>
<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/D10663#209711" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10663#209711</a>, <a href="https://phabricator.kde.org/p/anthonyfieroni/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@anthonyfieroni</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>It looks <a href="https://phabricator.kde.org/D10635" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: line-through;" rel="noreferrer">D10635</a> is duplicate.</p></div>
</blockquote>
<p>In fact, this revision is a duplicate of <a href="https://phabricator.kde.org/D10635" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: line-through;" rel="noreferrer">D10635</a>. I am a bit surprised why this one has a smaller number :)</p>
<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/D10663#209720" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10663#209720</a>, <a href="https://phabricator.kde.org/p/meven/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@meven</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>It was suggested on IRC to hide this behavior behind a flag such as the jobFlag enum, so that it can be opt-in/opt-out in applications.</p></div>
</blockquote>
<p>I agree with this proposal, but I can suggest that the added flag should rather turn off the new behavior, so the default behavior will be to do this cleanup on job cancel.<br />
However, if more people think that it should not be turned on by default, it would be easy to change this diff to achieve an opposite behavior.</p>
<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/D10663#209720" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;" rel="noreferrer">D10663#209720</a>, <a href="https://phabricator.kde.org/p/meven/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;" rel="noreferrer">@meven</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>We could consider changing the job state during the clean up, something like</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);">d->state == STATE_CLEANING_INCOMPLETE_FILE</pre></div></div>
</blockquote>
<p>For now, I see no point in this change, since, to avoid breaking a logic of doKill() method, the deletion job is currently launched not as subjob, but as a separate job. So, after doKill() execution the CopyJob does nothing anymore (as it is supposed to be) and has no need in tracking its state. Feel free to correct me if I am wrong.</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/D10663#inline-50694" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">meven</span> wrote in <span style="color: #4b4d51; font-weight: bold;">copyjob.cpp:559</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">I believe this is because files are copied in sequence, so at any given time only the first file is being copied.</p>
<p style="padding: 0; margin: 8px;">Maybe you could use m_currentSrcURL to avoid accessing the iterator.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">You are right, updated a diff.</p>
<p style="padding: 0; margin: 8px;">On the question on deleting first file only, yes, we need to delete only the file that was being copied before the user canceled the job. Perhaps using m_currentDestURL would make this intention more clear.</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/D10663" rel="noreferrer">https://phabricator.kde.org/D10663</a></div></div><br /><div><strong>To: </strong>dmitrio, Frameworks, dfaure<br /><strong>Cc: </strong>anthonyfieroni, meven, Frameworks, michaelh<br /></div>