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"), | ||
595 | QDBusConnection::sessionBus()); | ||||
596 | QVector<QPair<QDBusPendingReply<QString>, QUrl>> replies; | ||||
597 | QList<QUrl> parsedUrls; | ||||
598 | for (QList<QUrl>::Iterator it = urls.begin(); it != urls.end(); ++it) { | ||||
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… | |||||
599 | QUrl url = *it; | ||||
600 | // NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports | ||||
601 | // smb://) but will struggle with passwords, so only pass on the | ||||
602 | // URLs that don't have passwords. | ||||
603 | // @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/ | ||||
604 | if (KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols) | ||||
605 | && url.password().isEmpty()) | ||||
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… | |||||
606 | continue; | ||||
594 | //qDebug() << "Looking at url=" << url << " supported=" << supported; | 607 | //qDebug() << "Looking at url=" << url << " supported=" << supported; | ||
595 | if (!supported && KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | 608 | if (KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | ||
596 | // Maybe we can resolve to a local URL? | 609 | // Maybe we can resolve to a local URL? | ||
597 | KIO::StatJob *job = KIO::mostLocalUrl(url); | 610 | KIO::StatJob *job = KIO::mostLocalUrl(url); | ||
598 | if (job->exec()) { // ## nasty nested event loop! | 611 | if (job->exec()) { // ## nasty nested event loop! | ||
599 | const QUrl localURL = job->mostLocalUrl(); | 612 | const QUrl localURL = job->mostLocalUrl(); | ||
600 | if (localURL != url) { | 613 | if (localURL != url) { | ||
dfaure: count or size | |||||
601 | url = localURL; | 614 | parsedUrls.append(localURL); | ||
dfaure: .at(i) | |||||
602 | //qDebug() << "Changed to" << localURL; | 615 | //qDebug() << "Changed to" << localURL; | ||
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. | |||||
616 | // KIOFuse doesn't work too well with mtp/gdrive. | ||||
617 | // @see https://invent.kde.org/kde/kio-fuse/issues/1 (GDrive) | ||||
618 | // @see https://invent.kde.org/kde/kio-fuse/issues/2 (MTP) | ||||
619 | // 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… | |||||
620 | } else if (url.scheme() != QLatin1String("mtp") | ||||
621 | && url.scheme() != QLatin1String("gdrive")) { | ||||
622 | // Can't convert... | ||||
623 | // 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.
| |||||
624 | replies.push_back(qMakePair(kiofuse_iface.mountUrl(url.toString()), url)); | ||||
625 | } | ||||
603 | } | 626 | } | ||
627 | } else { | ||||
628 | replies.push_back(qMakePair(kiofuse_iface.mountUrl(url.toString()), url)); | ||||
604 | } | 629 | } | ||
605 | } | 630 | } | ||
631 | // Doesn't matter that we've blocked here to process the replies. | ||||
632 | // Main thing that we want is to send the mount requests without blocking. | ||||
633 | for (auto reply : replies) { | ||||
634 | reply.first.waitForFinished(); | ||||
635 | if (!reply.first.isError()) { | ||||
636 | reply.second = QUrl::fromLocalFile(reply.first.value()); | ||||
606 | } | 637 | } | ||
638 | parsedUrls.append(reply.second); | ||||
607 | } | 639 | } | ||
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… | |||||
640 | return parsedUrls; | ||||
641 | } else { | ||||
608 | return urls; | 642 | return urls; | ||
609 | } | 643 | } | ||
644 | } | ||||
610 | 645 | | |||
611 | // Helper function to make the given .desktop file executable by ensuring | 646 | // 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 | 647 | // 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. | 648 | // the +x bit set for the user. Returns false if either fails. | ||
614 | static bool makeServiceFileExecutable(const QString &fileName, QString &errorString) | 649 | static bool makeServiceFileExecutable(const QString &fileName, QString &errorString) | ||
615 | { | 650 | { | ||
616 | // Open the file and read the first two characters, check if it's | 651 | // Open the file and read the first two characters, check if it's | ||
617 | // #!. If not, create a new file, prepend appropriate lines, and copy | 652 | // #!. If not, create a new file, prepend appropriate lines, and copy | ||
▲ Show 20 Lines • Show All 1179 Lines • Show Last 20 Lines |
Is there a reason you are not using a range based for loop here? for (const QUrl &url : urls)