<table><tr><td style="">hallas marked an inline comment as done.<br />hallas added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D21760">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D21760#525842" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D21760#525842</a>, <a href="https://phabricator.kde.org/p/dfaure/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@dfaure</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>Yes, the filenames should match the classname, obviously :)</p>
<p>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...</p></div>
</blockquote>
<p>No problem ;) I would rather have this change go in slow and correct than quick and dirty :)</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21760#inline-134104">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">klistopenfiles_unix.cpp:57</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Emitting <tt style="background: #ebebeb; font-size: 13px;">result</tt> twice would be a very bad idea, a KJob can only emit <tt style="background: #ebebeb; font-size: 13px;">result</tt> once.</p>
<p style="padding: 0; margin: 8px;">But QProcess can emit <tt style="background: #ebebeb; font-size: 13px;">errorOccurred</tt> followed by <tt style="background: #ebebeb; font-size: 13px;">finished</tt> (e.g. if the subprocess crashes, from what I can see in qprocess.cpp).<br />
I think we want to only qWarning() in this method, and let <tt style="background: #ebebeb; font-size: 13px;">lsofFinished</tt> take care of finishing the job.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">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.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D21760#inline-134102">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">klistopenfiles_unix.cpp:73</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">The class would be slightly easier to use if this was a getter instead of a signal.</p>
<p style="padding: 0; margin: 8px;">Because then, in a unittest, you can just do <tt style="background: #ebebeb; font-size: 13px;">job->exec()</tt> and then <tt style="background: #ebebeb; font-size: 13px;">job->processInfoList()</tt>, no spy needed.<br />
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 (<tt style="background: #ebebeb; font-size: 13px;">processInfoAvailable</tt> in case of success, and <tt style="background: #ebebeb; font-size: 13px;">result</tt> to handle failure).</p>
<p style="padding: 0; margin: 8px;">See <tt style="background: #ebebeb; font-size: 13px;">KIO::StatJob::statResult()</tt> for an example.</p>
<p style="padding: 0; margin: 8px;">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 <tt style="background: #ebebeb; font-size: 13px;">result</tt>, then it's easier to use <tt style="background: #ebebeb; font-size: 13px;">result</tt> as the notification signal.</p>
<p style="padding: 0; margin: 8px;">The downside is one more member variable, but it'll be very short-lived so I wouldn't worry about memory usage.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Agree, I have refactored the code to do this and it is <strong>much</strong> simpler :)</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R244 KCoreAddons</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D21760">https://phabricator.kde.org/D21760</a></div></div><br /><div><strong>To: </strong>hallas, davidedmundson, broulik, Frameworks, dfaure, bruns, Plasma<br /><strong>Cc: </strong>meven, cfeck, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns<br /></div>