D24962: WIP [DeleteJob] Use a separate worker thread to run actual IO operation

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  I like the approach.
  Just a few minor things.
  
  (and remember to remove the WIP from the commit log and the phab request)
  
  The passing of iterators to another thread *looks* scary, but of course it's safe here since we only run one subjob at a time, so the list can't change while the worker does its work.

INLINE COMMENTS

> deletejob.cpp:68
> +
> +public:
> +

can be removed

> deletejob.cpp:192
> +
> +    m_ioworker = new DeleteJobIOWorkerPrivate;
> +    m_ioworker->moveToThread(&m_thread);

I would remove the "Private" from the name, it makes one think that there is a corresponding public class somewhere.

> deletejob.cpp:194
> +    m_ioworker->moveToThread(&m_thread);
> +    m_thread.connect(&m_thread, &QThread::finished, m_ioworker, &QObject::deleteLater);
> +    m_ioworker->connect(m_ioworker, &DeleteJobIOWorkerPrivate::rmddirResult, [=](bool result, const QList<QUrl>::iterator it){

You're using the 4-args connect, which is static, so `m_thread.connect` is confusing/wrong. This should be `QObject::connect` instead.

> deletejob.cpp:195
> +    m_thread.connect(&m_thread, &QThread::finished, m_ioworker, &QObject::deleteLater);
> +    m_ioworker->connect(m_ioworker, &DeleteJobIOWorkerPrivate::rmddirResult, [=](bool result, const QList<QUrl>::iterator it){
> +        this->rmdirResult(result, it);

Prefer QObject::connect(4 args), the receiver is missing and could be q_func() for extra safety (so that the connect is broken if the DeleteJob is deleted, even if m_ioworker is leaked for some reason).

> deletejob.cpp:359
> +    } else {
> +        deleteNextFile();
> +    }

(With the change I suggest below, this would just move to after `m_processedFiles++;` above. This would simplify the control flow in this method)

> deletejob.cpp:363
> +
> +SimpleJob* DeleteJobPrivate::deleteFileUsingJob (const QList<QUrl>::iterator it)
> +{

No space after the method name.

This method could take a QUrl instead of an iterator, all it does is `*it` to get to the QUrl.

Even better, it could keep using an iterator, but also take care of of the two `erase(it)` calls and `addSubjob`. This is currently duplicated between the two callers. The only difference is `bool isLink`, you could pass that as parameter.

This would even be more consistent with `DeleteJobPrivate::deleteDirUsingJob` which takes care of `erase(it)` and `addSubjob`.

> deletejob.cpp:388
> +        QList<QUrl>::iterator it = files.begin();
> +        bool isLink = (it == files.end()); // No more files
> +        if (isLink) {

You can make it `const` then.

> deletejob.cpp:397
> +            // separate thread will do the work
> +            //m_ioworker->rmfile((*it).toLocalFile());
> +            QMetaObject::invokeMethod(m_ioworker, "rmfile", Qt::QueuedConnection,

I suggest to remove the erroneous comment

> deletejob.cpp:461
> +                                      Q_ARG(const QList<QUrl>::iterator, it));
> +            return;
> +        } else {

Move both `return` to after the `if/else`?

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191027/53188bcb/attachment-0001.html>