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
