<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106500/">http://git.reviewboard.kde.org/r/106500/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for looking at this! Looks mostly good, apart from two issues below.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106500/diff/1/?file=86190#file86190line1090" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/krun.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">bool KRun::runCommand(const QString& cmd, const QString &execName, const QString & iconName, QWidget* window, const QByteArray& asn)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1087</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">//proc->setShellCommand(cmd);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes we need this, it's the command to be executed :-)
Example, "kget srcUrl destUrl". I guess KShell::splitArgs could be used, if QProcess can only take individual args?
This would still leave a behavior difference with $HOME and ~ and other shell-specific things, but I guess we could ban these, due to the lack of portability (to Windows for instance).
Looking at the uses of runCommand in kdelibs, I see that many of these are assembling the command line themselves... so it sounds like they should use QProcess directly instead, or (to benefit from startup notification and error boxes if the executable isn't found, etc.), we need to ensure KRun provides a way to run a command like "executable + args", in addition to the current "QString cmd". Only then we can start considering to deprecate the "QString cmd" one...</pre>
</div>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106500/diff/1/?file=86190#file86190line1735" style="color: black; font-weight: bold; text-decoration: underline;">kio/kio/krun.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">mode_t KRun::mode() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1730</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="p">(</span><span class="k">new</span> <span class="n"><span class="hl">K</span>ProcessRunner</span><span class="p">(</span><span class="n">p</span><span class="p">,</span> <span class="n">executable</span><span class="p">,</span> <span class="n">id</span><span class="p">))</span><span class="o">-></span><span class="n">pid</span><span class="p">();</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1732</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="p">(</span><span class="k">new</span> <span class="n"><span class="hl">Q</span>ProcessRunner</span><span class="p">(</span><span class="n">p</span><span class="p">,</span> <span class="n">executable</span><span class="p">,</span> <span class="n">id</span><span class="p">))</span><span class="o">-></span><span class="n">pid</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That was a bit of an over-zealous search/replace ;-)
We should keep the KProcessRunner class name, only Qt should have classes starting with a Q.</pre>
</div>
<br />
<p>- David</p>
<br />
<p>On September 18th, 2012, 11:12 p.m., Kevin Funk wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Kevin Funk.</div>
<p style="color: grey;"><i>Updated Sept. 18, 2012, 11:12 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Port some uses of KProcess to QProcess</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdeui/dialogs/kedittoolbar.cpp <span style="color: grey">(0582934b0bf8cb7160cb48b4c8151c81b35277f0)</span></li>
<li>kinit/klauncher.cpp <span style="color: grey">(855e56041be5a5b76b9a7e9d0597ac7ad485682e)</span></li>
<li>kio/kfile/kfilemetadatareader.cpp <span style="color: grey">(88cadaa2edf1b1de24c0e91576cca368db41f470)</span></li>
<li>kio/kio/krun.h <span style="color: grey">(7bfe66b59f1deffc37d3ceae999fb929e453fd31)</span></li>
<li>kio/kio/krun.cpp <span style="color: grey">(031dbc1dfef685729038b4a59cbeacd34d448ed2)</span></li>
<li>kio/kio/krun_p.h <span style="color: grey">(0ad15c8434599ccabcd649f251aa622d4fb0b0f7)</span></li>
<li>staging/kwidgets/autotests/kglobalsettingstest.cpp <span style="color: grey">(4426fee08427499c777cc7fc94e4b1345c790ac2)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106500/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>