D28286: [Task Manager] Port backend to ApplicationLauncherJob

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=7

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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://

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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 REVISI

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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.o

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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 DETAI

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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, pl

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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-F

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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, zac

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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, pla

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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 KNotificationJobUiD

D28286: [Task Manager] Port backend to ApplicationLauncherJob

anthonyfieroni added inline comments. INLINE COMMENTS > backend.cpp:236-239 > +auto *job = new KIO::ApplicationLauncherJob(service); > +auto *delegate = new KNotificationJobUiDelegate; > +delegate->setAutoErrorHandlingEnabled(true); > +job->setUiDel

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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 ApplicationLauncherJo

D28286: [Task Manager] Port backend to ApplicationLauncherJob

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. br