D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Kai Uwe Broulik]

broulik updated this revision to Diff 78521.
broulik retitled this revision from "WIP: [Task Manager] Port backend to 
ApplicationLauncherJob" to "[Task Manager] Port backend to 
ApplicationLauncherJob".
broulik edited the summary of this revision.
broulik edited the test plan for this revision.
broulik added a comment.


  - Use `KNotificationJobUiDelegate`

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28286?vs=78471&id=78521

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

AFFECTED FILES
  applets/taskmanager/CMakeLists.txt
  applets/taskmanager/plugin/backend.cpp
  applets/taskmanager/plugin/backend.h

To: broulik, #plasma, hein, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

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


  Looks good to me, but please wait until D28367 
 and D28268 
 land, since they change the API of 
ApplicationLauncherJob for KServiceAction.
  
  Many thanks for helping with this!

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> backend.cpp:236-239
> +auto *job = new KIO::ApplicationLauncherJob(service);
> +auto *delegate = new KNotificationJobUiDelegate;
> +delegate->setAutoErrorHandlingEnabled(true);
> +job->setUiDelegate(delegate);

I see that redundant code in many places can you wrap it in function

  KJob* createApplicationNotificationLauncherJob

or something similar.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  Alternatively I'm wondering if we should add constructors to the delegates 
that take some AutoErrorHandlingEnabled enum value, then it can become 
something like
  
auto *job = new KIO::ApplicationLauncherJob(service);
job->setUiDelegate(new 
KNotificationJobUiDelegate(KJob::AutoErrorHandlingEnabled));
  
  (and the same with KDialogJobUiDelegate in widget applications)
  
  Benefit: available everywhere, unlike a local wrapper function.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D28286#636668 , @dfaure wrote:
  
  > Benefit: available everywhere, unlike a local wrapper function.
  
  
  I think wrapper function in KIO :)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  I don't think we want to have wrapper functions in KIO for all combinations 
of jobs and delegates. There are many of each.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added a comment.


  Why not? Furthermore they can be easy fixable.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  Why not -> because it just doesn't scale. 30 jobs * 4 delegates = 120 wrapper 
methods...

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Kai Uwe Broulik]

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


  Will rebase on pending API change in D28268 


REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D28286#636881 , @dfaure wrote:
  
  > Why not -> because it just doesn't scale. 30 jobs * 4 delegates = 120 
wrapper methods...
  
  
  Even 1200 is not problem to me.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  In D28286#637346 , @anthonyfieroni 
wrote:
  
  > Even 1200 is not problem to me.
  
  
  We seem to have very different opinions on good API design.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Kai Uwe Broulik]

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


  - Adjust to API change in `KServiceAction`
  - Drop linkage of `KIOWidgets` in favor of `KIOGui` (though `KIOFileWidgets` 
probably implies it anyway)

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28286?vs=78521&id=78842

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

AFFECTED FILES
  applets/taskmanager/CMakeLists.txt
  applets/taskmanager/plugin/backend.cpp
  applets/taskmanager/plugin/backend.h

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added a comment.


  And what about the idea to pass delegate to job constructor? At least it's 
better than current one. I'm pretty pedantic about duplicate code in plus it 
makes porting harder.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  That would save only one line (the call to setUiDelegate).
  
  I prefer my earlier suggestion: job->setUiDelegate(new 
KNotificationJobUiDelegate(KJobUiDelegate::AutoErrorHandlingEnabled));
  That one alone would save 2 lines ;)

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added a comment.


  OK, let's not keep things as they are. Because the best i can make without 
dedicated function is `new KIO::ApplicationLauncherJob(service, 
KIO::ApplicationLauncher::WITH_AUTO_ERROR_HANDLED_DELEGATE)`

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[David Faure]

dfaure added a comment.


  That wouldn't work either, you need to be able to choose between a 
Notification delegate, a Dialog delegate (which lives in a different library 
due to the QtWidgets dependency), and some more.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D28286#641165 , @dfaure wrote:
  
  > That wouldn't work either, you need to be able to choose between a 
Notification delegate, a Dialog delegate (which lives in a different library 
due to the QtWidgets dependency), and some more.
  
  
  Got it, that's pretty good reason.

REPOSITORY
  R119 Plasma Desktop

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

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28286: [Task Manager] Port backend to ApplicationLauncherJob

[Kai Uwe Broulik]

This revision was automatically updated to reflect the committed changes.
Closed by commit R119:fa8e49b3be08: [Task Manager] Port backend to 
ApplicationLauncherJob (authored by broulik).

REPOSITORY
  R119 Plasma Desktop

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28286?vs=78842&id=79456

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

AFFECTED FILES
  applets/taskmanager/CMakeLists.txt
  applets/taskmanager/plugin/backend.cpp
  applets/taskmanager/plugin/backend.h

To: broulik, #plasma, hein, dfaure
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart