KService: deprecate/slim down KToolInvocation
Open, Needs TriagePublic

Description

kdeinit related parts can be killed, the way to invoke "system email client" needs to be kept

mart created this task.Nov 23 2019, 5:05 PM

Fleshing this task out a bit more.

KToolInvocation::startServiceByDesktopName
becomes ApplicationLaunchJob

KToolInvocation::kdeinitExec
becomes QProcess if it's something that's conceptually the same task and doesn't need startup indicators and processing .desktop files
or ApplicationLaunchJob/CommandLaunchJob if it's starting something new

the way to invoke "system email client" needs to be kept

Wouldn't a QDesktopService::openUrl("mailto:/...") suffice? It surely isn't as pretty though.

dfaure added a subscriber: dfaure.Apr 24 2020, 9:24 AM

Yeah, who needs proper convenient and type-safe C++ API when they can instead mangle arguments into a string and mess up the encoding? :-)

Wouldn't a QDesktopService::openUrl("mailto:/...") suffice?

I looked at KToolInvocation::invokeMailer

Internally it has 3 paths:

A windows path, which builds a URL, finds the URL handler for mailto and manually invokes it with low level windows commands
A linux-non-kmail path which builds a URL, finds the URL handler for mailto and manually invokes it with low level kdeinit commands
A linux-kmail specific path, which builds a command line specifically for kmail.

I feel like we should be able to have the best of both worlds and reduce KToolInvokcation to provide a public API with convenient type-safe API to build a URL which then internally calls QDesktopServices:openUrl()

@hindenburg This phabricator isn't a change in itself.. The relevant commit from your CI is this one: https://invent.kde.org/utilities/konsole/-/merge_requests/194

I don't see how it could have broken the unit tests in itself.

KF6 meeting notes:

The still useful parts of KToolInvocation should be redesigned as jobs on top of ApplicationLauncherJob, like MailClientLauncherJob, TerminalLauncherJob etc. Maybe in KIO for now, then it can all move down together for KF6.

I volunteer to write these jobs, but I'm having second thoughts about providing a replacement in KIOGui for API in KService. It *raises* the amount of needed dependencies.

Solution 1: we find a solution for KRecentDocuments (like forking it in KService or below, under a new name), we fork CommandLauncherJob in KService (no name clash, thanks to the KIO namespace), and we can then have these jobs in KService right away. When I say forking, the old class could become a wrapper for the underlying (new) class, but that's more work [for a job it means using the other as a subjob...].

Solution 2: we (I) just implement these jobs in KIO, and solve the dependency issue at KF6 time.

One concrete problem with solution 1 is that kdesu uses KToolInvocation::kdeinitExecWait and doesn't link to KIO. But well, we can just wait for that one, or maybe use QProcess.

Solution 1: we find a solution for KRecentDocuments (like forking it in KService or below, under a new name), we fork CommandLauncherJob in KService (no name clash, thanks to the KIO namespace), and we can then have these jobs in KService right away. When I say forking, the old class could become a wrapper for the underlying (new) class, but that's more work [for a job it means using the other as a subjob...].

We would also need desktopexecparser and potentially add KWindowSystem dependency for the startup id handling

Solution 2: we (I) just implement these jobs in KIO, and solve the dependency issue at KF6 time.

Seems like the easiest solution to me

Interesting about desktopexecparser. I always assumed it would just move along with CommandLauncherJob/ApplicationLauncherJob/kprocessrunner.

This made me realize that it needs KProtocolInfo::isHelperProtocol(url) / KProtocolInfo::isKnownProtocol(url) which very much belong to KIO,
but that's only in the static method KIO::DesktopExecParser::hasSchemeHandler() which is only used in OpenUrlJob (and KRun). I just added KF6 TODOs to solve that, quite easy: https://invent.kde.org/frameworks/kio/commit/65a374daa83947cbdf8c9d427d1462b4af8ae1af

So yes, it can move to KService too.

For KWindowSystem, I guess it would be ok to add that dependency to KService (tier 3).

But yes, that's a lot of things to fork right now, let's go for solution 2. Thanks for your thoughts on this.

alex moved this task from Backlog to Waiting on Other Tasks on the KF6 board.May 8 2021, 1:35 PM
vkrause moved this task from Waiting on KF6 Branching to Done on the KF6 board.Jan 29 2023, 2:22 PM