D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:5f2be012c27d: [CommandLauncherJob] Add constructor taking 
an executable and argument list (authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28515?vs=79267&id=79270

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

AFFECTED FILES
  autotests/commandlauncherjobtest.cpp
  autotests/commandlauncherjobtest.h
  src/gui/commandlauncherjob.cpp
  src/gui/commandlauncherjob.h

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  Awesome.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

broulik requested review of this revision.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

broulik updated this revision to Diff 79267.
broulik added a comment.
This revision is now accepted and ready to land.


  - Add test for binary in path with space

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28515?vs=79189&id=79267

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

AFFECTED FILES
  autotests/commandlauncherjobtest.cpp
  autotests/commandlauncherjobtest.h
  src/gui/commandlauncherjob.cpp
  src/gui/commandlauncherjob.h

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

broulik planned changes to this revision.
broulik added a comment.


  Good idea

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[David Faure]

dfaure added a comment.


  You could create a subdir of a QTemporaryDir with a fixed name like "abc 
def", copy `cp` or `copy.exe` in there, and then run that with an absolute path?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

broulik added a comment.


  Not sure how I would test running a command with a space in it.. putting a 
shell script in running it through QFINDTESTDATA? Not sure how to make that 
sort of stuff work on Windows

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

broulik updated this revision to Diff 79189.
broulik edited the test plan for this revision.
broulik added a comment.


  - Quote executable in commandline
  - Add unittest

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28515?vs=79136&id=79189

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

AFFECTED FILES
  autotests/commandlauncherjobtest.cpp
  autotests/commandlauncherjobtest.h
  src/gui/commandlauncherjob.cpp
  src/gui/commandlauncherjob.h

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[David Faure]

dfaure added a comment.


  Good question. I haven't seen that done in most callers, but indeed, what if 
it's in a path with a space, like often happens on Windows?
  It sounds like the answer is yes, it should be quoted.
  The real answer is to write a CommandLauncherJob unittest for such a case, of 
course :-)

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

broulik added a comment.


  Ok, makes sense. Just realized: do I need to `quoteArgs` the `executable`?

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  KProcessRunner::KProcessRunner(QString cmd) calls 
KProcess::setShellCommand(cmd) which ... has two modes. Either 
KShell::splitArgs()[0] finds an executable that exists (checking that there is 
no "&&" or "||" or ";" in the command) in which case it uses the splitted args, 
and no /bin/sh is involved, *or* it passes the whole command as is to "sh -c", 
as a single argument. That first optimization aside, it is logical for 
setShellCommand to take a single string. So It's logical to me that 
KProcessRunner takes a single string, and that you join the stringlist in 
CommandLauncherJob.

REPOSITORY
  R241 KIO

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

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D28515: [CommandLauncherJob] Add constructor taking an executable and argument list

[Kai Uwe Broulik]

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

REVISION SUMMARY
  More convenient than having to construct a proper escaped commandline.

TEST PLAN
  - Ported the runners in D28512  to use 
it, had something with a space properly escaped
  
  Somehow this looks a bit wrong to me, i.e. shouldnt we have some args in 
kprocessrunner, too, or does it also just pass that command along verbatim?

REPOSITORY
  R241 KIO

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

AFFECTED FILES
  src/gui/commandlauncherjob.cpp
  src/gui/commandlauncherjob.h

To: broulik, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns