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
