Differential D23384 Diff 70866 src/core/desktopexecparser.cpp

Changeset View

Standalone View

View Options

src/core/desktopexecparser.cpp

Show All 14 Lines1/* 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​
42class KRunMX1 : public KMacroExpanderBase 45​class KRunMX1 : public KMacroExpanderBase
43{ 46​{
44public: 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
dfaureUnsubmitted
Done

, 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"),
sitterUnsubmitted
Done

align arguments with first argument. same in krun.cpp.

sitter: align arguments with first argument. same in krun.cpp.
315​ QDBusConnection::sessionBus());
316​ struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; };
sitterUnsubmitted
Done

Do we need this? Seems to me you could just append to d->url directly.

sitter: Do we need this? Seems to me you could just append to d->url directly.
317​ QVector<MountRequest> requests;
sitterUnsubmitted
Not Done

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>`…
feverfewAuthorUnsubmitted
Not Done

Gone for the vector option.

feverfew: Gone for the vector option.
dfaureUnsubmitted
Done

requests.reserve(d->urls.count());

dfaure: `requests.reserve(d->urls.count());`
310 if (!mx1.hasUrls) { 318​ if (!mx1.hasUrls) {
311 for (const QUrl &url : qAsConst(d->urls)) { 319​ for (int i = 0; i < d->urls.length(); ++i) {
dfaureUnsubmitted
Done

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…
320​ const QUrl url = d->urls[i];
sitterUnsubmitted
Done

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)
dfaureUnsubmitted
Done

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)) { 321​ if (!url.isLocalFile() && !hasSchemeHandler(url)) {
322​ // Lets try a KIOFuse mount instead.
323​ requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
dfaureUnsubmitted
Not Done

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)

dfaure: Could we have a kill switch for KIOFuse if it fails to work properly in some release? E.g.
feverfewAuthorUnsubmitted
Not Done

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…
dfaureUnsubmitted
Not Done

OK, that's a possible solution indeed.

dfaure: OK, that's a possible solution indeed.
324​ }
325​ }
326​ } else if (!supportedProtocols(d->service).contains(QLatin1String("KIO"))) {
327​ for (int i = 0; i < d->urls.length(); ++i) {
328​ const QUrl url = d->urls[i];
329​ // NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://)
330​ // but will struggle with passwords.
dfaureUnsubmitted
Done

"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…
331​ // Hence convert URL to KIOFuse equivalent in case there is a password.
dfaureUnsubmitted
Not Done

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?

dfaure: The comment says "in case there is a password", the code doesn't. Probably historical, please…
feverfewAuthorUnsubmitted
Not Done

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…
feverfewAuthorUnsubmitted
Not Done

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
332​ // @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/
333​ // @see https://bugs.kde.org/show_bug.cgi?id=330192
334​ if (url.scheme() == QLatin1String("mtp")
335​ || url.scheme() == QLatin1String("gdrive")) {
336​ // KIOFuse doesn't work too well with mtp/gdrive.
337​ // @see https://invent.kde.org/kde/kio-fuse/issues/1 (GDrive)
338​ // @see https://invent.kde.org/kde/kio-fuse/issues/2 (MTP)
339​ // This can be removed once those two issues are fixed.
313 useKioexec = true; 340​ useKioexec = true;
314 //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec"; ​
315 break; 341​ break;
342​ } else if (url.scheme() == QLatin1String("http") || url.scheme() == QLatin1String("https")) {
dfaureUnsubmitted
Not Done

s/length/count/

dfaure: s/length/count/
343​ if (!supportedProtocols(d->service).contains(url.scheme())) {
344​ requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
345​ }
346​ } else {
347​ // Lets try a KIOFuse mount instead.
348​ requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
dfaureUnsubmitted
Done

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())) {

dfaure: This duplicates line 344. `if (a) { if (!b) { foo } } else { foo }` is equivalent to `if (!a…
349​ }
316 } 350​ }
dfaureUnsubmitted
Not Done

coding style: missing space after "if"

dfaure: coding style: missing space after "if"
317 } 351​ }
318 } else { // app claims to support %u/%U, check which protocols 352​
319 QStringList appSupportedProtocols = supportedProtocols(d->service); 353​ // Doesn't matter that we've blocked here to process the replies.
320 for (const QUrl &url : qAsConst(d->urls)) { 354​ // Main thing that we want is to send the mount requests without blocking.
321 if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) { 355​ for (auto request : requests) {
dfaureUnsubmitted
Done

auto &request to avoid copying

dfaure: `auto &request` to avoid copying
356​ request.reply.waitForFinished();
dfaureUnsubmitted
Not Done

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…
broulikUnsubmitted
Not Done

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…
feverfewAuthorUnsubmitted
Not Done

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…
dfaureUnsubmitted
Not Done

I'll try to keep this in mind when coming back to my KJob-based rewrite of KRun.
I keep postponing it because people keep changing KRun :-)

dfaure: I'll try to keep this in mind when coming back to my KJob-based rewrite of KRun. I keep…
357​ if (request.reply.isError()) {
322 useKioexec = true; 358​ useKioexec = true;
323 //qCDebug(KIO_CORE) << "application does not support url, using kioexec:" << url; 359​ // At this point we should just send the original urls to kioexec.
360​ // There's no point sending urls to kioexec that go through kiofuse.
324 break; 361​ break;
325 } 362​ }
326 } 363​ }
327 } 364​
328 if (useKioexec) { 365​ if (useKioexec) {
329 // We need to run the app through kioexec 366​ // We need to run the app through kioexec
330 result << kioexecPath(); 367​ result << kioexecPath();
331 if (d->tempFiles) { 368​ if (d->tempFiles) {
332 result << QStringLiteral("--tempfiles"); 369​ result << QStringLiteral("--tempfiles");
333 } 370​ }
334 if (!d->suggestedFileName.isEmpty()) { 371​ if (!d->suggestedFileName.isEmpty()) {
335 result << QStringLiteral("--suggestedfilename"); 372​ result << QStringLiteral("--suggestedfilename");
336 result << d->suggestedFileName; 373​ result << d->suggestedFileName;
337 } 374​ }
338 result << exec; 375​ result << exec;
339 result += QUrl::toStringList(d->urls); 376​ result += QUrl::toStringList(d->urls);
340 return result; 377​ return result;
378​ } else {
dfaureUnsubmitted
Done

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…
379​ // At this point we know we're not using kioexec, so feel free to replace
380​ // KIO URLs with their KIOFuse local path.
381​ for (auto request : requests) {
dfaureUnsubmitted
Done

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…
382​ d->urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value());
383​ }
341 } 384​ }
342 385​
343 if (appHasTempFileOption) { 386​ if (appHasTempFileOption) {
344 exec += QLatin1String(" --tempfile"); 387​ exec += QLatin1String(" --tempfile");
345 } 388​ }
346 389​
347 // Did the user forget to append something like '%f'? 390​ // Did the user forget to append something like '%f'?
dfaureUnsubmitted
Not Done

indentation of those last 5 lines seems wrong (2 spaces instead of 4?)

dfaure: indentation of those last 5 lines seems wrong (2 spaces instead of 4?)
348 // If so, then assume that '%f' is the right choice => the application 391​ // If so, then assume that '%f' is the right choice => the application
349 // accepts only local files. 392​ // accepts only local files.
350 if (!mx1.hasSpec) { 393​ if (!mx1.hasSpec) {
351 exec += QLatin1String(" %f"); 394​ exec += QLatin1String(" %f");
352 mx2.ignFile = true; 395​ mx2.ignFile = true;
353 } 396​ }
354 397​
355 mx2.expandMacrosShellQuote(exec); // syntax was already checked, so don't check return value 398​ mx2.expandMacrosShellQuote(exec); // syntax was already checked, so don't check return value
▲ Show 20 Lines • Show All 105 Lines • Show Last 20 Lines