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
