Make "Default Applications" in mimeapps.list the preferred applications
ClosedPublic

Authored by meven on Jan 15 2020, 4:20 PM.

Details

Summary

Use mimeapps.list "Default Applications" to prepend the service to the offers for this mimetype.
So that KMimeTypeTrader::preferredService will respect the setting.

Use KMimeTypeTrader::preferredService to replace usage of KEmailSettings / emaildefaults.

Specs:
https://specifications.freedesktop.org/shared-mime-info-spec/shared-mime-info-spec-latest.html
https://specifications.freedesktop.org/mime-apps-spec/mime-apps-spec-latest.html

CCBUG: 403499
FIXED-IN: 5.67

Diff Detail

Repository
R309 KService
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
meven created this revision.Jan 15 2020, 4:20 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 15 2020, 4:20 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
meven requested review of this revision.Jan 15 2020, 4:20 PM
meven retitled this revision from Add KMimeTypeTrader::defaultSchemaHandler to return default service associated with scheme to [WIP] Add KMimeTypeTrader::defaultSchemaHandler to return default service associated with scheme.Jan 15 2020, 4:25 PM
meven edited the summary of this revision. (Show Details)
meven added reviewers: dfaure, dvratil, ervin.
meven added a comment.Jan 15 2020, 4:27 PM

I mean here to build consensus then implement about the need for this, then on how to implement it.

I think KService should handle building this when parsing mimeapps.list files.
And then expose this as KService::findDefaultSchemeHandler(scheme)

s/Schema/Scheme/ everywhere (method name, commit log)

I want to deprecate KMimeTypeTrader, this should rather go into the upcoming KApplicationTrader D25698, if new API is needed.

I'm pretty sure you can use KMimeTypeTrader::query(mimeHandler) instead of parsing mimeapps.list. And then I wonder if you really need new API :)

meven added a comment.Jan 16 2020, 8:50 AM

s/Schema/Scheme/ everywhere (method name, commit log)

I want to deprecate KMimeTypeTrader, this should rather go into the upcoming KApplicationTrader D25698, if new API is needed.

I'm pretty sure you can use KMimeTypeTrader::query(mimeHandler) instead of parsing mimeapps.list. And then I wonder if you really need new API :)

Let me be clearer about my intend.

Basically it is about fixing the TODO in kmimeassociation.cpp added in 3116911fa0d02dc4c5f05923cc6d7bf490a580e5

// TODO "Default Applications" is a separate query and a separate algorithm, says the spec.
// For now this is better than nothing though.
parseAddedAssociations(KConfigGroup(&profile, "Default Applications"), file, basePreference);

For instance say, I have in ~/.config/mimeapps.list :

[Added Associations]
x-scheme-handler/mailto=org.kde.kate.org;thunderbird.desktop

[Default Applications]
x-scheme-handler/mailto=thunderbird.desktop;

Thunderbird will think it is the default email client (according to the spec) but KMimeTypeTrader::preferredService("x-scheme-handler/mailto") KApplicationTrader::preferredService("x-scheme-handler/mailto") will return "org.kde.kate.org".
And in general we have no alternative way to find default association to mimetype according to the xdg specs. It may work as long as we don't have any "Added Associations", or when they match "Default Applications" but non-kde apps are not expected to deviate from the spec for our limitations.

Perhaps all we need is to improve parsing of "Default Applications" so that it is returned by KMimeTypeTrader::preferredService, that is prepend default apps to the list of apps associated with mimetypes.
I will update this PR soonish to implement this idea.

meven retitled this revision from [WIP] Add KMimeTypeTrader::defaultSchemaHandler to return default service associated with scheme to Make "Default Applications" in mimeapps.list the preferred applications.Jan 16 2020, 9:11 AM
meven edited the summary of this revision. (Show Details)
meven updated this revision to Diff 73679.Jan 16 2020, 9:13 AM

Update code to have a better implementation reusing existant API

Thanks for looking into this, I'm glad that finally someone does dig into this code.

I'm a bit surprised by the solution though. The spec simply says

  • add any "Default Applications" and then "Added Associations" in the first mimeapps.list

This doesn't need any reverse iteration or prepending, it's just about reading Default Applications before Added Associations rather than the other way around, isn't it?

I think my comment 5 years ago also meant that the xdg spec allows for the default app (left-click in file manager) to be different from the preferred app (RMB / Open With).
But looking at the spec now (Application ordering) it really treats "Default Applications" as higher-priority Added Associations, which proves that having separated the two is just complete nonsense, they serve the same purpose. Bleh. At least it's easier to implement this way :-)

