Differential D29385

Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun
ClosedPublic
Actions

Authored by dfaure on May 3 2020, 2:39 PM.

Details

Reviewers
ahmadsamir
broulik
meven
kossebau
davidedmundson
nicolasfella
svuorela
Commits
R241:b2b10ee69cce: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun
Summary
  • KRun::runUrl() is now new OpenUrlJob(url, mimeType).
  • new KRun() is now OpenUrlJob(url), which will first determine the mimeType.
  • the "open with" dialog is provided by KIOWidgets via JobUiDelegate and an interface defined in kiogui. When not using JobUiDelegate, we'll fall back to QDesktopServices::openUrl, which will call xdg-open, which will call kde-open5, which will prompt an open with dialog :-)
  • Running of scripts and executables is off by default, unlike in KRun, since this created a few security bugs in unsuspecting applications in the past. File managers should enable it, apps that can open attachments or similar things should leave it disabled.
  • Instead of BrowserRun/KonqRun deriving from KRun, they can use an OpenUrlJob as is, connect to mimeTypeFound and kill the job if they want to take over (to embed the document instead of launching an app)

I tried to unittest OpenUrlJob as much as possible. The parts that are
least unittests are those related to remote protocols though, since they
require test servers.

Test Plan

Mostly via unittests, for now

Diff Detail

Repository
R241 KIO
Branch
2020_05_openurljob
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 26424
Build 26442: arc lint + arc unit
dfaure created this revision.May 3 2020, 2:39 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 3 2020, 2:39 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.May 3 2020, 2:39 PM
kossebau added a comment.May 3 2020, 3:02 PM

Quick first nitpicks from initial scan what this patch is about, without having looked closer.

autotests/openurljobtest.cpp
109

why no simple range-based for loop?

src/gui/openurljob.h
74

Makes me wonder if those runflags should not be rather part of KIO namespace, to decouple classes here.

97

Could this and the following bool flags be turned into some flags instead? Just wondering, not looked further.

Also wondering if there should not be some getter as well, when using a flagset that would be just a single method/symbol.

128

-> "mimeTypeFound"? would match other casing.

src/gui/openurljobhandlerinterface.cpp
35

= default; instead?

src/gui/openurljobhandlerinterface.h
34

Please prepend a line

* @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h <KIO/OpenUrlJobHandlerInterface>

at the top, to trick doxygen into documenting the full CamelCase include.

55

override instead?

85

Prevent use of nested Private class here, declare a class OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for consistency and preparedness in case there ever is an actual object.

src/widgets/widgetsopenurljobhandler.h
46

Do not use embedded Private class. Also pimp needed here, given not a public class?

svuorela added a comment.May 3 2020, 5:46 PM

I've been looking at it for a while, and after trying to decipher the long lambdas, which might have been better as named functions, I think it is, beside Kossebau's comments, fine.

dfaure marked 5 inline comments as done.May 3 2020, 6:00 PM
In D29385#662336, @svuorela wrote:

I've been looking at it for a while, and after trying to decipher the long lambdas, which might have been better as named functions, I think it is, beside Kossebau's comments, fine.

Interesting comment. I thought it was actually more readable to keep that code together, rather than having to jump around in the file to find the right slots.

autotests/openurljobtest.cpp
109

Good point, copy/pasted from another (old) test.

src/gui/openurljob.h
74

Interesting point, let's discuss it quickly (because if we want to change that, I need to redo the kio RC1 tag for 5.70, which has ApplicationLauncherJob).

But I'm not sure we should do that, anyway.

On hindsight, I would rather change this one to be setTemporaryFiles(bool b);

I don't like all the flag-conversion hell that we end up with otherwise. Stuff like

RunFlags flags = tempFile ? KRun::DeleteTemporaryFiles : RunFlags();
if (runExecutables) {
    flags |= KRun::RunExecutables;
}

The input data, all the way up, is for sure not in the form of a set of job flags.
So I find it more readable to have code like setSomething(conditionForSomething)
than to have a bunch of flag manipulation code.

97

With this one in particular, since it's on by default, we'd have to either have DefaultRunFlags that includes RunExecutables --- or we'd have to make the flag DoNotRunExecutables.
I don't like either....

