Delete the destination file if the copy job was canceled. In case Overwrite flag
was provided to the job, delete the .part file.
Details
Unit testing and manual testing done.
Diff Detail
- Repository
- R241 KIO
- Branch
- arcpatch-D18904_1
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 9246 Build 9264: arc lint + arc unit
autotests/jobtest.cpp | ||
---|---|---|
2066 | no space after connect | |
2069 | 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. | |
2080 | 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. |
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.
Fixed all issues.
Added test case for suspend.
so the user knows that there data is gone
*their
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.
@ngraham I'm not convinced. Warning users that an operation can't be undone makes sense, but canceling in the middle of the operation is not usually something to warn about. It's nice to handle it better, but I wouldn't bother the user with "you know, if you cancel, you'll be in trouble". Well, yes, I'm OVERWRITING a file. If I'm too slow to cancel, I'll have the same trouble too.
Anyhow, that's for the GUI side of things, this commit seems fine in itself.
OK. I'm open to being convinced out of my crazy idea. :)
In any event, this is good. Thanks for your work on this @chinmoyr!
Delete .part file when Overwrite flag is set.
@dfaure please have a look at it again.
Withdrawing my approval, after realizing that this is missing the "enough free space at destination" check.
Imagine a 5GB partition with a 4GB file on it. You want to copy another version of that 4GB file onto it. Old implementation: it works. New implementation: we start copying 1GB into a .part file, then it fails (out of space), and then IIUC we finally do the right thing and overwrite the file. Right? So it ends up working, but it's much slower than it should behave. This could be improved with a free space check, right?
src/core/filecopyjob.cpp | ||
---|---|---|
479 | "file write" sounds like putJob, which isn't what this is about. I guess this should be called d->m_bFileCopyInProgress instead. |
There's free space check in CopyJob. And Dolphin still gives an error in the said situation. But if some application decides to directly use FileCopyJob then IMO the free space check should be at application's side. Anyway, I tried adding it here and it felt awkward to me.
Ah, the free space check in CopyJob doesn't take into account that it's an overwrite? Heh, two bugs canceling each other, in a way ;-)
In the situation I describe, there *is* space to do the copying, just not with a temporary .part file.
So I didn't expect an error in that case, but rather directly overwriting without using .part.
But OK that's a separate issue then, the check itself could be improved.