- Ported runCommand to CommandLauncherJob
- Added a unittest
I'll try to port all users away from runCommand before deprecating
it, once apps can depend on KF 5.69.
Details
Details
Test passes. I'll now test the code where dolphin start kompare
usin runCommand.
Diff Detail
Diff Detail
- Repository
- R241 KIO
- Branch
- 2020_03_commandlauncherjob
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 24182 Build 24200: arc lint + arc unit
Comment Actions
Tools/Compare Files still works in dolphin (it uses KRun::runCommand).
http://www.davidfaure.fr/2020/dolphin.diff ports it to CommandLauncherJob, it still works afterwards.
Comment Actions
Overview:
KRun::runService/runApplication => ProcessLauncherJob
KRun::runCommand => CommandLauncherJob
new KRun => OpenUrlJob, not written yet
Maybe the first one should be renamed to ApplicationLauncherJob, now that I found out it wouldn't be a good place for running commands (the setters are too different).
Comment Actions
(technically, connecting to &KJob::result is missing in the dolphin port, but unlike ProcessLauncherJob, CommandLauncherJob is very unlikely to fail. QProcess will only fail to start if we're out of resources for forking or if sh can't be executed... we don't detect bad shell commands, that happens after QProcess::start(). I wonder if it's worth connecting to the result signal of a CommandLauncherJob....)
Comment Actions
Maybe the first one should be renamed to ApplicationLauncherJob, now that I found out it wouldn't be a good place for running commands (the setters are too different).
Probably.
| src/gui/commandlauncherjob.cpp | ||
|---|---|---|
| 64 | Can we have a setDesktopName() as an additional shortcut for icon + executable It will also help the heuristic KProcessRunner is doing: KService::Ptr service = KService::serviceByDesktopName(bin); which won't work now kompare is probably with the ID org.kde.kompare | |
| src/gui/commandlauncherjob.h | ||
| 40 | PID without (s) | |
| 99 | this is copy pasted | |
Comment Actions
Add setDesktopName(), excellent idea by davidedmundson.
Fix API docs, fix copyright years, fix "do terminate" comment.
Comment Actions
Do we have a kf6 workboard entry for this effort?
I think we should also have a replacement job for new KRun(url) and I'd love if we could split out the "default app for this mimetype" logic we (also?) have in KFileItemActions out into a dedicated public class.
This comment was removed by davidedmundson.
Comment Actions
One thing I noticed, when I do new CommandLaunchJob("somenonexistantbinary") no error is raised and I just get console output
`/bin/sh: 1: somenonexistantbinary: not found
Though this was also the case of runCommand, so maybe not an issue.
| autotests/commandlauncherjobtest.cpp | ||
|---|---|---|
| 99 | See this unittest ;-) As the comment says, QProcess works since it manages to start /bin/sh. We don't want to wait until the application/process exits so we don't catch this. | |
Comment Actions
Yes, these launcher jobs are the building blocks for finally being able to turn KRun into a job (KIO::OpenUrlJob). Work in progress....
and I'd love if we could split out the "default app for this mimetype" logic we (also?) have in KFileItemActions out into a dedicated public class.
The default app for *this* mimetype (singular) is a one-line call to KApplicationTrader::preferredService(mimeType). (Before last month or so, that would have been KMimeTypeTrader).
What KFileItemActions::associatedApplications does on top of that is handle the case of multiple files with multiple mimetypes. Is this the use case you have in mind here?
I can see how it would make total sense to move this to KApplicationTrader as well, if it's needed outside KFileItemActions. I can work on that, if you confirm that your question here wasn't really about a singular mimetype, but a list of mimetypes.