dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed.
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. INLINE COMMENTS > copyjob.h:99 > + * Returns the current file destination URL. > + * @return the current file destination URL > + */ @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. > dropjob.cpp:559 > { > - if (job->error()) { > - KIO::Job::slotResult(job); // will set the error and emit > result(this) > - return; > + int jobError = job->error(); > + if (jobError) { const int... > dropjob.cpp:563 > + // Must remove last file that was not finished moving/copying > + KIO::CopyJob *copyJob = qobject_cast<KIO::CopyJob *>(job); > + this->addSubjob(KIO::del(copyJob->currentDestUrl(), > JobFlag::HideProgressInfo)); 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. > dropjob.cpp:564 > + KIO::CopyJob *copyJob = qobject_cast<KIO::CopyJob *>(job); > + this->addSubjob(KIO::del(copyJob->currentDestUrl(), > JobFlag::HideProgressInfo)); > + return; this-> isn't really useful, I'd remove it REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D10635 To: meven, dfaure Cc: ltoscano, ngraham, #frameworks, michaelh