Technically yes it could all be flags, but I'd much rather have independent setters.

The TempFile flag is a good example why: if it's shared with ApplicationLauncherJob by being in the KIO namespace, does that mean we also put all of those in there? But then people can write things that don't make sense like ApplicationLauncherJob *job = ...; job->setFlags(DoNotRunExecutables). What? That flag doesn't apply to that job. If there's one thing that job does, it's to run executables :-) [but via desktop files, while that flag is actually about what OpenUrlJob should do when opening an executable directly].

Sharing flags between jobs who need a different set, is a problem. It ends up with docu like "only these flags make sense here". Not great.

I don't see a use case for getters, but we can certainly add them (either now or as needed).

128

Well spotted, thanks.

src/gui/openurljobhandlerinterface.h
34

Ah! I was wondering what that was about...

85

LOL I was the one arguing against nested Private classes recently. Looks like I forgot that when copy/pasting here. Thanks!

Hmm note that using QScopedPointer requires actually defining (in the .cpp) a dummy private class, even if there's no "new" anywhere (generated code wants to delete it).

dfaure updated this revision to Diff 81814.May 3 2020, 6:00 PM
dfaure marked 2 inline comments as done.

Adjusments based on Friedrich's initial review

broulik added a comment.May 3 2020, 6:33 PM

Cool!

src/gui/openurljob.cpp
262

I know what you always say when I say what I always say, but why not just always stat/mimetype job?

275

I know what you always say when I say what I always say but can we use a mimetype job here? :)

306

Using an early return here would make the code less nested

448

was or should be?

507

While at it, can we clean up/unify those texts? Sometimes it puts the file on a new line, sometimes it's bold, sometines in quotes, etc. Generally I wouldn't really want any HTML formatting in there.

broulik added inline comments.May 3 2020, 6:34 PM
src/gui/openurljob.cpp
275

Seems I forgot to remove this when I added the comment above instead :)

feverfew added a subscriber: feverfew.May 3 2020, 7:51 PM

Quick question, how does this affect D23384? Previously KRun used KIO::DesktopExecParser::resultingArguments() which handled the conversion of URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

dfaure added a comment.May 3 2020, 8:02 PM
In D29385#662422, @feverfew wrote:

Quick question, how does this affect D23384? Previously KRun used KIO::DesktopExecParser::resultingArguments() which handled the conversion of URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

No worries, this should still happen.
KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob and ApplicationLauncherJob. The last two delegate the work to an internal class, KProcessRunner (like KRun did).
So (to roll out the common case of a document-type file), once OpenUrlJob finds out which application to start, it calls ApplicationLauncherJob, which creates a KProcessRunner, which uses KIO::DesktopExecParser::resultingArguments().

feverfew added a comment.May 3 2020, 8:08 PM
In D29385#662435, @dfaure wrote:
In D29385#662422, @feverfew wrote:

Quick question, how does this affect D23384? Previously KRun used KIO::DesktopExecParser::resultingArguments() which handled the conversion of URLs to local KIOFuse URLs if needed, but now I believe this new API doesn't?

No worries, this should still happen.
KRun is actually split into three classes: OpenUrlJob, CommandLauncherJob and ApplicationLauncherJob. The last two delegate the work to an internal class, KProcessRunner (like KRun did).
So (to roll out the common case of a document-type file), once OpenUrlJob finds out which application to start, it calls ApplicationLauncherJob, which creates a KProcessRunner, which uses KIO::DesktopExecParser::resultingArguments().

Ahhh yes correct. Seeming as these jobs are now async (unlike KRun IIRC?), it isn't a problem that resultingArguments makes several blocking calls to the KIOFuse daemon?

dfaure added a comment.May 3 2020, 8:37 PM
In D29385#662439, @feverfew wrote:

Ahhh yes correct. Seeming as these jobs are now async (unlike KRun IIRC?), it isn't a problem that resultingArguments makes several blocking calls to the KIOFuse daemon?

Some of KRun was async too ("new KRun" was async, the static methods like runUrl and others were not).
The blocking calls in resultingArguments() don't make things worse for OpenUrlJob than it was for KRun, but of course the fact that the job is async is an opportunity to do everything async.
The problem is that we can't get rid of KRun just yet....

