D18904: [FileCopyJob] Clean up after file copy operation is cancelled

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> jobtest.cpp:2019
> +    QSignalSpy spyCancelCopy(this, &JobTest::cancelCopy);
> +    connect (copyJob, &KIO::Job::processedSize, this, [this](KJob *job, qulonglong processedSize) {
> +        if (processedSize > 0) {

no space after `connect`

> jobtest.cpp:2022
> +            Q_UNUSED(job);
> +            emit cancelCopy();
> +        }

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.

> jobtest.cpp:2033
> +    QCOMPARE(spyCancelCopy.count(), 1);
> +}
> +

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

(The alternative would be to use QTemporaryFile)

> filecopyjob.cpp:478
> +    // If result comes from one of the file writing jobs,
> +    // then we are probably not writing anymore.
> +    if (job == d->m_copyJob) {

Why the "probably"? ;-)

> filecopyjob.cpp:577
> +    // source file is intact (m_delJob == NULL).
> +    if (!isSuspended() && d->m_bFileWriteInProgress) {
> +        if (d->m_copyJob && !d->m_delJob) {

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

> filecopyjob.cpp:578
> +    if (!isSuspended() && d->m_bFileWriteInProgress) {
> +        if (d->m_copyJob && !d->m_delJob) {
> +            QFile::remove(d->m_dest.toLocalFile());

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.

> filecopyjob.cpp:579
> +        if (d->m_copyJob && !d->m_delJob) {
> +            QFile::remove(d->m_dest.toLocalFile());
> +        }

A test for isLocalFile() is missing first.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D18904

To: chinmoyr, dfaure, dmitrio
Cc: kde-frameworks-devel, ngraham, michaelh, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190210/3b3b9998/attachment.html>