Allow to handle apps with Terminal=True in their desktop file, handle their associated mimetype properly
ClosedPublic

Authored by meven on Jan 10 2020, 10:28 AM.

Details

Summary

The Terminal field in .desktop file was not taken into account as the desktop was only used to get an Exec line.
Also the supported protocols was missing x-scheme-handler declared protocols.

BUG:410506
FIXED-IN:5.67

Test Plan

Install mutt
Set mutt as default mail client in kcmshell5 componentchooser
kioclient5 exec mailto:test@test.com

Mutt is opened in default terminal application.

Diff Detail

Repository
R241 KIO
Branch
arcpatch-D26557
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21097
Build 21115: arc lint + arc unit
meven created this revision.Jan 10 2020, 10:28 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 10 2020, 10:28 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Jan 10 2020, 10:28 AM
meven edited the test plan for this revision. (Show Details)Jan 10 2020, 10:28 AM
meven planned changes to this revision.Jan 10 2020, 10:30 AM
meven updated this revision to Diff 73182.Jan 10 2020, 10:37 AM

Clean up

dfaure requested changes to this revision.Jan 12 2020, 11:24 AM
dfaure added a subscriber: dfaure.

Please add me as reviewer when touching my code in KIO. I almost didn't notice this one.

src/core/desktopexecparser.cpp
213

Move result of method call into local const variable. The usual range-for-detaches problem.

Can you call mimeTypes() instead of serviceTypes() here? I'm trying to slowly split the two notions, after it all got mixed up long ago.

214

QLatin1String is enough here

src/widgets/krun.cpp
100–102

Just remove the if then. This method becomes a one-liner.

101 ↗(On Diff #73490)

OK I wasn't happy about this docu disappearing but actually it fits better into hasSchemeHandler() which I later on moved to KIO::DesktopExecParser.

Done in https://commits.kde.org/kio/45f5d79600809f4c153c7b39ad90652cb921875c
You can now remove it from here indeed :)

952

You forgot to pass d->m_asn here (last argument of runApplication).

963

I think what you meant here was scheme-associated *mimetype* ?
or "default mimetype from the protocol file".

In a URL, scheme==protocol, but in KIO we're mostly using the word protocol to refer to those .protocol files. KProtocolInfo::exec() is also reading from the [scheme associated] protocol file so this comment is confusing.

(I wonder if this isn't dead code though - helper protocols don't set an empty exec line and a default mimetype, this would make no sense, only real ioslaves like kio_man set a default mimetype, text/html)

967

remove space before 'run'

This revision now requires changes to proceed.Jan 12 2020, 11:24 AM
meven added inline comments.Jan 12 2020, 11:32 AM
src/core/desktopexecparser.cpp
213

Can you call mimeTypes() instead of serviceTypes() here? I'm trying to slowly split the two notions, after it all got mixed up long ago.

No in fact, "x-scheme-handler/<protocol>" are not included in KService::mimeTypes() :

if (db.mimeTypeForName(sv).isValid()) { // keep only mimetypes, filter out servicetypes
meven updated this revision to Diff 73449.Jan 13 2020, 6:40 PM
meven marked 7 inline comments as done.

Address review

dfaure accepted this revision.Jan 13 2020, 11:12 PM
dfaure added inline comments.
src/core/desktopexecparser.cpp
213

OK, I see.

src/widgets/krun.cpp
962 ↗(On Diff #73449)

That is not true. isHelperProtocol returns true so there *is* a kio .protocol file.
Just no kioslave behind it.
A helper protocol is like vnc.protocol which sends vnc urls to the krdc program.
So it says exec=krdc '%u'.

A helper protocol with an empty exec line is weird, but I see a few like mms.protocol (https://bugs.kde.org/show_bug.cgi?id=84664 has some context).

A helper protocol with an empty exec line *and* a default mimetype... well there's one: rtsp.protocol which says defaultMimetype=audio/x-pn-realaudio. Well at least I see the point of that: it'll launch the app associated with that mimetype, for any rtsp://url.

OK, scheme-handler replaces most of these uses. I think we should just deprecate "helper protocols" (which either hardcode an exec line or abuse mimetypes) and move it all to the scheme-handler mechanism.

That's unrelated to your change so let's do that separately, of course.

For now, this comment makes no sense, you can just remove this line (962) and push.

This revision is now accepted and ready to land.Jan 13 2020, 11:12 PM
meven updated this revision to Diff 73490.Jan 14 2020, 9:37 AM

Remove uncorrect comment

meven marked an inline comment as done.Jan 14 2020, 9:37 AM
meven added a comment.Jan 14 2020, 9:41 AM

OK, scheme-handler replaces most of these uses. I think we should just deprecate "helper protocols" (which either hardcode an exec line or abuse mimetypes) and move it all to the scheme-handler mechanism.

I completely agree, we have a standardized way to handle scheme better implement the standard and improve compatibility.
It means KService will gain more responsibility over KProtocolInfo.
Could be a KF6 item.

This revision was automatically updated to reflect the committed changes.