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 
  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.


> 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

  R241 KIO


To: meven, dfaure
Cc: ltoscano, ngraham, #frameworks, michaelh

Reply via email to