New query mechanism for applications: KApplicationTrader
ClosedPublic

Authored by dfaure on Dec 2 2019, 9:37 PM.

Details

Summary

KMimeTypeTrader and KServiceTypeTrader could lookup both applications and
plugins, but plugins should nowadays be loaded via their JSON file
(KPluginLoader), no desktop-file trader needed anymore. Except maybe for non-C++ plugins?

KApplicationTrader gets rid of plugins and servicetypes from the API, in
order to keep only the API to look up application desktop files.

The trader query language is no longer used, as discussed on
kde-frameworks-devel; instead we use a std::function for filtering, like
KPluginLoader.

This commit also adds KApplicationTrader::isSubsequence, necessary to
replace the subseq operator of the trader query language.

Test Plan

New unittest (initially based on kservicetest, simplified, ported
to lambdas, and with some new types of checks) passes

Diff Detail

Repository
R309 KService
Branch
kapplicationtrader
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21642
Build 21660: arc lint + arc unit
dfaure created this revision.Dec 2 2019, 9:37 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 2 2019, 9:37 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
dfaure requested review of this revision.Dec 2 2019, 9:37 PM
dhaumann added inline comments.
src/services/kapplicationtrader.h
34

Typo: you want to _get_...

dfaure updated this revision to Diff 70787.Dec 2 2019, 9:58 PM

add missing "get" (this bug was in kmimetypetrader.h already, blatant proof of copy/pasting... ;-) )

Works fine for my rather simple KDE Connect use case

+1

Feedback on the API question would be welcome.

Also, see the discovery in T12256. I'm thinking of renaming this class to KServiceTrader and add a queryByServiceType to it.

aacid added a subscriber: aacid.Dec 7 2019, 10:06 AM

How would i load a "KPart that can open PDF files" then?

Doesn't seem that KPluginLoader has api for that and a Part is not an Application either

dfaure added a comment.Dec 7 2019, 9:10 PM

Assuming we install all parts under plugins/parts instead of just plugins/ like we currently do, the application should be able to do

KPluginTrader::createInstanceFromQuery<KParts::ReadOnlyPart>("parts", "KParts/ReadOnlyPart", "'application/pdf' in MimeTypes", parent, parentWidget)

This makes me realize however that mimetype inheritance would probably not be supported when doing it this way.
We probably want to add a queryByMimeType to KPluginTrader as well, and a createInstanceFromQuery that takes a mimetype name, like KMimeTypeTrader has (maybe with another name though, too many overloads otherwise).

Would this work for you?

aacid added a comment.Dec 8 2019, 10:22 PM

Would this work for you?

I guess, i'm not really sure if i understand all you wrote, but i just want to make sure we can do a "generic part loader that opens files" if we want to

Part-loading discussion continues in https://phabricator.kde.org/T12173.

Meanwhile this review request is about application lookup :)

dfaure updated this revision to Diff 73800.Jan 17 2020, 10:04 PM

Remove self()
Replace trader language with std::function
Port tests to std::function, including KService::isSubSeq

aacid added inline comments.Jan 17 2020, 11:21 PM
src/services/kapplicationtrader.h
58

all this "constraint" definition needs updating

dfaure updated this revision to Diff 73858.Jan 19 2020, 9:25 AM
dfaure retitled this revision from New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader to New query mechanism for applications: KApplicationTrader.
dfaure edited the summary of this revision. (Show Details)
dfaure edited the test plan for this revision. (Show Details)
dfaure added reviewers: aacid, davidedmundson.
dfaure removed subscribers: aacid, dhaumann.

Integrate isSubsequence, fix docu, reword commit log

aacid added inline comments.Jan 19 2020, 11:52 AM
src/services/kapplicationtrader.h
57

why is it slow? Looking at the code we have to go trhough all apps anyway since what we do is erase if returning false, so wouldn't returning true actually be faster?

dfaure marked 3 inline comments as done.Sun, Jan 19, 5:51 PM
dfaure added inline comments.
src/services/kapplicationtrader.h
57

Excellent point.

This was a mental copy/paste from allServices() which basically tells people, instead of iterating over the full list of services, better try to use an existing "database index" like "query by servicetype" or "query by mimetype" or "query by name". Here's there's no choice (well, this is a query by servicetype "Application", at least it doesn't look at plugins).

I'll remove this.

dfaure updated this revision to Diff 73890.Sun, Jan 19, 5:51 PM
dfaure marked an inline comment as done.

Simplify docu for query()

dhaumann added inline comments.Tue, Jan 21, 7:37 AM
src/services/kapplicationtrader.cpp
88

I would prefer the std::erase(std::remove_if(...), ...end()); idiom here.

Assuming the list is a vector this will be much faster, or do you have to preserve the order? I fear I know the answer :-)

src/services/kapplicationtrader.h
49

Please document this typedef so that FilterFunc will be clickable in the generated API documentation.

81

method without training e.

91

Maybe mention when this function is useful? For me it looks like a private helper function. Why is it public?

dfaure marked 2 inline comments as done.Wed, Jan 22, 8:31 AM
dfaure added inline comments.
src/services/kapplicationtrader.cpp
88

Order is very important here, it's the order of preference.

But doesn't erase(remove_if()) preserve order? I thought it did.

src/services/kapplicationtrader.h
81

LOL, I'm trying but sometimes the French in me takes over :)

91

It's not a private helper, it's one of the things you might want to call from your lambda filter function. See the unittest (which is a bit of a "porting guide" from the old traders).

Added a line of docu about that.

dfaure updated this revision to Diff 74073.Wed, Jan 22, 8:31 AM

Improve docu

broulik added inline comments.Wed, Jan 22, 8:55 AM
src/services/kapplicationtrader.cpp
88

cppreference says it does:

Relative order of the elements that remain is preserved and the physical size of the container is unchanged

dfaure updated this revision to Diff 74334.Fri, Jan 24, 10:06 PM

Port to erase(remove_if), add unittest for OnlyShowIn, which showed inconsistencies => now removed from results of both methods.

dfaure updated this revision to Diff 74335.Fri, Jan 24, 10:11 PM

improve unittest, let the name match more queries

aacid added a comment.Tue, Feb 4, 6:57 PM

For me it can go in, but i'm not really really netiher a Frameworks developer nor potentially a user of this class, so i'd feel more confortable if someone else also +1'ed

On the other hand you're the mega-manintainer of everything, so i guess it can go in :)

apol accepted this revision.Tue, Feb 4, 7:01 PM
apol added a subscriber: apol.

+1 Makes sense.

This revision is now accepted and ready to land.Tue, Feb 4, 7:01 PM
dfaure closed this revision.Tue, Feb 4, 10:49 PM