> 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? > > David Faure wrote: > 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 :-) > > Eike Hein wrote: > I maintain the primary user of desktop:/, so I think bugs are bound to > hit me first :-) > > Another reason against adding that fromLocalFile() call might be that the > user could technically set a fancy slave for their desktop path in the KCM > ... not sure if anything anywhere prevents this, but if it doesn't, bypassing > the mostLocalUrl() call in KRun causes another bug, etc. > > So yeah, let's go with this for now ... can you Ship It then? :)
Well, make sure UDS_LOCAL_PATH never points to anything else than a local path.... (it's a path, not a URL!). Much code relies on that. - 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
