New class ProcessLauncherJob in KIOGui
ClosedPublic

Authored by dfaure on Mar 13 2020, 11:02 AM.

Details

Summary

This is meant to replace KRun::runApplication/runService,
without the QWidget dependency, for most use cases. One exception:
applications who want to allow the user to make desktop files executable
(this requires messageboxes, and therefore is still in KRun).

The code is mostly based on KRun's internal KProcessRunner class,
now moved to KIOGui, but still private.

Next step: also routing KRun::runCommand via ProcessLauncherJob,
and then writing OpenUrlJob on top of ProcessLauncherJob (but
runUrl calling KOpenWithDialog is a problem in KIOGui...).

Test Plan

see new unittest (which is based on krununittest)

Diff Detail

Repository
R241 KIO
Branch
2020_ProcessLauncherJob
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23725
Build 23743: arc lint + arc unit
dfaure created this revision.Mar 13 2020, 11:02 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 13 2020, 11:02 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Mar 13 2020, 11:02 AM

From the POV of the task at hand, it's great.

If we are making new public API I have some minor requests for things we want in future.

src/gui/kprocessrunner.cpp
40

WId as an int is problematic for wayland.

Can we do QWindow*? it'll allow adding support in future.

For the compatibility path we can loop through QApp->windows to find the object from windowId

src/gui/processlauncherjob.h
112

I don't like that this has to be available immediately after start()

it means we can't make all the blocking calls for kiofuse/discrete gpus/whatever async.

which would be really nice for the future.

can we have a signal
started();

and it's valid after that?

or...alternatively have ProcessLaunchJob::finished() come after processStarted, instead of when the process ends? and this is valid when the job completes? From a plasma POV I just want to know if kate started ok, but I don't care if it crashes later.

dfaure planned changes to this revision.Mar 15 2020, 9:45 AM
dfaure added inline comments.
src/gui/kprocessrunner.cpp
40

Actually if D28016 is approved, I can remove the windowId altogether.

src/gui/processlauncherjob.h
112

This is an excellent point. This keeps bothering me too.

One issue is that in order to use this from KRun I needed the PID immediately.
But maybe we can add a blocking method (with QProcess::waitForStarted, no event loop there) specifically for KRun, until KF6.

Re job completion, I agree that apps only care about starting the process.
The idea of watching for exit code to detect non-existing executable was only a small optimization in 2000 (commit 15bd0141a63f244bdd6f9), let's move this around and check before hand, it'll be much simpler.

dfaure updated this revision to Diff 77682.Mar 15 2020, 9:56 PM
  • Don't block in start(), make it fully async
  • Add waitForStarted() for KRun (with unittests)
  • Add test for non-existing executables, with and without kioexec
    • after making sure that the command isn't trivial, by starting with cd KIO::DesktopExecParser helps, though, it returns /bin/sh as such a case So we don't have to do this when the process finishes, we can check it upfront
  • Connect to QProcess::errorOccurred just for debugging (debug output in case the process crashes)
davidedmundson accepted this revision.Mar 18 2020, 5:36 PM
davidedmundson added inline comments.
src/gui/kprocessrunner.cpp
194

It's the DBus calls that come before start that I want to get async, not the tiny bit between fork and the child process exec()'ing.

Obviously we can do that piecemeal later, and it isn't a reason to delay this.

I'm not sure how much of that we'll be able to get both async and into this "waitForStarted" pattern without event loops, but worst case we can do it for KF6. It's all a bit frustrating as FWICT no-one even uses the returned PID.

src/widgets/krun.cpp
490–491

or we can just ignore it if we're phasing out klauncher anyway?

This revision is now accepted and ready to land.Mar 18 2020, 5:36 PM
dfaure added inline comments.Mar 18 2020, 8:14 PM
src/gui/kprocessrunner.cpp
194

Ah you mean inside DesktopExecParser? I thought about that, but it's easier to fix later when the other users of DesktopExecParser (which need sync) don't exist anymore.
Otherwise we need to fork it into an async variant, not sure I'm motivated for that myself (especially for FUSE stuff...).

Yes, waitForStarted will also disappear in a pure-async KF6 world for this class.

And at least until KF6 we can port all apps to ProcessLauncherJob already.

Interesting point about the PID being useless, I never thought that this might be the case. But indeed, why would one need this...

src/widgets/krun.cpp
490–491

Right. Separate issue though. I like incremental steps.

dfaure closed this revision.Mar 18 2020, 8:16 PM

Ah there was still the WId question. I'll remove it before anyone starts using this API.

We can always add a setter later if we want to (but if it's just for the unused setLaunchedBy, I'm not convinced...)

Ah there was still the WId question. I'll remove it before anyone starts using this API.

We can always add a setter later if we want to (but if it's just for the unused setLaunchedBy, I'm not convinced...)

For wayland (which currently lacks a spec for this!) I will probably end up needing a QWindow

But as you say, we can always add a setter later.

Sorry for using this diff to ask this question, I couldn't find you in kde-devel.
Would it be possible to expose the finished signal of QProcess in KIO::ApplicationLauncherJob?
I need something like this to close the startup feedback in the Plasma Mobile shell (a fullscreen overlay which shows that an app is starting) when the app crashed.
Or is there an entirely different way to implement this?

Would it be possible to expose the finished signal of QProcess in KIO::ApplicationLauncherJob?

By design the job finishes before the subprocess finishes (which could be in 3 days, if the user keeps the window open that long).
Also this would only help you for subprocesses started by plasma itself, but not for the case where application A starts application B.

I need something like this to close the startup feedback in the Plasma Mobile shell (a fullscreen overlay which shows that an app is starting) when the app crashed.

This is exactly what the code in KProcessRunner::terminateStartupNotification is about.
If you're not on X11, then this code needs to be extended to notify plasma another way about terminating startup notification.
I'm no expert on non-X11 startup notification. If there's a cross-desktop spec then it could be used here. Otherwise, I'm OK with a plasma-specific DBus call in there, it won't harm other environments.

By doing it there, it will work from any application. It's "apps talking to the workspace", while giving you access to QProcess::finished would only work for the specific case of the workspace itself starting other apps.

IIRC startup notification is still a big question mark in Wayland, but other Plasma devs know better