D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[David Faure]

dfaure added a comment.


  Makes sense.
  
  But in many places, having to provide both is problematic. E.g. when the 
KServiceAction is the value<>() of a QAction.
  I'll look into storing the KService::Ptr inside the KServiceAction. If this 
works out, we can remove the KService::Ptr argument from this class...

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[Kai Uwe Broulik]

broulik added a comment.


  > KDesktopFileActions uses the name and icon from the action rather than the 
namd and icon from the service. Dunno if that's better though?
  
  I think we want to communicate to the user that "[Firefox icon] Firefox" is 
starting, not "[No icon] New incognito tab", which is why we use the service 
icon and name.

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[David Faure]

dfaure added a comment.


  Interestingly, D28268  is also about 
executing a KServiceAction, but I cannot port it to this API, because the 
KService::Ptr is not known in that (public) method.
  The caller, KFileItemActions, knows it so it could pass it to a new overload 
with one more parameter, but this seems to point out that maybe a 
KServiceAction is enough? KDesktopFileActions uses the name and icon from the 
action rather than the namd and icon from the service. Dunno if that's better 
though?

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:4f5503644aa6: [ApplicationLauncherJob] Add way to launch 
KServiceAction (authored by broulik).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28285?vs=78470&id=78518

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

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[David Faure]

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

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[Kai Uwe Broulik]

broulik edited the summary of this revision.
broulik edited the test plan for this revision.

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[Kai Uwe Broulik]

broulik edited the test plan for this revision.
broulik added a dependency: D28278: Move responsiblity for creating KService to 
CommandLauncherJob.

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[Kai Uwe Broulik]

broulik updated this revision to Diff 78470.
broulik added a comment.


  - Assert

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28285?vs=78466&id=78470

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

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


  Excellent idea!
  
  I'm glad to see that the class design allows these kinds of things.

INLINE COMMENTS

> applicationlauncherjob.cpp:57
> +d->m_service.detach();
> +if (d->m_service) {
> +d->m_service->setExec(serviceAction.exec());

Maybe this should even be a Q_ASSERT? We can't do much with a null service here.

REPOSITORY
  R241 KIO

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

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

D28285: [ApplicationLauncherJob] Add way to launch KServiceAction

[Kai Uwe Broulik]

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

REVISION SUMMARY
  This offers convenience API for launching a specific `KServiceAction` on a 
`KService`.

TEST PLAN
  We have a bunch of places in Plasma where (task manager, kickoff, krunner) 
where we deal with service actions. Right now they all pretty much 
`KRun::runCommand(action.exec())`.
  This new API ensures it gets the same capabilities (including URL expansion) 
and proper startup info (app icon bouncy cursor) as regular service launches.

REPOSITORY
  R241 KIO

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

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

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