Differential D23384 Diff 72637 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 "kiofuse_interface.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, or KIOFuse
dfaureUnsubmitted
Done

, or KIOFuse

dfaure: `, or KIOFuse`
309 bool useKioexec = false; 312​ bool useKioexec = false;
313​ org::kde::KIOFuse::VFS 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());`
318​ requests.reserve(d->urls.count());
319​ QStringList appSupportedProtocols = supportedProtocols(d->service);
310 if (!mx1.hasUrls) { 320​ if (!mx1.hasUrls) {
311 for (const QUrl &url : qAsConst(d->urls)) { 321​ for (int i = 0; i < d->urls.count(); ++i) {
322​ const QUrl url = d->urls.at(i);
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…
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]
312 if (!url.isLocalFile() && !hasSchemeHandler(url)) { 323​ if (!url.isLocalFile() && !hasSchemeHandler(url)) {
313 useKioexec = true; 324​ // Lets try a KIOFuse mount instead.
314 //qCDebug(KIO_CORE) << "non-local files, application does not support urls, using kioexec"; 325​ requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
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; ​
316 } 326​ }
317 } 327​ }
318 } else { // app claims to support %u/%U, check which protocols 328​ } else if (!appSupportedProtocols.contains(QLatin1String("KIO"))) {
319 QStringList appSupportedProtocols = supportedProtocols(d->service); 329​ for (int i = 0; i < d->urls.count(); ++i) {
dfaureUnsubmitted
Not Done

s/length/count/

dfaure: s/length/count/
320 for (const QUrl &url : qAsConst(d->urls)) { 330​ const QUrl url = d->urls.at(i);
321 if (!isProtocolInSupportedList(url, appSupportedProtocols) && !hasSchemeHandler(url)) { 331​ const bool supported = isProtocolInSupportedList(url, appSupportedProtocols);
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…
332​ // NOTE: Some non-KIO apps may support the URLs (e.g. VLC supports smb://)
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
333​ // but will not have the password if they are not in the URL itself.
334​ // Hence convert URL to KIOFuse equivalent in case there is a password.
335​ // @see https://pointieststick.com/2018/01/17/videos-on-samba-shares/
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…
336​ // @see https://bugs.kde.org/show_bug.cgi?id=330192
337​ if (!supported || (!url.userName().isEmpty() && url.password().isEmpty())) {
dfaureUnsubmitted
Not Done

coding style: missing space after "if"

dfaure: coding style: missing space after "if"
338​ requests.push_back({ kiofuse_iface.mountUrl(url.toString()), i });
339​ }
340​ }
341​ }
342​
343​ // Doesn't matter that we've blocked here to process the replies.
344​ // Main thing that we want is to send the mount requests without blocking.
345​ for (auto &request : requests) {
dfaureUnsubmitted
Done

auto &request to avoid copying

dfaure: `auto &request` to avoid copying
346​ request.reply.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…
347​ if (request.reply.isError()) {
322 useKioexec = true; 348​ useKioexec = true;
323 //qCDebug(KIO_CORE) << "application does not support url, using kioexec:" << url; 349​ // At this point we should just send the original urls to kioexec.
350​ // There's no point sending urls to kioexec that go through kiofuse.
324 break; 351​ break;
325 } 352​ }
326 } 353​ }
327 } 354​
328 if (useKioexec) { 355​ if (useKioexec) {
329 // We need to run the app through kioexec 356​ // We need to run the app through kioexec
330 result << kioexecPath(); 357​ result << kioexecPath();
331 if (d->tempFiles) { 358​ if (d->tempFiles) {
332 result << QStringLiteral("--tempfiles"); 359​ result << QStringLiteral("--tempfiles");
333 } 360​ }
334 if (!d->suggestedFileName.isEmpty()) { 361​ if (!d->suggestedFileName.isEmpty()) {
335 result << QStringLiteral("--suggestedfilename"); 362​ result << QStringLiteral("--suggestedfilename");
336 result << d->suggestedFileName; 363​ result << d->suggestedFileName;
337 } 364​ }
338 result << exec; 365​ result << exec;
339 result += QUrl::toStringList(d->urls); 366​ result += QUrl::toStringList(d->urls);
340 return result; 367​ return result;
341 } 368​ }
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…
342 369​
370​ // At this point we know we're not using kioexec, so feel free to replace
371​ // KIO URLs with their KIOFuse local path.
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…
372​ for (const auto &request : qAsConst(requests)) {
373​ d->urls[request.urlIndex] = QUrl::fromLocalFile(request.reply.value());
374​ }
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?)
375​
343 if (appHasTempFileOption) { 376​ if (appHasTempFileOption) {
344 exec += QLatin1String(" --tempfile"); 377​ exec += QLatin1String(" --tempfile");
345 } 378​ }
346 379​
347 // Did the user forget to append something like '%f'? 380​ // Did the user forget to append something like '%f'?
348 // If so, then assume that '%f' is the right choice => the application 381​ // If so, then assume that '%f' is the right choice => the application
349 // accepts only local files. 382​ // accepts only local files.
350 if (!mx1.hasSpec) { 383​ if (!mx1.hasSpec) {
▲ Show 20 Lines • Show All 110 Lines • Show Last 20 Lines