<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/111272/">http://git.reviewboard.kde.org/r/111272/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 2nd, 2013, 7:48 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/111272/diff/2/?file=167013#file167013line82" style="color: black; font-weight: bold; text-decoration: underline;">kdecore/services/kservice.h</a>
<span style="font-weight: normal;">
(Diff revision 2)
</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; ">public:</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">82</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> * If @ref exec is not empty, then its value will override the one</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">How about a setExec() method rather than a special constructor? This seems a lot clearer to me (and e.g. easier to grep for in order to find usages, etc.).
But anyway, I have my doubts that this really works: returning a KService that isn't exactly like the one in ksycoca seems fragile to me.
E.g. when krun uses that service, it will call KToolInvocation with the entry path to the service - i.e. just the path to the desktop file. The modified Exec field (in memory) won't be used in that case.
What KRun can do, though, is work with a "temp" kservice, created like this:
KService::Ptr service(new KService(_name, _exec, _icon));
This is detected as "not in ksycoca" and therefore not given to KToolInvocation (but to runTempService).
I think this would recreate the issue you saw though, since indeed nowadays it's not enough to have %U in the exec line, one must also have Categories or X-KDE-Protocols indicating that KIO protocols are supported. But this could be copied from the orig file, using a new setter in KService. Hmm.
Maybe it would be simpler to just clear entryPath() in the KService::setExec() I'm suggesting initially. i.e.: if we're modifying the KService then the path to the file that has been cached in ksycoca, no longer applies.
And then KRun will go to runTempService, when running it.
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well I created a special constructor instead of a setter because I wanted to limit the scope of change to KService's behavior as low as possible. If I added a setExec, it would allow the "Exec=" to be changed at anytime regardless of how KService was created. To me that did not make sense as the need to change the "Exec=" line is very specific scenarios like the one we have in the openWith dialog. Moreover, we already have several different ctors for different use cases right now. Adding one that could be merged with an existing on in KF5 would be the lowest impact change that could be made to KService itself. Are
you sure adding a setExec() is the better way to go in this case?</pre>
<br />
<p>- Dawit</p>
<br />
<p>On July 2nd, 2013, 2:41 a.m. UTC, Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs and David Faure.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated July 2, 2013, 2:41 a.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;">The attached patch addresses a bug where a user enters the name of a KDE application in OpenWith dialog to open a remote file and the file is opened as if the user requested to open it with a non KDE application. That is a local copy of the file is created first. Currently this problem can be reproduced with kate because the "Exec=" line in its desktop file contains an additional option, "-b".
Note that this patch only addresses the specific condition where the user only typed in the KDE executable name. Other scenarios, like the user typing in not only the name of the KDE app but also additional command line options, will still produce this same issue.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Try to open a remote text or source file by typing "kate" in the open with dialog.
</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=222519">222519</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdecore/services/kservice.h <span style="color: grey">(3843bad)</span></li>
<li>kdecore/services/kservice.cpp <span style="color: grey">(e2cc86f)</span></li>
<li>kio/kfile/kopenwithdialog.cpp <span style="color: grey">(84465cd)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111272/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>