D21760: Add KListOpenFilesJob

[David Faure]

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


  Yes, the filenames should match the classname, obviously :)
  
  I did a deeper review of the KJob usage and I have two more comments, sorry for not taking the time to do this earlier...

INLINE COMMENTS

> klistopenfiles_unix.cpp:57
> +        job->setErrorText(QObject::tr("Failed to execute `lsof' error code %1").arg(processError));
> +        job->emitResult();
> +    }

Emitting `result` twice would be a very bad idea, a KJob can only emit `result` once.

But QProcess can emit `errorOccurred` followed by `finished` (e.g. if the subprocess crashes, from what I can see in qprocess.cpp).
I think we want to only qWarning() in this method, and let `lsofFinished` take care of finishing the job.

> klistopenfiles_unix.cpp:62
> +        QStringList blockApps;
> +        QString out(QString::fromLocal8Bit(lsofProcess.readAll()));
> +        QStringList pidList = out.split(QRegExp(QStringLiteral("\\s+")), QString::SkipEmptyParts);

const

> klistopenfiles_unix.cpp:73
> +        }
> +        emit job->processInfoAvailable(path.path(), processInfoList);
> +        job->emitResult();

The class would be slightly easier to use if this was a getter instead of a signal.

Because then, in a unittest, you can just do `job->exec()` and then `job->processInfoList()`, no spy needed.
In (GUI) application code you still need to connect to a signal, but then the usual KJob::result signal is enough, instead of two signals (`processInfoAvailable` in case of success, and `result` to handle failure).

See `KIO::StatJob::statResult()` for an example.

Signals emitted by jobs are useful if they happen during the job lifetime (e.g. progress signals). If they happen at the same time as `result`, then it's easier to use `result` as the notification signal.

The downside is one more member variable, but it'll be very short-lived so I wouldn't worry about memory usage.

REPOSITORY
  R244 KCoreAddons

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

To: hallas, davidedmundson, broulik, #frameworks, dfaure, bruns, #plasma
Cc: meven, cfeck, 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/20190905/5c00f995/attachment.html>