Add a DeleteJob for the last file that was being copied/moved when pasting or dropping a file when the copyjob failed because of a user cancelation or because the destination disk was full.
Details
Details
- Reviewers
dfaure
Copy using paste action a file and cancel the action before it finishes, the destination file should be removed
Same operation using a drop "copy" action
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- bug-383764-clean-up-incomplete-files-after-cancelation (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage
Comment Actions
Thanks for the patch! For formatting guidelines, please see https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch
In essence, put "BUG: 383764" on its own line in the Summary section, and remove it from the title.
Also please put "FIXED-IN: KDE Frameworks 5.44" on its own line, since that's the version of KDE Frameworks that this will make it into.
Works great in testing, and I'm excited to get this in. One comment about the code, indicated below, and I'll let the more experienced KDE developers offer a more in-depth code review.
Comment Actions
I like to use the full string since not all of our users or people browsing the bug tracker will be familiar enough with all our products and their versioning conventions to able to map a version number to its product. Is there a technical reason why the field should only have the version number instead of the full string?
Comment Actions
And they should not need to track the product: the version is per component.
So please fix this page https://community.kde.org/Guidelines_and_HOWTOs/Write_a_version_string (or I can do it) to describe the current status and ask for feedback if you want to change it without deciding for everyone. According the current usage, only the version number is used and needed, with the exception sometimes of applications part of KDE Applications, where some authors decide to use "x.y.z (KDE Applications some.thin.g)".
In general using the version number only does not require looking for the version of the product.
Comment Actions
The idea kind of makes sense, but as is, it's very dangerous, what if the destination already existed?
Case 1: copy file:///dir1 to ftp://host/dir2 which already exists. During the 'stat src' or 'stat dest' phase, cancel the job -> it will now delete dir2 !
(see copyjob.cpp:879 which sets m_currentDestURL).
Case 2: *move* dir1 to dir2 which already exists, cancel before the move happens, boom.
I don't have time to think about more cases, but I suspect there are some more.
Unittesting "cancelling at this exact step of the copy" sounds very tricky, unfortunately... but maybe we can add unittest-specific env vars to abort with a disk full error at some points in the code.
Also, why is this done at the PasteJob level? there are many other ways to trigger a copy, including drag-n-drop. It sounds like this should rather be done inside CopyJob, which would also make it possible to have more information about whether it's safe to clean up or not.
| src/core/copyjob.h | ||
|---|---|---|
| 99 | @since 5.44 if it has to be public, but please also document what this really is, for people who haven't read the copyjob.cpp source code. | |
| src/widgets/dropjob.cpp | ||
| 559 | const int... | |
| 563 | Since you don't test the result of the cast (I assume because by construction only a CopyJob is connected to this slot?), just use static_cast. | |
| 564 | this-> isn't really useful, I'd remove it | |
Comment Actions
I have also done some work on this issue, and my solution for this includes mechanism that should avoid deletion of existing data. I have uploaded my proposal to Phabricator too — hope it will be useful.
Comment Actions
I already like better your proposal than mine. I wish I had reimplemented doKill as you did.
We should consider closing this revision in favor of https://phabricator.kde.org/D10663 that needs polish though IMHO.
Comment Actions
Feel free to abandon the revision. Click the Add Action... pop-up menu that's above the comments box, and select "Abandon Revision."