[Task Manager] Port backend to ApplicationLauncherJob
AcceptedPublic

Authored by broulik on Wed, Mar 25, 4:07 PM.

Details

Reviewers
hein
dfaure
Group Reviewers
Plasma
Summary

In the process, port jump list actions from custom KDesktopFile parsing to KServiceAction on KService, simplifying the code a lot.
Also, use the new KNotificationJobUiDelegate to provide a Plasma notification on error rather than a modal dialog box.

Test Plan
  • Launching incognito tabs in my browser still works
  • Opening Dolphin places still works
  • Opening recent docs still work and do open in the app the task manager entry is for rather than the default app for the mimetype

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Wed, Mar 25, 4:07 PM
Restricted Application added a project: Plasma. · View Herald TranscriptWed, Mar 25, 4:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Wed, Mar 25, 4:07 PM
dfaure added inline comments.Wed, Mar 25, 5:42 PM
applets/taskmanager/plugin/backend.cpp
179

Only if you like messageboxes.

Plasma code might prefer something else? A notification maybe? I think we both know a guy who likes them very much ;)

broulik planned changes to this revision.Wed, Mar 25, 9:59 PM

I'll look into creating a KNotificationUiDelegate or something along the lines

broulik updated this revision to Diff 78521.Thu, Mar 26, 8:29 AM
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. (Show Details)
broulik edited the test plan for this revision. (Show Details)
  • Use KNotificationJobUiDelegate
dfaure accepted this revision.Sat, Mar 28, 9:46 AM

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!

This revision is now accepted and ready to land.Sat, Mar 28, 9:46 AM
anthonyfieroni added inline comments.
applets/taskmanager/plugin/backend.cpp
236–239

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

KJob* createApplicationNotificationLauncherJob

or something similar.

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.

Benefit: available everywhere, unlike a local wrapper function.

I think wrapper function in KIO :)

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

Why not? Furthermore they can be easy fixable.

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