<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/123781/">https://git.reviewboard.kde.org/r/123781/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 13th, 2015, 11:09 p.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not against the patch, but in general urls without scheme</p></pre>
</blockquote>
<p>On May 13th, 2015, 11:17 p.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Eh sorry, I published before finishing the sentence.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm not against the patch, but in general urls without scheme usually sound like QUrl API misuse.</p></pre>
</blockquote>
<p>On May 13th, 2015, 11:52 p.m. UTC, <b>Eike Hein</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you're asking why KFileItem ends up returning a QUrl without a scheme after UDS_TARGET_URL is set to UDS_LOCAL_PATH: Because QUrl("/path/to/bla").scheme().isEmpty() is true.</p></pre>
</blockquote>
<p>On May 14th, 2015, 12:14 a.m. UTC, <b>Aleix Pol Gonzalez</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">True, so can we turn the QUrl("/path/to/bla") into QUrl::fromLocalFile("/path/to/bla")?</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">No, because it makes no sense to run fromLocalFile() over the UDS_TARGET_URL field, since it's not meant to be a local path. A KFileItem isn't required to be a local file (that's why it has a isLocalFile()). kio_desktop was wrong to put the local path into the field. I assume it was a hack designed to make sure consumers of the slave get local paths, but as explained KRun contains logic to resolve desktop:/ to a file:// URL when needed by the app already.</p></pre>
<br />
<p>- Eike</p>
<br />
<p>On May 13th, 2015, 7:38 p.m. UTC, Eike Hein wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, Plasma, David Faure, and Fredrik Höglund.</div>
<div>By Eike Hein.</div>
<p style="color: grey;"><i>Updated May 13, 2015, 7:38 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio-extras
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kio_desktop's prepareUDSEntry() implementation currently overwrites the entry's UDS_TARGET_URL with UDS_LOCAL_PATH. Down the line this causes KFileItem to return QUrls with an empty scheme(), which leads to problems in kio/src/widgets/krun.cpp's resolveURLs(), used internally by KRun::runService. resolveURLs() will determine that the app doesn't support the (empty) scheme and fall through to check whether it meets the criteria for a KProtocolInfo::protocolClass of :local (which it doesn't either) before running KIO::mostLocalUrl (which thus isn't reached, but if it were, would also balk on an invalid QUrl). Ultimately the URL isn't getting fixed up, which in the case of using an action produced by KFileItemActions::addOpenWithActionsTo will cause the subprocess to be started non-blocking (freezing plasmashell in Folder View's case) and throw up a "Couldn't launch kioexec" error dialog box once it exits.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch simply removes the mangling (originally added by b0f798df), which will cause the entries to have the original desktop:/ URL. When an app doesn't explicitly support this protocol the fallback logic in resolvedURLs() will then produce a file:// URL. This fits in with the overall approach of producing the URLs needed by the app (based on its .desktop file) in KRun, which has all the support it needs to produce local URLs from desktop:/.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Double-clicking files in Folder View wasn't affected because it already had a hack to set the scheme for scheme-less URLs to 'file'; this workaround can be dropped once plasma-desktop depends on a KIO version with this patch applied.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Tried various KDE and non-KDE apps. Also compared this to the URLs handed to KRun by the regular local file browsing slave; they also use the file scheme.</p></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>desktop/kio_desktop.cpp <span style="color: grey">(28fdfe4)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/123781/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>