src/sycoca/kmimeassociations.cpp
112

typo: precedence

113

Iterating with rbegin/rend (instead of the range-for on line 119) would be faster.

Or .... factorize the rest of the loop with the other method, since that's all duplicated otherwise? Then that's a good reason to keep the same range-for.

125

Replace the last argument with true, clearly this service is allowed as default :)
(I'll remove allowAsDefault in KF6 anyway)

176

I must be tired, but I don't see any difference between the code of this method and the one in addServiceOffer?

(and depending on what the difference should be, I think this should be a single method with an enum argument, to reduce duplication)

meven updated this revision to Diff 73778.Jan 17 2020, 3:49 PM
meven marked 4 inline comments as done.

Review : Add an enum AddServiceFlag

meven updated this revision to Diff 73781.Jan 17 2020, 3:57 PM
This comment was removed by meven.
meven added a comment.Jan 17 2020, 5:18 PM

Partially I believe, D26557 should have helped too.
Currently a file change of mimeapps.list does not trigger kbuildsyscoca AFAICT so manual file edition will cause issues.
And D26731 will help to use the mimeapps.list defined browser before the kdeglobals one.

meven edited the summary of this revision. (Show Details)Jan 17 2020, 5:18 PM

I'm quite confused by all this. Wouldn't it be enough to do http://www.davidfaure.fr/2020/kmimeassociations.cpp.diff ?
The modified unittest passes :-)

meven planned changes to this revision.Jan 18 2020, 6:44 PM

I'm quite confused by all this. Wouldn't it be enough to do http://www.davidfaure.fr/2020/kmimeassociations.cpp.diff ?
The modified unittest passes :-)

No because, Default Applications containst lists, means a same mime type in this group may have several

I'm quite confused by all this. Wouldn't it be enough to do http://www.davidfaure.fr/2020/kmimeassociations.cpp.diff ?
The modified unittest passes :-)

Well I think it does the trick.
Simply by adding 25 to the basePreferrence is way more simple.

I will integrate this to this patch as well as the unit test.
Thanks

meven updated this revision to Diff 73913.Jan 20 2020, 8:24 AM

Simplify implementation of Default Applications parsing, update tests

dfaure accepted this revision.Jan 20 2020, 9:03 AM

Good solution :-) LOL.

This revision is now accepted and ready to land.Jan 20 2020, 9:03 AM
meven edited the summary of this revision. (Show Details)Jan 20 2020, 9:25 AM
ervin added inline comments.Jan 20 2020, 1:44 PM
src/sycoca/kmimeassociations.cpp
104

Shouldn't the comment also include why the value 25 (and not 1, or 42 or NaN ;-)) for mere mortals like me?

dfaure added inline comments.Jan 20 2020, 2:03 PM
src/sycoca/kmimeassociations.cpp
104

No. What about my job security? ;-)

It's half of the +50 from line 86.

The range 1000-1025 is used by Added Associations, the range 1025-1050 is used by Default Applications, and so on for the next file.

meven added inline comments.Jan 20 2020, 4:01 PM
src/sycoca/kmimeassociations.cpp
104

The code kinda assumes we don't have more than 25 entries by configgroup.

dfaure added inline comments.Jan 20 2020, 5:15 PM
src/sycoca/kmimeassociations.cpp
104

More precisely, it assumes that a single mimetype entry doesn't have more than 25 desktop files associated with it. And even then, what would happen is that the ordering is wrong after the first 25. I'd say after 25 you start to not really care about the order....

meven added inline comments.Jan 20 2020, 6:25 PM
src/sycoca/kmimeassociations.cpp
104

The first 25 with the highest preferences cannot be affected but then they can have the same base reference as the next most category loaded.
Like the 26th "Default Applications" would have the same preference as the first "Added Applications"
But this is of low concern here anyway.
It just shows the limitations of the current design based on integer ordering.

@ervin Do you still feel it needs more comment ?

ervin added inline comments.Jan 21 2020, 7:20 AM
src/sycoca/kmimeassociations.cpp
104

The first one was fine, but what I meant was a comment in the code for when people bump into those magic numbers. ;-)

meven updated this revision to Diff 73996.Jan 21 2020, 8:52 AM

Add explanatory comment regarding 25 magic number

ervin accepted this revision.Jan 21 2020, 10:28 AM
meven updated this revision to Diff 74002.Jan 21 2020, 10:36 AM

Fix indentation issue

This revision was automatically updated to reflect the committed changes.