Async on top of sync works -- but still blocks the GUI thread so it's not perfect. Sync on top of async requires awful eventloop hacks which are often the source of nasty bugs.

Solution 1: we write an async version of DesktopExecParser, keeping the existing one for KRun.
Solution 2: we wait until KRun is gone (KF6) to make DesktopExecParser async.

(I'm skipping non-solutions like event loop hacks in KRun or using the fact that nobody seems to really care for the PIDs or "bool success" returned by KRun so we could just make KRun "fire and forget" the underlying jobs and always return success).

Basically this means rewriting DesktopExecParser as a KJob. Are you up for it? ;-)
I can provide guidance, but I really care about FUSE enough myself to test a rewrite if I'd have to write it.
Or I write, you test ;)

dfaure marked 2 inline comments as done.May 3 2020, 11:11 PM
dfaure added inline comments.
src/gui/openurljob.cpp
262

LOL we're like an old couple, the explicit discussion doesn't actually need to happen anymore ;)

OK for everyone else, we're debating whether it's ok to use blocking local-file I/O like QFile or QMimeDatabase which reads from the file.

Of course less code paths is a good thing for maintenance, but it seems *so* overkill to make two calls to a kioslave just to find out the mimetype of a file.... My main counter argument is performance.

For the whole OpenUrlJob until the mimetype is found:

With KIO

RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
 0.29 msecs per iteration (total: 75, iterations: 256)

With the local-file optimization

RESULT : OpenUrlJobTest::takeOverAfterMimeTypeFound():
 0.0986 msecs per iteration (total: 101, iterations: 1024)

That's 3 times faster. Admittedly this is not the kind of things people do in a loop.

Well, OK, if nobody objects I can remove the local-files fast paths.
0.2ms is nothing when lharming 2018 PG Demi Lovato Ashley Tisdale Avril Lavigne-02052020.mpgaunching an app, or even when opening a URL in a browser.

[More context: QMimeDatabase *might* determine the mimetype from just the extension, in which case no I/O happens and we could do that here, or it might need to look into the contents of the file. We can ask it to not do that but then the mimetype determination will be less good, for some mimetypes; and we can't ask it if we would get better information by looking at content, so there's no way to split up the work between here and the kioslave. It's "quick search" vs "full search", not phase 1, phase 2.]

448

Hehe, see kdesktopfileactions.cpp:141 ;-)

507

Completely agree. I removed most of the HTML formatting when grabbing the error messages from KRun, looks like I forgot this one.

So now it's quotes everywhere.

But indeed sometimes the filename is on its own line. Should I just use quotes everywhere?

I guess the problem is when the filename (with full path) is very long.

dfaure updated this revision to Diff 81830.May 3 2020, 11:12 PM
dfaure marked an inline comment as done.

Multiple fixes thanks to review comments; didn't remove the fast path yet (a unittest needs fixup)

dfaure updated this revision to Diff 81840.May 4 2020, 8:02 AM

Remove local-files fast path.

