D28278: Move responsiblity for creating KService to CommandLauncherJob

2020-03-25 Thread Kai Uwe Broulik 
broulik added a dependent revision: D28285: [ApplicationLauncherJob] Add way to 
launch KServiceAction.

REPOSITORY
  R241 KIO

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

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

D28278: Move responsiblity for creating KService to CommandLauncherJob

2020-03-25 Thread David Edmundson 
davidedmundson added inline comments.

INLINE COMMENTS

> broulik wrote in kprocessrunner.cpp:119
> I //think// we can still end up with a `null` `service` through 
> `ApplicationLauncherJob`?

I don't think so, or at least, it would be crashing already.

The constructor with the KService::Ptr uses service-> unconditionally

REPOSITORY
  R241 KIO

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

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

D28278: Move responsiblity for creating KService to CommandLauncherJob

2020-03-25 Thread David Edmundson 
davidedmundson updated this revision to Diff 78463.
davidedmundson added a comment.


  rebase, fix icon

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28278?vs=78451&id=78463

BRANCH
  master

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

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

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

D28278: Move responsiblity for creating KService to CommandLauncherJob

2020-03-25 Thread David Edmundson 
davidedmundson marked an inline comment as done.

REPOSITORY
  R241 KIO

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

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

D28278: Move responsiblity for creating KService to CommandLauncherJob

2020-03-25 Thread Kai Uwe Broulik 
broulik added a comment.


  Lovely simplification

INLINE COMMENTS

> kprocessrunner.cpp:119
>  {
> -if (service && !service->entryPath().isEmpty()
> +if (!service->entryPath().isEmpty()
>  && !KDesktopFile::isAuthorizedDesktopFile(service->entryPath())) 
> {

I //think// we can still end up with a `null` `service` through 
`ApplicationLauncherJob`?

> kprocessrunner.cpp:145
>  data.setDescription(i18n("Launching %1", data.name()));
> -if (!iconName.isEmpty()) {
> -data.setIcon(iconName);
> -} else if (service && !service->icon().isEmpty()) {
> +if (service->icon().isEmpty()) {
>  data.setIcon(service->icon());

`!isEmpty`

REPOSITORY
  R241 KIO

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

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

D28278: Move responsiblity for creating KService to CommandLauncherJob

2020-03-25 Thread David Edmundson 
davidedmundson created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
davidedmundson requested review of this revision.

REVISION SUMMARY
  KProcessRunner had two code paths for if data was part of the KService
  or if it was separate pars. This leads to a lot of args and extra if
  statements.
  
  We can move this abstraction into CommandLineJob, creating a KService
  instance as a data structure for our explicitly set iconName and user
  visible name and command line.

TEST PLAN
  Relevant unit test

REPOSITORY
  R241 KIO

BRANCH
  master

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

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

To: davidedmundson
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns