dfaure added a comment.
Why does KRun duplicate all of the (new) code from DesktopExecParser, when DesktopExecParser is actually a helper class for KRun? I would expect it to have solved all this already, unless I'm missing something about the various code paths. The code you changed in krun.cpp was supposed to simply resolve desktop:/foo to file:///bleh but everything else about %u/%f is done by DesktopExecParser. INLINE COMMENTS > desktopexecparser.cpp:311 > > // Check if we need kioexec > bool useKioexec = false; `, or KIOFuse` > desktopexecparser.cpp:317 > + struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; }; > + QVector<MountRequest> requests; > if (!mx1.hasUrls) { `requests.reserve(d->urls.count());` > desktopexecparser.cpp:319 > if (!mx1.hasUrls) { > - for (const QUrl &url : qAsConst(d->urls)) { > + for (int i = 0; i < d->urls.length(); ++i) { > + const QUrl url = d->urls[i]; Minor: can you use count() (Qt-like) or size() (STL-like)? In 20 years I never saw code that uses length() for a vector or a list, it's just very unusual in Qt code :-) [occurs twice] > desktopexecparser.cpp:320 > + for (int i = 0; i < d->urls.length(); ++i) { > + const QUrl url = d->urls[i]; > if (!url.isLocalFile() && !hasSchemeHandler(url)) { Use d->urls.at(i) to avoid a detach given that d->urls isn't const here. [occurs twice] > desktopexecparser.cpp:323 > + // Lets try a KIOFuse mount instead. > + requests.push_back({ kiofuse_iface.mountUrl(url.toString()), > i }); > } Could we have a kill switch for KIOFuse if it fails to work properly in some release? E.g. some `export KIO_FUSE=0` or `export KIO_FUSE_DISABLE=1`. Or a KConfig entry (use `KSharedConfig::openConfig()->group("KIO")` for best performance and flexibility -- then it can be disabled for one calling application or for all) > desktopexecparser.cpp:330 > + // NOTE: Some non-KIO apps may support the URLs (e.g. VLC > supports smb://) > + // but will struggle with passwords. > + // Hence convert URL to KIOFuse equivalent in case there is a > password. "Struggle" sounds like the apps are limited/buggy. But if what you mean is that the password (e.g. stored in kpasswdserver after the user typed it in the KDE dialog asking for it) won't be available to non-KIO applications, that seems logical and I do accept that argument. It's just the writing that is surprising. > desktopexecparser.cpp:331 > + // but will struggle with passwords. > + // Hence convert URL to KIOFuse equivalent in case there is a > password. > + // @see > https://pointieststick.com/2018/01/17/videos-on-samba-shares/ The comment says "in case there is a password", the code doesn't. Probably historical, please fix :) On this topic I saw earlier comments in the merge request which I found confusing. It's very rare for an application to have the password stored inside the URL itself. That would mean the user typing it in clear in a location bar or in a terminal, bad idea. Instead, the user typically types a URL like ftp://user@host/ and the kioslave asks for the password, and stores it in kpasswdserver. Also you wrote "we strip out the stuff that's in userInfo()". (where QUrl::userInfo is username+password). But surely, while we might strip out passwords for security reasons, we never strip out the username, do we? > desktopexecparser.cpp:348 > + // Lets try a KIOFuse mount instead. > + requests.push_back({ kiofuse_iface.mountUrl(url.toString()), > i }); > } This duplicates line 344. `if (a) { if (!b) { foo } } else { foo }` is equivalent to `if (!a || !b) { foo }` In your case `a` is `http || https` which makes this a big uglier, but you could use what other pieces of code do and write url.scheme().startsWith(QLatin1String("http")). So this becomes `if (!url.scheme().startsWith(QLatin1String("http")) || !supportedProtocols(d->service).contains(url.scheme())) {` > desktopexecparser.cpp:355 > + // Main thing that we want is to send the mount requests without > blocking. > + for (auto request : requests) { > + request.reply.waitForFinished(); `auto &request` to avoid copying > desktopexecparser.cpp:356 > + for (auto request : requests) { > + request.reply.waitForFinished(); > + if (request.reply.isError()) { This blocks. I don't mind much myself, but I know some people had a mandate to remove as many blocking calls as possible from KRun and related code. Or was that only avoiding blocking I/O because of network mounts? [which this patch is all about adding more of...]. @broulik ? > desktopexecparser.cpp:378 > return result; > + } else { > + // At this point we know we're not using kioexec, so feel free to > replace This nesting under `else` is unnecessary given the `return` just above. It's even a bit confusing that some code is in `else` and then some code is outside the `if/else`, but the '}' could be anywhere in between, that would make no difference. I would suggest to just remove the `else`. > desktopexecparser.cpp:381 > + // KIO URLs with their KIOFuse local path. > + for (auto request : requests) { > + d->urls[request.urlIndex] = > QUrl::fromLocalFile(request.reply.value()); `for (const auto & request : qAsConst(requests))` or use std::vector if you find qAsConst ugly :-) > CMakeLists.txt:80 > qt5_add_dbus_interface(kiowidgets_SRCS org.kde.kuiserver.xml > kuiserver_interface) > +qt5_add_dbus_interface(kiowidgets_SRCS org.kde.KIOFuse.VFS.xml > kiofuse_interface) > Just use ../core/org.kde.KIOFuse.VFS.xml instead of duplicating the file. But even better would be to not need it in KRun, see separate comment. > krun.cpp:581 > + QVector<MountRequest> requests; > + for (int i = 0; i < urls.length(); ++i) { > + const QUrl url = urls[i]; count or size > krun.cpp:582 > + for (int i = 0; i < urls.length(); ++i) { > + const QUrl url = urls[i]; > + // NOTE: Some non-KIO apps may support the URLs (e.g. VLC > supports smb://) .at(i) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23384 To: feverfew, fvogt, davidedmundson, dfaure, ngraham Cc: alexde, broulik, sitter, davidedmundson, kde-frameworks-devel, ngraham, LeGast00n, GB_2, michaelh, bruns