Changeset View
Standalone View
src/core/desktopexecparser.cpp
Show All 14 Lines | 1 | /* This file is part of the KDE libraries | |||
---|---|---|---|---|---|
15 | 15 | | |||
16 | You should have received a copy of the GNU Library General Public License | 16 | You should have received a copy of the GNU Library General Public License | ||
17 | along with this library; see the file COPYING.LIB. If not, write to | 17 | along with this library; see the file COPYING.LIB. If not, write to | ||
18 | the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, | 18 | the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, | ||
19 | Boston, MA 02110-1301, USA. | 19 | Boston, MA 02110-1301, USA. | ||
20 | */ | 20 | */ | ||
21 | 21 | | |||
22 | #include "desktopexecparser.h" | 22 | #include "desktopexecparser.h" | ||
23 | #include "kiofuse_interface.h" | ||||
23 | 24 | | |||
24 | #include <kmacroexpander.h> | 25 | #include <kmacroexpander.h> | ||
25 | #include <kshell.h> | 26 | #include <kshell.h> | ||
26 | #include <ksharedconfig.h> | 27 | #include <ksharedconfig.h> | ||
27 | #include <kdesktopfile.h> | 28 | #include <kdesktopfile.h> | ||
28 | #include <kservice.h> | 29 | #include <kservice.h> | ||
29 | #include <kconfiggroup.h> | 30 | #include <kconfiggroup.h> | ||
30 | #include <kprotocolinfo.h> | 31 | #include <kprotocolinfo.h> | ||
31 | #include <kmimetypetrader.h> | 32 | #include <kmimetypetrader.h> | ||
32 | 33 | | |||
33 | #include <QFile> | 34 | #include <QFile> | ||
34 | #include <QDir> | 35 | #include <QDir> | ||
35 | #include <QUrl> | 36 | #include <QUrl> | ||
36 | #include <QStandardPaths> | 37 | #include <QStandardPaths> | ||
38 | #include <QDBusConnection> | ||||
39 | #include <QDBusReply> | ||||
37 | 40 | | |||
38 | #include <config-kiocore.h> // CMAKE_INSTALL_FULL_LIBEXECDIR_KF5 | 41 | #include <config-kiocore.h> // CMAKE_INSTALL_FULL_LIBEXECDIR_KF5 | ||
39 | 42 | | |||
40 | #include "kiocoredebug.h" | 43 | #include "kiocoredebug.h" | ||
41 | 44 | | |||
42 | class KRunMX1 : public KMacroExpanderBase | 45 | class KRunMX1 : public KMacroExpanderBase | ||
43 | { | 46 | { | ||
44 | public: | 47 | public: | ||
▲ Show 20 Lines • Show All 255 Lines • ▼ Show 20 Line(s) | 301 | if (d->tempFiles && !appHasTempFileOption && d->urls.size()) { | |||
300 | if (!d->suggestedFileName.isEmpty()) { | 303 | if (!d->suggestedFileName.isEmpty()) { | ||
301 | result << QStringLiteral("--suggestedfilename"); | 304 | result << QStringLiteral("--suggestedfilename"); | ||
302 | result << d->suggestedFileName; | 305 | result << d->suggestedFileName; | ||
303 | } | 306 | } | ||
304 | result += QUrl::toStringList(d->urls); | 307 | result += QUrl::toStringList(d->urls); | ||
305 | return result; | 308 | return result; | ||
306 | } | 309 | } | ||
307 | 310 | | |||
308 | // Check if we need kioexec | 311 | // Check if we need kioexec, or KIOFuse | ||
dfaure: `, or KIOFuse` | |||||
309 | bool useKioexec = false; | 312 | bool useKioexec = false; | ||
313 | org::kde::KIOFuse::VFS kiofuse_iface(QStringLiteral("org.kde.KIOFuse"), | ||||
314 | QStringLiteral("/org/kde/KIOFuse"), | ||||
sitter: align arguments with first argument. same in krun.cpp. | |||||
315 | QDBusConnection::sessionBus()); | ||||
316 | struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; }; | ||||
sitter: Do we need this? Seems to me you could just append to d->url directly. | |||||
317 | QVector<MountRequest> requests; | ||||
There's nothing wrong with this. But wouldn't using QHash<QDBusPendingReply<QString>, QUrl> make for easier to read code all in all since you don't have to mess with pairs? Alternatively with a vector I'd still make a struct for the data struct MountRequest { QDBusPendingReply<QString> reply, QUrl url }; QVector<MountRequest> requests; ... requests.push_back({ mountUrl(url), url }); sitter: There's nothing wrong with this. But wouldn't using `QHash<QDBusPendingReply<QString>, QUrl>`… | |||||
feverfew: Gone for the vector option. | |||||
dfaure: `requests.reserve(d->urls.count());` | |||||
318 | requests.reserve(d->urls.count()); | ||||
319 | QStringList appSupportedProtocols = supportedProtocols(d->service); | ||||
310 | if (!mx1.hasUrls) { | 320 | if (!mx1.hasUrls) { | ||
311 | for (const QUrl &url : qAsConst(d->urls)) { | 321 | for (int i = 0; i < d->urls.count(); ++i) { | ||
322 | const QUrl url = d->urls.at(i); | ||||
there should be a space after for. what happened to the constness (also in the loop below) sitter: there should be a space after for.
what happened to the constness (also in the loop below) | |||||
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] dfaure: Minor: can you use count() (Qt-like) or size() (STL-like)? In 20 years I never saw code that… | |||||
Use d->urls.at(i) to avoid a detach given that d->urls isn't const here. [occurs twice] dfaure: Use d->urls.at(i) to avoid a detach given that d->urls isn't const here.
[occurs twice] | |||||
312 | if (!url.isLocalFile() && !hasSchemeHandler(url)) { | 323 | if (!url.isLocalFile() && !hasSchemeHandler(url)) { | ||
313 | useKioexec = true; | 324 | // Lets try a KIOFuse mount instead. | ||
314 | //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec"; | 325 | 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. dfaure: Could we have a kill switch for KIOFuse if it fails to work properly in some release?
E.g. | |||||
One can always remove/uninstall kiofuse if it doesn't work or if they don't really want the feature? feverfew: One can always remove/uninstall kiofuse if it doesn't work or if they don't really want the… | |||||
dfaure: OK, that's a possible solution indeed. | |||||
315 | break; | | |||
316 | } | 326 | } | ||
317 | } | 327 | } | ||
318 | } else { // app claims to support %u/%U, check which protocols | 328 | } else if (!appSupportedProtocols.contains(QLatin1String("KIO"))) { | ||
319 | QStringList appSupportedProtocols = supportedProtocols(d->service); | 329 | for (int i = 0; i < d->urls.count(); ++i) { | ||
dfaure: s/length/count/ | |||||
320 | for (const QUrl &url : qAsConst(d->urls)) { | 330 | const QUrl url = d->urls.at(i); | ||
321 | if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) { | 331 | const bool supported = isProtocolInSupportedList(url, appSupportedProtocols); | ||
"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. dfaure: "Struggle" sounds like the apps are limited/buggy.
But if what you mean is that the password… | |||||
332 | // NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://) | ||||
The comment says "in case there is a password", the code doesn't. On this topic I saw earlier comments in the merge request which I found confusing. 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? dfaure: The comment says "in case there is a password", the code doesn't.
Probably historical, please… | |||||
From what others have tested userInfo() is always empty even when the URL is actually a protected Samba share... This means I can't know if a certain URL is likely to have a password or not, I just have to assume it does. I'll check again just in case, as indeed it does seem odd, but that was my conclusion. feverfew: From what others have tested `userInfo()` is always empty even when the URL is actually a… | |||||
From my own testing your intuition seems correct. I've now amended my diff appropriately feverfew: From my own testing your intuition seems correct. I've now amended my diff appropriately | |||||
333 | // but will not have the password if they are not in the URL itself. | ||||
334 | // Hence convert URL to KIOFuse equivalent in case there is a password. | ||||
335 | // @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/ | ||||
This duplicates line 344. if (a) { if (!b) { foo } } else { 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())) { dfaure: This duplicates line 344.
`if (a) { if (!b) { foo } } else { foo }`
is equivalent to
`if (!a… | |||||
336 | // @see https://bugs.kde.org/show_bug.cgi?id=330192 | ||||
337 | if (!supported || (!url.userName().isEmpty() && url.password().isEmpty())) { | ||||
dfaure: coding style: missing space after "if" | |||||
338 | requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); | ||||
339 | } | ||||
340 | } | ||||
341 | } | ||||
342 | | ||||
343 | // Doesn't matter that we've blocked here to process the replies. | ||||
344 | // Main thing that we want is to send the mount requests without blocking. | ||||
345 | for (auto &request : requests) { | ||||
dfaure: `auto &request` to avoid copying | |||||
346 | request.reply.waitForFinished(); | ||||
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 ? dfaure: This blocks.
I don't mind much myself, but I know some people had a mandate to remove as many… | |||||
Blocking IO is worse than blocking DBus, still not nice, especially given the "job"-like nature of KRun I would expect it to be fully async. broulik: Blocking IO is worse than blocking DBus, still not nice, especially given the "job"-like nature… | |||||
In these methods I have to return the URLs within the function, so blocking is inevitable. If I could return the URLs via a signal then blocking could be avoided. feverfew: In these methods I have to return the URLs within the function, so blocking is inevitable. If I… | |||||
I'll try to keep this in mind when coming back to my KJob-based rewrite of KRun. dfaure: I'll try to keep this in mind when coming back to my KJob-based rewrite of KRun.
I keep… | |||||
347 | if (request.reply.isError()) { | ||||
322 | useKioexec = true; | 348 | useKioexec = true; | ||
323 | //qCDebug(KIO_CORE) << "application does not support url, using kioexec:" << url; | 349 | // At this point we should just send the original urls to kioexec. | ||
350 | // There's no point sending urls to kioexec that go through kiofuse. | ||||
324 | break; | 351 | break; | ||
325 | } | 352 | } | ||
326 | } | 353 | } | ||
327 | } | 354 | | ||
328 | if (useKioexec) { | 355 | if (useKioexec) { | ||
329 | // We need to run the app through kioexec | 356 | // We need to run the app through kioexec | ||
330 | result << kioexecPath(); | 357 | result << kioexecPath(); | ||
331 | if (d->tempFiles) { | 358 | if (d->tempFiles) { | ||
332 | result << QStringLiteral("--tempfiles"); | 359 | result << QStringLiteral("--tempfiles"); | ||
333 | } | 360 | } | ||
334 | if (!d->suggestedFileName.isEmpty()) { | 361 | if (!d->suggestedFileName.isEmpty()) { | ||
335 | result << QStringLiteral("--suggestedfilename"); | 362 | result << QStringLiteral("--suggestedfilename"); | ||
336 | result << d->suggestedFileName; | 363 | result << d->suggestedFileName; | ||
337 | } | 364 | } | ||
338 | result << exec; | 365 | result << exec; | ||
339 | result += QUrl::toStringList(d->urls); | 366 | result += QUrl::toStringList(d->urls); | ||
340 | return result; | 367 | return result; | ||
341 | } | 368 | } | ||
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. dfaure: This nesting under `else` is unnecessary given the `return` just above. It's even a bit… | |||||
342 | 369 | | |||
370 | // At this point we know we're not using kioexec, so feel free to replace | ||||
371 | // KIO URLs with their KIOFuse local path. | ||||
for (const auto & request : qAsConst(requests)) or use std::vector if you find qAsConst ugly :-) dfaure: `for (const auto & request : qAsConst(requests))`
or use std::vector if you find qAsConst ugly… | |||||
372 | for (const auto &request : qAsConst(requests)) { | ||||
373 | d->urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value()); | ||||
374 | } | ||||
dfaure: indentation of those last 5 lines seems wrong (2 spaces instead of 4?) | |||||
375 | | ||||
343 | if (appHasTempFileOption) { | 376 | if (appHasTempFileOption) { | ||
344 | exec += QLatin1String(" --tempfile"); | 377 | exec += QLatin1String(" --tempfile"); | ||
345 | } | 378 | } | ||
346 | 379 | | |||
347 | // Did the user forget to append something like '%f'? | 380 | // Did the user forget to append something like '%f'? | ||
348 | // If so, then assume that '%f' is the right choice => the application | 381 | // If so, then assume that '%f' is the right choice => the application | ||
349 | // accepts only local files. | 382 | // accepts only local files. | ||
350 | if (!mx1.hasSpec) { | 383 | if (!mx1.hasSpec) { | ||
▲ Show 20 Lines • Show All 110 Lines • Show Last 20 Lines |
, or KIOFuse