D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure added a task: T11549: KIO: remove unneeded QWidget dependencies to set 
parent windows or display errors.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: jbbgameich, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28020: New class ProcessLauncherJob in KIOGui

[Nicolas Fella]

nicolasfella added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: jbbgameich, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure added a comment.


  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: jbbgameich, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28020: New class ProcessLauncherJob in KIOGui

[Jonah Brüchert]

jbbgameich added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: jbbgameich, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, 
bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Edmundson]

davidedmundson added a comment.


  In D28020#630185 , @dfaure wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure added a comment.


  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...)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure closed this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> davidedmundson wrote in 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.

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...

> davidedmundson wrote in krun.cpp:488
> or we can just ignore it if we're phasing out klauncher anyway?

Right. Separate issue though. I like incremental steps.

REPOSITORY
  R241 KIO

BRANCH
  2020_ProcessLauncherJob

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Edmundson]

davidedmundson accepted this revision.
davidedmundson added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> kprocessrunner.cpp:194
> +{
> +return m_process->waitForStarted();
> +}

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.

> krun.cpp:488
>  // This code is also used in klauncher.
> -// TODO: move this to KProcessRunner
> +// TODO: port klauncher to KIOGuiPrivate::checkStartupNotify once this lands
> +// TODO: then deprecate this method, and remove in KF6

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

REPOSITORY
  R241 KIO

BRANCH
  2020_ProcessLauncherJob

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure updated this revision to Diff 77682.
dfaure added a comment.


  - 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)

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28020?vs=77547&id=77682

BRANCH
  2020_ProcessLauncherJob

REVISION DETAIL
  https://phabricator.kde.org/D28020

AFFECTED FILES
  KF5KIOConfig.cmake.in
  autotests/CMakeLists.txt
  autotests/kiotesthelper.h
  autotests/processlauncherjobtest.cpp
  autotests/processlauncherjobtest.h
  src/gui/CMakeLists.txt
  src/gui/config-kiogui.h.cmake
  src/gui/kprocessrunner.cpp
  src/gui/kprocessrunner_p.h
  src/gui/processlauncherjob.cpp
  src/gui/processlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/krun.h
  src/widgets/krun_p.h

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure planned changes to this revision.
dfaure added inline comments.

INLINE COMMENTS

> davidedmundson wrote in kprocessrunner.cpp:39
> 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

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

> davidedmundson wrote in processlauncherjob.h:111
> 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.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Edmundson]

davidedmundson added a comment.


  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.

INLINE COMMENTS

> kprocessrunner.cpp:39
> +
> +KProcessRunner::KProcessRunner(const KService::Ptr &service, const 
> QList &urls, WId windowId,
> +   KIO::ProcessLauncherJob::RunFlags flags, 
> const QString &suggestedFileName, const QByteArray &asn)

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

> processlauncherjob.h:111
> + */
> +qint64 pid() const;
> +

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D28020

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28020: New class ProcessLauncherJob in KIOGui

[David Faure]

dfaure created this revision.
dfaure added reviewers: apol, davidedmundson, nicolasfella, vkrause, broulik.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION 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)

REPOSITORY
  R241 KIO

BRANCH
  2020_ProcessLauncherJob

REVISION DETAIL
  https://phabricator.kde.org/D28020

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kiotesthelper.h
  autotests/processlauncherjobtest.cpp
  autotests/processlauncherjobtest.h
  src/gui/CMakeLists.txt
  src/gui/config-kiogui.h.cmake
  src/gui/kprocessrunner.cpp
  src/gui/kprocessrunner_p.h
  src/gui/processlauncherjob.cpp
  src/gui/processlauncherjob.h
  src/widgets/CMakeLists.txt
  src/widgets/krun.cpp
  src/widgets/krun.h
  src/widgets/krun_p.h

To: dfaure, apol, davidedmundson, nicolasfella, vkrause, broulik
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns