Differential D23384 Diff 64440 src/core/desktopexecparser.cpp

Changeset View

Standalone View

View Options

src/core/desktopexecparser.cpp

Show All 32 Lines
33#include <QFile> 33​#include <QFile>
34#include <QDir> 34​#include <QDir>
35#include <QUrl> 35​#include <QUrl>
36#include <QStandardPaths> 36​#include <QStandardPaths>
37 37​
38#include <config-kiocore.h> // CMAKE_INSTALL_FULL_LIBEXECDIR_KF5 38​#include <config-kiocore.h> // CMAKE_INSTALL_FULL_LIBEXECDIR_KF5
39 39​
40#include "kiocoredebug.h" 40​#include "kiocoredebug.h"
41​#include <QDBusInterface>
42​#include <QDBusReply>
41 43​
42class KRunMX1 : public KMacroExpanderBase 44​class KRunMX1 : public KMacroExpanderBase
43{ 45​{
44public: 46​public:
45 explicit KRunMX1(const KService &_service) 47​ explicit KRunMX1(const KService &_service)
46 : KMacroExpanderBase(QLatin1Char('%')) 48​ : KMacroExpanderBase(QLatin1Char('%'))
47 , hasUrls(false) 49​ , hasUrls(false)
48 , hasSpec(false) 50​ , hasSpec(false)
▲ Show 20 Lines • Show All 251 Lines • ▼ Show 20 Line(s)300 if (d->tempFiles && !appHasTempFileOption && d->urls.size()) {
300 if (!d->suggestedFileName.isEmpty()) { 302​ if (!d->suggestedFileName.isEmpty()) {
301 result << QStringLiteral("--suggestedfilename"); 303​ result << QStringLiteral("--suggestedfilename");
302 result << d->suggestedFileName; 304​ result << d->suggestedFileName;
303 } 305​ }
304 result += QUrl::toStringList(d->urls); 306​ result += QUrl::toStringList(d->urls);
305 return result; 307​ return result;
306 } 308​ }
307 309​
308 // Check if we need kioexec 310​ // Check if we need kioexec
dfaureUnsubmitted
Done

, or KIOFuse

dfaure: `, or KIOFuse`
309 bool useKioexec = false; 311​ bool useKioexec = false;
310 if (!mx1.hasUrls) { 312​ if (!mx1.hasUrls) {
311 Q_FOREACH (const QUrl &url, d->urls) 313​ for(QUrl &url : d->urls) {
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)
sitterUnsubmitted
Done

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

sitter: align arguments with first argument. same in krun.cpp.
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…
312 if (!url.isLocalFile() && !hasSchemeHandler(url)) { 314​ if (!url.isLocalFile() && !hasSchemeHandler(url)) {
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]
315​ // Lets try a KIOFuse mount instead.
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.
316​ QDBusInterface kiofused(QStringLiteral("org.kde.kded5"),
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());`
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.
317​ QStringLiteral("/modules/kiofuse"),
318​ QStringLiteral("org.kde.KIOFuse"),
319​ QDBusConnection::sessionBus());
320​ if (!kiofused.isValid() || kiofused.lastError().isValid()) {
313 useKioexec = true; 321​ useKioexec = true;
314 //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec"; 322​ //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec";
315 break; 323​ break;
316 } 324​ }
325​ QDBusReply<QString> reply = kiofused.call(QStringLiteral("mountUrl"), url.toString());
326​ // Empty string means mounting failed...
327​ if(reply.isValid() && !reply.value().isEmpty()) {
328​ url = QUrl::fromLocalFile(reply.value());
329​ } else {
330​ useKioexec = true;
331​ //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec";
332​ break;
333​ }
334​ }
335​ }
317 } else { // app claims to support %u/%U, check which protocols 336​ } else { // app claims to support %u/%U, check which protocols
318 QStringList appSupportedProtocols = supportedProtocols(d->service); 337​ QStringList appSupportedProtocols = supportedProtocols(d->service);
319 Q_FOREACH (const QUrl &url, d->urls) { 338​ for (QUrl &url : d->urls) {
320 if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) { 339​ if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) {
340​ // Lets try a KIOFuse mount instead.
341​ QDBusInterface kiofused(QStringLiteral("org.kde.kded5"),
dfaureUnsubmitted
Not Done

s/length/count/

dfaure: s/length/count/
342​ QStringLiteral("/modules/kiofuse"),
343​ QStringLiteral("org.kde.KIOFuse"),
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…
344​ QDBusConnection::sessionBus());
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
345​ if (!kiofused.isValid() || kiofused.lastError().isValid()) {
321 useKioexec = true; 346​ useKioexec = true;
322 //qCDebug(KIO_CORE) << "application does not support url, using kioexec:" << url; 347​ //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec";
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…
323 break; 348​ break;
324 } 349​ }
dfaureUnsubmitted
Not Done

coding style: missing space after "if"

dfaure: coding style: missing space after "if"
350​ QDBusReply<QString> reply = kiofused.call(QStringLiteral("mountUrl"), url.toString());
351​ // Empty string means mounting failed...
352​ if(reply.isValid() && !reply.value().isEmpty()) {
353​ url = QUrl::fromLocalFile(reply.value());
354​ } else {
355​ useKioexec = true;
356​ //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec";
357​ break;
358​ }
359​ }
325 } 360​ }
326 } 361​ }
327 if (useKioexec) { 362​ if (useKioexec) {
328 // We need to run the app through kioexec 363​ // We need to run the app through kioexec
329 result << kioexecPath(); 364​ result << kioexecPath();
330 if (d->tempFiles) { 365​ if (d->tempFiles) {
dfaureUnsubmitted
Done

auto &request to avoid copying

dfaure: `auto &request` to avoid copying
331 result << QStringLiteral("--tempfiles"); 366​ result << QStringLiteral("--tempfiles");
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…
332 } 367​ }
333 if (!d->suggestedFileName.isEmpty()) { 368​ if (!d->suggestedFileName.isEmpty()) {
334 result << QStringLiteral("--suggestedfilename"); 369​ result << QStringLiteral("--suggestedfilename");
335 result << d->suggestedFileName; 370​ result << d->suggestedFileName;
336 } 371​ }
337 result << exec; 372​ result << exec;
338 result += QUrl::toStringList(d->urls); 373​ result += QUrl::toStringList(d->urls);
339 return result; 374​ return result;
340 } 375​ }
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…
341 376​
342 if (appHasTempFileOption) { 377​ if (appHasTempFileOption) {
343 exec += QLatin1String(" --tempfile"); 378​ exec += QLatin1String(" --tempfile");
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…
344 } 379​ }
345 380​
346 // Did the user forget to append something like '%f'? 381​ // 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?)
347 // If so, then assume that '%f' is the right choice => the application 382​ // If so, then assume that '%f' is the right choice => the application
348 // accepts only local files. 383​ // accepts only local files.
349 if (!mx1.hasSpec) { 384​ if (!mx1.hasSpec) {
350 exec += QLatin1String(" %f"); 385​ exec += QLatin1String(" %f");
351 mx2.ignFile = true; 386​ mx2.ignFile = true;
352 } 387​ }
353 388​
354 mx2.expandMacrosShellQuote(exec); // syntax was already checked, so don't check return value 389​ mx2.expandMacrosShellQuote(exec); // syntax was already checked, so don't check return value
▲ Show 20 Lines • Show All 105 Lines • Show Last 20 Lines