[FileCopyJob] Clean up after file copy operation is cancelled
Needs ReviewPublic

Authored by chinmoyr on Sun, Feb 10, 5:43 PM.

Details

Reviewers
dfaure
dmitrio
Summary

This patch borrows code from D10663.

The destination file is deleted if a copy job is in progress and
delete job is yet to start (in case of move).

One additional change I made is to exclude put operation as there's
a possibility that someone might try to resume the partial copy at a
later date.

Test Plan

Unit testing and manual testing done.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D10663
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 8144
Build 8162: arc lint + arc unit
chinmoyr created this revision.Sun, Feb 10, 5:43 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSun, Feb 10, 5:43 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Sun, Feb 10, 5:43 PM
dfaure added inline comments.Sun, Feb 10, 7:42 PM
autotests/jobtest.cpp
2019

no space after connect

2022

I'm curious, why this signal, just to call another lambda? Can't you move the code of the second lambda into this one? It's not like the signal is queued right now, it's a direct call.

2033

Add f.remove() here, to avoid leaving a 10MB file around...

(The alternative would be to use QTemporaryFile)

src/core/filecopyjob.cpp
478

Why the "probably"? ;-)

576

Why "not suspended"? If we suspend and kill, don't we want the cleanup?

577

This if makes little sense as is, since d->m_delJob can only be set when d->m_copyJob is nullptr (see line 515). So it's redundant, and the same as writing if (d->m_copyJob) -- and it looks less scary, I was initially worried when reading your line about what would happen before m_delJob was set or after m_delJob was null again.

578

A test for isLocalFile() is missing first.

Woohoo! Please add:

BUG: 125102
FIXED-IN: 5.56

To the Summary section. :)

Hmm actually maybe I take it back... is this in fact intended to fix 125102? Because the destination file is still destroyed when an overwrite operation is canceled in the middle. That's not what we want; we want the destination file to be left untouched when the user intentionally cancels an overwrite. This will probably require moving/copying to a temp file and replacing the destination at the end, once the operation has completed--if there's enough space on disk for this of course.

@ngraham The destination file is corrupted as soon as the user clicks overwrite. This patch deletes that file. Is that file supposed to be preserved (to prevent user panic)? My suggestion is to append ".part" or some similar extension when an overwrite starts, so the user knows that there data is gone.

chinmoyr updated this revision to Diff 51383.Mon, Feb 11, 2:17 AM
chinmoyr marked 4 inline comments as done.

Fixed all issues.
Added test case for suspend.

so the user knows that there data is gone

*their

@ngraham The destination file is corrupted as soon as the user clicks overwrite.

That's exactly the problem we need to fix.

For example, if the user starts an overwrite by accident and then cancels it, their original file in the destination has been destroyed. This is bad. Canceling an accidental action should never be destructive.

That's why I'm recommending that for overwrite operations, we don't instantly delete or start overwriting the destination file, but rather move or copy the source file to a separate location, and then overwrite the destination file. This preserves the user's ability to cancel the job during the move or copy portion of the operation without destroying their own files.

Your reasoning seems to forget that there is a first confirmation, in case of overwriting, in the form of the "Overwrite/Skip/Cancel" dialog?

Otherwise I kind of like the idea, but there's the problem of overwriting a 4GB file on a 5GB partition... (no space for 4*2 GB temporarily).

Yes, obviously this won't work when disk space is limited and we'll need to check for that. In this case, I would like the the confirmation dialog to actually say to the user that the overwrite operation cannot be canceled without destroying the target file.