Changeset View
Standalone View
src/widgets/krun.cpp
Show First 20 Lines • Show All 502 Lines • ▼ Show 20 Line(s) | |||||
503 | { | 503 | { | ||
504 | // Check which protocols the application supports. | 504 | // Check which protocols the application supports. | ||
505 | // This can be a list of actual protocol names, or just KIO for KDE apps. | 505 | // This can be a list of actual protocol names, or just KIO for KDE apps. | ||
506 | QStringList appSupportedProtocols = KIO::DesktopExecParser::supportedProtocols(_service); | 506 | QStringList appSupportedProtocols = KIO::DesktopExecParser::supportedProtocols(_service); | ||
507 | QList<QUrl> urls(_urls); | 507 | QList<QUrl> urls(_urls); | ||
508 | if (!appSupportedProtocols.contains(QStringLiteral("KIO"))) { | 508 | if (!appSupportedProtocols.contains(QStringLiteral("KIO"))) { | ||
509 | for (QList<QUrl>::Iterator it = urls.begin(); it != urls.end(); ++it) { | 509 | for (QList<QUrl>::Iterator it = urls.begin(); it != urls.end(); ++it) { | ||
510 | const QUrl url = *it; | 510 | const QUrl url = *it; | ||
511 | bool supported = KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols); | 511 | // NOTE: Some non-KIO apps may support the URLs (i.e. VLC supports smb://) | ||
512 | // but will struggle with passwords. | ||||
513 | // @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/ | ||||
514 | // TODO: if protocol is supported by non-KIO app and there is no password, | ||||
515 | // don't mount over KIOFuse. | ||||
516 | //bool supported = KIO::DesktopExecParser::isProtocolInSupportedList(url, appSupportedProtocols); | ||||
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… | |||||
512 | //qDebug() << "Looking at url=" << url << " supported=" << supported; | 517 | //qDebug() << "Looking at url=" << url << " supported=" << supported; | ||
513 | if (!supported && KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | 518 | if (KProtocolInfo::protocolClass(url.scheme()) == QLatin1String(":local")) { | ||
514 | // Maybe we can resolve to a local URL? | 519 | // Maybe we can resolve to a local URL? | ||
515 | KIO::StatJob *job = KIO::mostLocalUrl(url); | 520 | KIO::StatJob *job = KIO::mostLocalUrl(url); | ||
516 | if (job->exec()) { // ## nasty nested event loop! | 521 | if (job->exec()) { // ## nasty nested event loop! | ||
517 | const QUrl localURL = job->mostLocalUrl(); | 522 | const QUrl localURL = job->mostLocalUrl(); | ||
518 | if (localURL != url) { | 523 | if (localURL != 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… | |||||
dfaure: count or size | |||||
519 | *it = localURL; | 524 | *it = localURL; | ||
dfaure: .at(i) | |||||
520 | //qDebug() << "Changed to" << localURL; | 525 | //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. | |||||
526 | } else { | ||||
527 | // Can't convert... | ||||
528 | // Lets try a KIOFuse mount instead. | ||||
529 | QDBusInterface kiofused(QStringLiteral("org.kde.kded5"), | ||||
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… | |||||
530 | QStringLiteral("/modules/kiofuse"), | ||||
531 | QStringLiteral("org.kde.KIOFuse"), | ||||
532 | QDBusConnection::sessionBus()); | ||||
533 | if (!kiofused.isValid() || kiofused.lastError().isValid()) { | ||||
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.
| |||||
534 | // Module isn't loaded... | ||||
535 | continue; | ||||
536 | } | ||||
537 | QDBusReply<QString> reply = kiofused.call(QStringLiteral("mountUrl"), url.toString()); | ||||
538 | // Empty string means mounting failed... | ||||
539 | if(reply.isValid() && !reply.value().isEmpty()) { | ||||
540 | *it = QUrl::fromLocalFile(reply.value()); | ||||
541 | } | ||||
521 | } | 542 | } | ||
522 | } | 543 | } | ||
544 | } else { | ||||
545 | // It's a remote url, lets try a KIOFuse mount. | ||||
546 | QDBusInterface kiofused(QStringLiteral("org.kde.kded5"), | ||||
547 | QStringLiteral("/modules/kiofuse"), | ||||
548 | QStringLiteral("org.kde.KIOFuse"), | ||||
549 | QDBusConnection::sessionBus()); | ||||
550 | if (!kiofused.isValid() || kiofused.lastError().isValid()) { | ||||
551 | // Module isn't loaded... | ||||
552 | continue; | ||||
553 | } | ||||
554 | QDBusReply<QString> reply = kiofused.call(QStringLiteral("mountUrl"), url.toString()); | ||||
555 | // Empty string means mounting failed... | ||||
556 | if(reply.isValid() && !reply.value().isEmpty()) { | ||||
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… | |||||
557 | *it = QUrl::fromLocalFile(reply.value()); | ||||
558 | } | ||||
523 | } | 559 | } | ||
524 | } | 560 | } | ||
525 | } | 561 | } | ||
526 | return urls; | 562 | return urls; | ||
527 | } | 563 | } | ||
528 | 564 | | |||
529 | // Simple QDialog that resizes the given text edit after being shown to more | 565 | // Simple QDialog that resizes the given text edit after being shown to more | ||
530 | // or less fit the enclosed text. | 566 | // or less fit the enclosed text. | ||
▲ Show 20 Lines • Show All 1151 Lines • Show Last 20 Lines |
Is there a reason you are not using a range based for loop here? for (const QUrl &url : urls)