> On May 14, 2015, 7:17 p.m., David Faure wrote: > > Right, this code should have been ported to > > > > entry.insert(KIO::UDSEntry::UDS_TARGET_URL, > > QUrl::fromLocalFile(entry.stringValue(KIO::UDSEntry::UDS_LOCAL_PATH))); > > > > This would seem safer to me, because closer to the kde4 behaviour. > > > > I'm just not sure you caught all the cases where the target url was used > > (e.g. properties dialog etc.) > > Eike Hein wrote: > Hmm well ... the properties dialog supports desktop:/ URLs of course > (works fine). Isn't it more that if we fake the URL and something KDE breaks, > that something's got a problem anyway since it should be using KIO?
Hmm. The git log doesn't say why Fredrikh added this (this is why commit logs should explain the "why", not the "what"...). You're right, this isn't about the properties dialog (we still set UDS_LOCAL_PATH). UDS_TARGET_URL is rather "when you click on this file, this is the URL that will open". Allowing to open files on the desktop into apps that don't support URLs, I suppose. But then again, it might be that we resolve to UDS_LOCAL_PATH in that case, when set (the concept of UDS_TARGET_URL is more generic, it could point to other things than local files). So... remove it if you want, as long as you monitor for bugs that this might introduce :-) - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/123781/#review80362 ----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
