Changeset View
Standalone View
src/widgets/krun.cpp
Show First 20 Lines • Show All 54 Lines • ▼ Show 20 Line(s) | |||||
55 | #include "kio/job.h" | 55 | #include "kio/job.h" | ||
56 | #include "kio/global.h" | 56 | #include "kio/global.h" | ||
57 | #include "kio/scheduler.h" | 57 | #include "kio/scheduler.h" | ||
58 | #include "kopenwithdialog.h" | 58 | #include "kopenwithdialog.h" | ||
59 | #include "krecentdocument.h" | 59 | #include "krecentdocument.h" | ||
60 | #include "kdesktopfileactions.h" | 60 | #include "kdesktopfileactions.h" | ||
61 | #include "executablefileopendialog_p.h" | 61 | #include "executablefileopendialog_p.h" | ||
62 | #include <kio/desktopexecparser.h> | 62 | #include <kio/desktopexecparser.h> | ||
63 | #include <kio/kiofuseinterface.h> | ||||
63 | 64 | | |||
64 | #include <kurlauthorized.h> | 65 | #include <kurlauthorized.h> | ||
65 | #include <kmessagebox.h> | 66 | #include <kmessagebox.h> | ||
66 | #include <ktoolinvocation.h> | 67 | #include <ktoolinvocation.h> | ||
67 | #include <klocalizedstring.h> | 68 | #include <klocalizedstring.h> | ||
68 | #include <kprotocolmanager.h> | 69 | #include <kprotocolmanager.h> | ||
69 | #include <kprocess.h> | 70 | #include <kprocess.h> | ||
70 | #include <kjobwidgets.h> | 71 | #include <kjobwidgets.h> | ||
▲ Show 20 Lines • Show All 513 Lines • ▼ Show 20 Line(s) | |||||
584 | // TODO: make this async, see the job->exec() in there... | 585 | // TODO: make this async, see the job->exec() in there... | ||
585 | static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_service) | 586 | static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_service) | ||
586 | { | 587 | { | ||
587 | // Check which protocols the application supports. | 588 | // Check which protocols the application supports. | ||
588 | // This can be a list of actual protocol names, or just KIO for KDE apps. | 589 | // This can be a list of actual protocol names, or just KIO for KDE apps. | ||
589 | const QStringList appSupportedProtocols = KIO::DesktopExecParser::supportedProtocols(_service); | 590 | const QStringList appSupportedProtocols = KIO::DesktopExecParser::supportedProtocols(_service); | ||
590 | QList<QUrl> urls(_urls); | 591 | QList<QUrl> urls(_urls); | ||
591 | if (!appSupportedProtocols.contains(QLatin1String("KIO"))) { | 592 | if (!appSupportedProtocols.contains(QLatin1String("KIO"))) { | ||
592 | for (QUrl &url : urls) { | 593 | org::kde::KIOFuse kiofuse_iface(QStringLiteral("org.kde.KIOFuse"), | ||
593 | bool supported = KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols); | 594 | QStringLiteral("/org/kde/KIOFuse"), | ||
594 | //qDebug() << "Looking at url=" << url << " supported=" << supported; | 595 | QDBusConnection::sessionBus()); | ||
595 | if (!supported && KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | 596 | struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; }; | ||
597 | QVector<MountRequest> requests; | ||||
sitter: Is there a reason you are not using a range based for loop here? `for (const QUrl &url : urls)` | |||||
What I meant is you should literally iterate using for (QUrl &url : urls) { which is what the code did before but you changed it for some reason. sitter: What I meant is you should literally iterate using
```
for (QUrl &url : urls) {
```
which is… | |||||
Yes, but note later I need to then change the values in the QList outside of the for loop, hence why I store the index in a struct associated with the reply. How would I do that easily with a range based for loop? feverfew: Yes, but note later I need to then change the values in the `QList` outside of the for loop… | |||||
So did the old code though. See old line 584. It's iterating by-reference, so you can simply assign to it via https://doc.qt.io/qt-5/qurl.html#operator-eq-1 to change the content of the QUrl object (not the instance of the object in the list, but rather the content of that object). sitter: So did the old code though. See old line 584.
It's iterating by-reference, so you can simply… | |||||
That's within the same loop though? I explicitly want to change the QUrl in that list, so that I can simply return the same list without having to do a copy of it all. I can't get this to work. I need to change the definition of the struct to be: struct MountRequest {QDBusReply<QString> reply; QUrl &url;}; This doesn't compile because "is implicitly deleted because the default definition would be ill-formed"... feverfew: That's within the same loop though?
I explicitly want to change the QUrl in that list, so that… | |||||
Can't reproduce with gcc. diff --git a/src/widgets/krun.cpp b/src/widgets/krun.cpp index c05875f7..2ea79257 100644 --- a/src/widgets/krun.cpp +++ b/src/widgets/krun.cpp @@ -576,10 +576,9 @@ static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_servic org::kde::KIOFuse::VFS kiofuse_iface(QStringLiteral("org.kde.KIOFuse"), QStringLiteral("/org/kde/KIOFuse"), QDBusConnection::sessionBus()); - struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; }; + struct MountRequest { QDBusPendingReply<QString> reply; QUrl &url; }; QVector<MountRequest> requests; - for (int i = 0; i < urls.length(); ++i) { - const QUrl url = urls[i]; + for (QUrl &url : urls) { if (KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols)) continue; if (KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { @@ -588,7 +587,7 @@ static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_servic if (job->exec()) { // ## nasty nested event loop! const QUrl localURL = job->mostLocalUrl(); if (localURL != url) { - urls[i] = localURL; + url = localURL; //qDebug() << "Changed to" << localURL; // KIOFuse doesn't work too well with mtp/gdrive. // @see https://invent.kde.org/kde/kio-fuse/issues/1 (GDrive) @@ -598,11 +597,11 @@ static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_servic && url.scheme() != QLatin1String("gdrive")) { // Can't convert... // Lets try a KIOFuse mount instead. - requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); + requests.push_back({ kiofuse_iface.mountUrl(url.toString()), url }); } } } else { - requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); + requests.push_back({ kiofuse_iface.mountUrl(url.toString()), url }); } } // Doesn't matter that we've blocked here to process the replies. @@ -610,7 +609,7 @@ static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_servic for (auto request : requests) { request.reply.waitForFinished(); if (!request.reply.isError()) { - urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value()); + request.url = QUrl::fromLocalFile(request.reply.value()); } } } That said, I forgot about the second loop and I am not too sure that carrying the reference across two loops is all that nice to read anyway. Perhaps best wait for dfaure on this matter. sitter: Can't reproduce with gcc.
```
diff --git a/src/widgets/krun.cpp b/src/widgets/krun.cpp
index… | |||||
598 | for (int i = 0; i < urls.length(); ++i) { | ||||
dfaure: count or size | |||||
599 | const QUrl url = urls[i]; | ||||
dfaure: .at(i) | |||||
600 | if (KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols)) | ||||
Coding style says we use curly braces even for single-line if statements I think. sitter: Coding style says we use curly braces even for single-line if statements I think. | |||||
feverfew: Yep you're right, will fix in a mo. | |||||
601 | continue; | ||||
602 | if (KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | ||||
596 | // Maybe we can resolve to a local URL? | 603 | // Maybe we can resolve to a local URL? | ||
597 | KIO::StatJob *job = KIO::mostLocalUrl(url); | 604 | KIO::StatJob *job = KIO::mostLocalUrl(url); | ||
That seems like a hack for a bug in VLC and also super opinionated and also somewhat unrelated to fuse? If an application says it supports %u/%U and a given protocol, we should expect it to be able to parse a valid rfc2396 URI I would think. sitter: That seems like a hack for a bug in VLC and also super opinionated and also somewhat unrelated… | |||||
598 | if (job->exec()) { // ## nasty nested event loop! | 605 | if (job->exec()) { // ## nasty nested event loop! | ||
599 | const QUrl localURL = job->mostLocalUrl(); | 606 | const QUrl localURL = job->mostLocalUrl(); | ||
600 | if (localURL != url) { | 607 | if (localURL != url) { | ||
601 | url = localURL; | 608 | urls[i] = localURL; | ||
602 | //qDebug() << "Changed to" << localURL; | 609 | //qDebug() << "Changed to" << localURL; | ||
610 | // KIOFuse doesn't work too well with mtp/gdrive. | ||||
611 | // @see https://invent.kde.org/kde/kio-fuse/issues/1 (GDrive) | ||||
612 | // @see https://invent.kde.org/kde/kio-fuse/issues/2 (MTP) | ||||
613 | // This can be removed once those two issues are fixed. | ||||
Generally QDBusInterface is a class to be avoided. I would recommend copy/pasting your XML file and then using the generated interface from that. QDBusInterface sucks as it makes a blocking call introspecting (which isn't a huge problem considering we're going to block anyway!) but also leads to going crazy figuring out typos in methods/arguments. davidedmundson: Generally QDBusInterface is a class to be avoided.
I would recommend copy/pasting your XML… | |||||
614 | } else if (url.scheme() != QLatin1String("mtp") | ||||
615 | && url.scheme() != QLatin1String("gdrive")) { | ||||
616 | // Can't convert... | ||||
617 | // Lets try a KIOFuse mount instead. | ||||
We rarely check lastError(), that's for if you use the lower level API calls. davidedmundson: We rarely check lastError(), that's for if you use the lower level API calls.
| |||||
618 | requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); | ||||
619 | } | ||||
620 | } | ||||
621 | } else { | ||||
622 | requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); | ||||
603 | } | 623 | } | ||
604 | } | 624 | } | ||
625 | // Doesn't matter that we've blocked here to process the replies. | ||||
626 | // Main thing that we want is to send the mount requests without blocking. | ||||
627 | for (auto request : requests) { | ||||
628 | request.reply.waitForFinished(); | ||||
629 | if (!request.reply.isError()) { | ||||
630 | urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value()); | ||||
605 | } | 631 | } | ||
606 | } | 632 | } | ||
607 | } | 633 | } | ||
All DBus replies are a union of their success value and an error. Returning an error reply when mounting fails would allow us to provide a more helpful message back to the user davidedmundson: All DBus replies are a union of their success value and an error.
Returning an error reply… | |||||
608 | return urls; | 634 | return urls; | ||
609 | } | 635 | } | ||
610 | 636 | | |||
611 | // Helper function to make the given .desktop file executable by ensuring | 637 | // Helper function to make the given .desktop file executable by ensuring | ||
612 | // that a #!/usr/bin/env xdg-open line is added if necessary and the file has | 638 | // that a #!/usr/bin/env xdg-open line is added if necessary and the file has | ||
613 | // the +x bit set for the user. Returns false if either fails. | 639 | // the +x bit set for the user. Returns false if either fails. | ||
614 | static bool makeServiceFileExecutable(const QString &fileName, QString &errorString) | 640 | static bool makeServiceFileExecutable(const QString &fileName, QString &errorString) | ||
615 | { | 641 | { | ||
▲ Show 20 Lines • Show All 1181 Lines • Show Last 20 Lines |
Is there a reason you are not using a range based for loop here? for (const QUrl &url : urls)