[Task Manager] Port backend to ApplicationLauncherJob
ClosedPublic

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

Details

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.Mar 25 2020, 4:07 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 25 2020, 4:07 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Mar 25 2020, 4:07 PM
dfaure added inline comments.Mar 25 2020, 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.Mar 25 2020, 9:59 PM

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

broulik updated this revision to Diff 78521.Mar 26 2020, 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.Mar 28 2020, 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.Mar 28 2020, 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...

broulik planned changes to this revision.Mar 29 2020, 10:33 AM

Will rebase on pending API change in D28268

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

Even 1200 is not problem to me.

Even 1200 is not problem to me.

We seem to have very different opinions on good API design.

broulik updated this revision to Diff 78842.Mar 30 2020, 7:41 AM
  • Adjust to API change in KServiceAction
  • Drop linkage of KIOWidgets in favor of KIOGui (though KIOFileWidgets probably implies it anyway)
This revision is now accepted and ready to land.Mar 30 2020, 7:41 AM
broulik requested review of this revision.Mar 30 2020, 7:58 AM
dfaure accepted this revision.Apr 3 2020, 11:07 PM
This revision is now accepted and ready to land.Apr 3 2020, 11:07 PM

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.

dfaure added a comment.Apr 4 2020, 9:41 AM

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 ;)

anthonyfieroni added a comment.EditedApr 4 2020, 10:17 AM

OK, let's 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)

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.

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.

This revision was automatically updated to reflect the committed changes.