Differential D23384 Diff 69769 src/core/desktopexecparser.cpp

Changeset View

Standalone View

View Options

src/core/desktopexecparser.cpp

Show All 14 Lines1/* This file is part of the KDE libraries
15 15​
16 You should have received a copy of the GNU Library General Public License 16​ You should have received a copy of the GNU Library General Public License
17 along with this library; see the file COPYING.LIB. If not, write to 17​ along with this library; see the file COPYING.LIB. If not, write to
18 the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, 18​ the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
19 Boston, MA 02110-1301, USA. 19​ Boston, MA 02110-1301, USA.
20*/ 20​*/
21 21​
22#include "desktopexecparser.h" 22​#include "desktopexecparser.h"
23​#include "kiofuseinterface.h"
23 24​
24#include <kmacroexpander.h> 25​#include <kmacroexpander.h>
25#include <kshell.h> 26​#include <kshell.h>
26#include <ksharedconfig.h> 27​#include <ksharedconfig.h>
27#include <kdesktopfile.h> 28​#include <kdesktopfile.h>
28#include <kservice.h> 29​#include <kservice.h>
29#include <kconfiggroup.h> 30​#include <kconfiggroup.h>
30#include <kprotocolinfo.h> 31​#include <kprotocolinfo.h>
31#include <kmimetypetrader.h> 32​#include <kmimetypetrader.h>
32 33​
33#include <QFile> 34​#include <QFile>
34#include <QDir> 35​#include <QDir>
35#include <QUrl> 36​#include <QUrl>
36#include <QStandardPaths> 37​#include <QStandardPaths>
38​#include <QDBusConnection>
39​#include <QDBusReply>
37 40​
38#include <config-kiocore.h> // CMAKE_INSTALL_FULL_LIBEXECDIR_KF5 41​#include <config-kiocore.h> // CMAKE_INSTALL_FULL_LIBEXECDIR_KF5
39 42​
40#include "kiocoredebug.h" 43​#include "kiocoredebug.h"
41 44​
42class KRunMX1 : public KMacroExpanderBase 45​class KRunMX1 : public KMacroExpanderBase
43{ 46​{
44public: 47​public:
▲ Show 20 Lines • Show All 255 Lines • ▼ Show 20 Line(s)301 if (d->tempFiles && !appHasTempFileOption && d->urls.size()) {
300 if (!d->suggestedFileName.isEmpty()) { 303​ if (!d->suggestedFileName.isEmpty()) {
301 result << QStringLiteral("--suggestedfilename"); 304​ result << QStringLiteral("--suggestedfilename");
302 result << d->suggestedFileName; 305​ result << d->suggestedFileName;
303 } 306​ }
304 result += QUrl::toStringList(d->urls); 307​ result += QUrl::toStringList(d->urls);
305 return result; 308​ return result;
306 } 309​ }
307 310​
308 // Check if we need kioexec 311​ // Check if we need kioexec
dfaureUnsubmitted
Done

, or KIOFuse

dfaure: `, or KIOFuse`
309 bool useKioexec = false; 312​ bool useKioexec = false;
313​ org::kde::KIOFuse kiofuse_iface(QStringLiteral("org.kde.KIOFuse"),
314​ QStringLiteral("/org/kde/KIOFuse"),
sitterUnsubmitted
Done

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

sitter: align arguments with first argument. same in krun.cpp.
315​ QDBusConnection::sessionBus());
316​ struct MountRequest { QDBusPendingReply<QString> reply; int urlIndex; };
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.
317​ QVector<MountRequest> requests;
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());`
310 if (!mx1.hasUrls) { 318​ if (!mx1.hasUrls) {
311 Q_FOREACH (const QUrl &url, d->urls) 319​ 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)
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)) { 320​ 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]
313 useKioexec = true; 321​ // Lets try a KIOFuse mount instead.
314 //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec"; 322​ requests.push_back(qMakePair(kiofuse_iface.mountUrl(url.toString()), url));
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.
315 break; 323​ } else {
324​ parsedUrls.append(url);
325​ }
316 } 326​ }
317 } else { // app claims to support %u/%U, check which protocols 327​ } else { // app claims to support %u/%U, check which protocols
318 QStringList appSupportedProtocols = supportedProtocols(d->service); 328​ QStringList appSupportedProtocols = supportedProtocols(d->service);
319 Q_FOREACH (const QUrl &url, d->urls) { 329​ for (QUrl &url : d->urls) {
320 if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) { 330​ if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) {
331​ if (url.scheme() == QLatin1String("mtp")
332​ || url.scheme() == QLatin1String("gdrive")
dfaureUnsubmitted
Not Done

s/length/count/

dfaure: s/length/count/
333​ ) {
334​ // KIOFuse doesn't work too well with mtp/gdrive.
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…
335​ // @see https://invent.kde.org/kde/kio-fuse/issues/1 (GDrive)
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
336​ // @see https://invent.kde.org/kde/kio-fuse/issues/2 (MTP)
337​ // This can be removed once those two issues are fixed.
321 useKioexec = true; 338​ useKioexec = true;
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…
322 //qCDebug(KIO_CORE) << "application does not support url, using kioexec:" << url; ​
323 break; 339​ break;
324 } 340​ }
dfaureUnsubmitted
Not Done

coding style: missing space after "if"

dfaure: coding style: missing space after "if"
341​ // Lets try a KIOFuse mount instead.
342​ requests.push_back(qMakePair(kiofuse_iface.mountUrl(url.toString()), url));
343​ } else {
344​ parsedUrls.append(url);
345​ }
346​ }
325 } 347​ }
348​
349​ // Doesn't matter that we've blocked here to process the replies.
350​ // Main thing that we want is to send the mount requests without blocking.
351​ for (auto reply : requests) {
dfaureUnsubmitted
Done

auto &request to avoid copying

dfaure: `auto &request` to avoid copying
352​ reply.first.waitForFinished();
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…
353​ if (reply.first.isError()) {
354​ useKioexec = true;
355​ // At this point we should just send the original urls to kioexec.
356​ // There's no point sending urls to kioexec that go through kiofuse.
357​ break;
358​ } else {
359​ reply.second = QUrl::fromLocalFile(reply.first.value());
326 } 360​ }
361​ parsedUrls.append(reply.second);
362​ }
363​
327 if (useKioexec) { 364​ if (useKioexec) {
328 // We need to run the app through kioexec 365​ // We need to run the app through kioexec
329 result << kioexecPath(); 366​ result << kioexecPath();
330 if (d->tempFiles) { 367​ if (d->tempFiles) {
331 result << QStringLiteral("--tempfiles"); 368​ result << QStringLiteral("--tempfiles");
332 } 369​ }
333 if (!d->suggestedFileName.isEmpty()) { 370​ if (!d->suggestedFileName.isEmpty()) {
334 result << QStringLiteral("--suggestedfilename"); 371​ result << QStringLiteral("--suggestedfilename");
335 result << d->suggestedFileName; 372​ result << d->suggestedFileName;
336 } 373​ }
337 result << exec; 374​ result << exec;
338 result += QUrl::toStringList(d->urls); 375​ result += QUrl::toStringList(d->urls);
339 return result; 376​ return result;
377​ } else {
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…
378​ d->urls = parsedUrls;
340 } 379​ }
341 380​
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…
342 if (appHasTempFileOption) { 381​ if (appHasTempFileOption) {
343 exec += QLatin1String(" --tempfile"); 382​ exec += QLatin1String(" --tempfile");
344 } 383​ }
345 384​
346 // Did the user forget to append something like '%f'? 385​ // 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 386​ // If so, then assume that '%f' is the right choice => the application
348 // accepts only local files. 387​ // accepts only local files.
349 if (!mx1.hasSpec) { 388​ if (!mx1.hasSpec) {
350 exec += QLatin1String(" %f"); 389​ exec += QLatin1String(" %f");
351 mx2.ignFile = true; 390​ mx2.ignFile = true;
352 } 391​ }
353 392​
354 mx2.expandMacrosShellQuote(exec); // syntax was already checked, so don't check return value 393​ mx2.expandMacrosShellQuote(exec); // syntax was already checked, so don't check return value
▲ Show 20 Lines • Show All 105 Lines • Show Last 20 Lines