Changeset View
Standalone View
src/widgets/krun.cpp
Show First 20 Lines • Show All 53 Lines • ▼ Show 20 Line(s) | |||||
54 | #include <kmimetypetrader.h> | 54 | #include <kmimetypetrader.h> | ||
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 <kio/desktopexecparser.h> | 61 | #include <kio/desktopexecparser.h> | ||
62 | #include "kiofuse_interface.h" | ||||
62 | 63 | | |||
63 | #include <kurlauthorized.h> | 64 | #include <kurlauthorized.h> | ||
64 | #include <kmessagebox.h> | 65 | #include <kmessagebox.h> | ||
65 | #include <ktoolinvocation.h> | 66 | #include <ktoolinvocation.h> | ||
66 | #include <klocalizedstring.h> | 67 | #include <klocalizedstring.h> | ||
67 | #include <kprotocolmanager.h> | 68 | #include <kprotocolmanager.h> | ||
68 | #include <kprocess.h> | 69 | #include <kprocess.h> | ||
69 | #include <kjobwidgets.h> | 70 | #include <kjobwidgets.h> | ||
▲ Show 20 Lines • Show All 497 Lines • ▼ Show 20 Line(s) | |||||
567 | // TODO: make this async, see the job->exec() in there... | 568 | // TODO: make this async, see the job->exec() in there... | ||
568 | static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_service) | 569 | static QList<QUrl> resolveURLs(const QList<QUrl> &_urls, const KService &_service) | ||
569 | { | 570 | { | ||
570 | // Check which protocols the application supports. | 571 | // Check which protocols the application supports. | ||
571 | // This can be a list of actual protocol names, or just KIO for KDE apps. | 572 | // This can be a list of actual protocol names, or just KIO for KDE apps. | ||
572 | const QStringList appSupportedProtocols = KIO::DesktopExecParser::supportedProtocols(_service); | 573 | const QStringList appSupportedProtocols = KIO::DesktopExecParser::supportedProtocols(_service); | ||
573 | QList<QUrl> urls(_urls); | 574 | QList<QUrl> urls(_urls); | ||
574 | if (!appSupportedProtocols.contains(QLatin1String("KIO"))) { | 575 | if (!appSupportedProtocols.contains(QLatin1String("KIO"))) { | ||
575 | for (QUrl &url : urls) { | 576 | org::kde::KIOFuse::VFS kiofuse_iface(QStringLiteral("org.kde.KIOFuse"), | ||
576 | bool supported = KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols); | 577 | QStringLiteral("/org/kde/KIOFuse"), | ||
577 | //qDebug() << "Looking at url=" << url << " supported=" << supported; | 578 | QDBusConnection::sessionBus()); | ||
578 | if (!supported && KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | 579 | struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; }; | ||
580 | 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… | |||||
581 | for (int i = 0; i < urls.length(); ++i) { | ||||
dfaure: count or size | |||||
582 | const QUrl url = urls[i]; | ||||
dfaure: .at(i) | |||||
583 | 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. | |||||
584 | continue; | ||||
585 | if (KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | ||||
579 | // Maybe we can resolve to a local URL? | 586 | // Maybe we can resolve to a local URL? | ||
580 | KIO::StatJob *job = KIO::mostLocalUrl(url); | 587 | 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… | |||||
581 | if (job->exec()) { // ## nasty nested event loop! | 588 | if (job->exec()) { // ## nasty nested event loop! | ||
582 | const QUrl localURL = job->mostLocalUrl(); | 589 | const QUrl localURL = job->mostLocalUrl(); | ||
583 | if (localURL != url) { | 590 | if (localURL != url) { | ||
584 | url = localURL; | 591 | urls[i] = localURL; | ||
585 | //qDebug() << "Changed to" << localURL; | 592 | //qDebug() << "Changed to" << localURL; | ||
593 | // KIOFuse doesn't work too well with mtp/gdrive. | ||||
594 | // @see https://invent.kde.org/kde/kio-fuse/issues/1 (GDrive) | ||||
595 | // @see https://invent.kde.org/kde/kio-fuse/issues/2 (MTP) | ||||
596 | // 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… | |||||
597 | } else if (url.scheme() != QLatin1String("mtp") | ||||
598 | && url.scheme() != QLatin1String("gdrive")) { | ||||
599 | // Can't convert... | ||||
600 | // 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.
| |||||
601 | requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); | ||||
602 | } | ||||
603 | } | ||||
604 | } else { | ||||
605 | requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i }); | ||||
586 | } | 606 | } | ||
587 | } | 607 | } | ||
608 | // Doesn't matter that we've blocked here to process the replies. | ||||
609 | // Main thing that we want is to send the mount requests without blocking. | ||||
610 | for (auto request : requests) { | ||||
611 | request.reply.waitForFinished(); | ||||
612 | if (!request.reply.isError()) { | ||||
613 | urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value()); | ||||
588 | } | 614 | } | ||
589 | } | 615 | } | ||
590 | } | 616 | } | ||
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… | |||||
591 | return urls; | 617 | return urls; | ||
592 | } | 618 | } | ||
593 | 619 | | |||
594 | // Helper function to make the given .desktop file executable by ensuring | 620 | // Helper function to make the given .desktop file executable by ensuring | ||
595 | // that a #!/usr/bin/env xdg-open line is added if necessary and the file has | 621 | // that a #!/usr/bin/env xdg-open line is added if necessary and the file has | ||
596 | // the +x bit set for the user. Returns false if either fails. | 622 | // the +x bit set for the user. Returns false if either fails. | ||
597 | static bool makeServiceFileExecutable(const QString &fileName, QString &errorString) | 623 | static bool makeServiceFileExecutable(const QString &fileName, QString &errorString) | ||
598 | { | 624 | { | ||
▲ Show 20 Lines • Show All 1182 Lines • Show Last 20 Lines |
Is there a reason you are not using a range based for loop here? for (const QUrl &url : urls)