> On May 13, 2015, 11:09 p.m., Aleix Pol Gonzalez wrote:
> > I'm not against the patch, but in general urls without scheme
>
> Aleix Pol Gonzalez wrote:
> Eh sorry, I published before finishing the sentence.
>
> I'm not against the patch, but in general urls without scheme usually
> sound like QUrl API misuse.
>
> Eike Hein wrote:
> 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.
>
> Aleix Pol Gonzalez wrote:
> True, so can we turn the QUrl("/path/to/bla") into
> QUrl::fromLocalFile("/path/to/bla")?
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.
- Eike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123781/#review80321
-----------------------------------------------------------
On May 13, 2015, 7:38 p.m., Eike Hein wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123781/
> -----------------------------------------------------------
>
> (Updated May 13, 2015, 7:38 p.m.)
>
>
> Review request for KDE Frameworks, Plasma, David Faure, and Fredrik Höglund.
>
>
> Repository: kio-extras
>
>
> Description
> -------
>
> 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.
>
> 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:/.
>
> 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.
>
>
> Diffs
> -----
>
> desktop/kio_desktop.cpp 28fdfe4
>
> Diff: https://git.reviewboard.kde.org/r/123781/diff/
>
>
> Testing
> -------
>
> 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.
>
>
> Thanks,
>
> Eike Hein
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