This also showed a small mistake in the handling of error texts
(this job isn't a KIO::Job so we need to call buildErrorString ourselves).

dfaure updated this revision to Diff 82058.May 6 2020, 7:43 AM

-void KIO::OpenUrlJob::setRunFlags(KIO::ApplicationLauncherJob::RunFlags runFlags)
+void KIO::OpenUrlJob::setDeleteTemporaryFile(bool b)

The more I think about it, the least I like the use of flags here.

  1. they are from the wrong class as Kai-Uwe pointed out, but more importantly:
  2. the other bool setters here are for unrelated concerns, better keep them separate.

Are we doing lineEdit->setDragEnabled(true); lineEdit->setClearButtonEnabled(true); lineEdit->setReadOnly(true);
or are we doing lineEdit->setFlags(QLineEdit::DragEnabled | QLineEdit::ClearButtonEnabled | QLineEdit::ReadOnly)?

broulik accepted this revision.May 8 2020, 8:28 AM
This revision is now accepted and ready to land.May 8 2020, 8:28 AM
dfaure closed this revision.May 8 2020, 8:31 AM
ahmadsamir added a comment.May 8 2020, 11:07 AM
In D29385#664552, @dfaure wrote:

-void KIO::OpenUrlJob::setRunFlags(KIO::ApplicationLauncherJob::RunFlags runFlags)
+void KIO::OpenUrlJob::setDeleteTemporaryFile(bool b)

The more I think about it, the least I like the use of flags here.

  1. they are from the wrong class as Kai-Uwe pointed out, but more importantly:
  2. the other bool setters here are for unrelated concerns, better keep them separate.

    Are we doing lineEdit->setDragEnabled(true); lineEdit->setClearButtonEnabled(true); lineEdit->setReadOnly(true); or are we doing lineEdit->setFlags(QLineEdit::DragEnabled | QLineEdit::ClearButtonEnabled | QLineEdit::ReadOnly)?

I vote:
lineEdit->setDragEnabled(true); lineEdit->setClearButtonEnabled(true); lineEdit->setReadOnly(true);
it's:

  • more readable, and easier to use (similar methods are used through out Qt code)
  • avoids the case you talked about, of the user setting nonsensical, from the code POV, flags.

I was too slow working my way through this short story^W^W review, so I've gathered my other comments (mostly code style, docs changes...etc) in a diff, that I'll submit shortly.

ahmadsamir added inline comments.May 15 2020, 6:21 PM
src/gui/openurljob.cpp
262

OK for everyone else, we're debating whether it's ok to use blocking local-file I/O like QFile or QMimeDatabase which reads from the file.

I know this ship has sailed (well, sunk in this case :)), but if it's 3 times faster to use QFile, then is it really a "blocking I/O" operation? it's too fast to be "blocking"...

dfaure added a comment.May 15 2020, 7:11 PM

It's 3 times faster on my local SSD.

Now think of a NFS mount on a server from another country....

ahmadsamir added a comment.May 15 2020, 7:16 PM
In D29385#671980, @dfaure wrote:

It's 3 times faster on my local SSD.

Now think of a NFS mount on a server from another country....

I was thinking mostly of QFile when url.isLocalFile() is true, but yeah I see your point :)

dfaure added a comment.May 15 2020, 7:22 PM

That's the point, a NFS mount *is* a local URL, so we do use QFile for it. And then it takes forever because the kernel has to talk over a socket to answer us.
Yes this is horrendous. I hate network mounts :-)

ahmadsamir added a comment.May 15 2020, 7:35 PM
In D29385#671987, @dfaure wrote:

That's the point, a NFS mount *is* a local URL, so we do use QFile for it. And then it takes forever because the kernel has to talk over a socket to answer us.
Yes this is horrendous. I hate network mounts :-)

I don't have *any* network mounts, never have, so I wasn't aware of that. Yep, that is horrendous.

Revision Contents
Changeset List

Diff 82058

View Options

autotests/CMakeLists.txt

Loading...
View Options

autotests/krununittest.cpp

Loading...
View Options

autotests/openurljobtest.h

Loading...
View Options

autotests/openurljobtest.cpp

Loading...
View Options

src/core/kurlauthorized.h

Loading...
View Options

src/gui/CMakeLists.txt

Loading...
View Options

src/gui/applicationlauncherjob.h

Loading...
View Options

src/gui/applicationlauncherjob.cpp

Loading...
View Options

src/gui/openurljob.h

Loading...
View Options

src/gui/openurljob.cpp

Loading...
View Options

src/gui/openurljobhandlerinterface.h

Loading...
View Options

src/gui/openurljobhandlerinterface.cpp

Loading...
View Options

src/widgets/CMakeLists.txt

Loading...
View Options

src/widgets/jobuidelegate.cpp

Loading...
View Options

src/widgets/kdesktopfileactions.cpp

Loading...
View Options

src/widgets/kopenwithdialog.h

Loading...
View Options

src/widgets/kopenwithdialog.cpp

Loading...
View Options

src/widgets/krun.cpp

Loading...
View Options

src/widgets/widgetsopenurljobhandler.h

Loading...
View Options

src/widgets/widgetsopenurljobhandler.cpp

Loading...