D21760: Add KListOpenFilesJob

[David Hallas]

hallas marked an inline comment as done.
hallas added a comment.


  In D21760#525842 <https://phabricator.kde.org/D21760#525842>, @dfaure wrote:
  
  > 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...
  
  
  No problem ;) I would rather have this change go in slow and correct than quick and dirty :)

INLINE COMMENTS

> dfaure wrote in klistopenfiles_unix.cpp:57
> 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.

Good catch! Actually it turns out to be a tad more complicated ;) QProcess will only emit finished if the process actually started, so if you have the case where lsof is not installed you will only get the errorOccurred signal and not finished, but if it crashes you get both. I actually added a unit test to test the first case (where lsof is not found), and I ended up making a solution where we simply remember if we have emitted the result and only emit it if we haven't done so before.

> dfaure wrote in klistopenfiles_unix.cpp:73
> 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.

Agree, I have refactored the code to do this and it is **much** simpler :)

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/20190906/70c23eaf/attachment-0001.html>