Details

Reviewers

broulik
mart
vkrause
nicolasfella
aacid
davidedmundson
apol

Maniphest Tasks

T12175: KService: write ApplicationTrader

Commits

R309:18c5a9e3cf90: New query mechanism for applications: KApplicationTrader

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

Branch

kapplicationtrader

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 21303
Build 21321: 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

Harbormaster completed remote builds in B19412: Diff 70785.Dec 2 2019, 9:37 PM

dhaumann added a subscriber: dhaumann.Dec 2 2019, 9:55 PM

dhaumann added inline comments.

src/services/kapplicationtrader.h
34	Typo: you want to _get_...

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

Harbormaster completed remote builds in B19413: Diff 70787.Dec 2 2019, 9:58 PM

Works fine for my rather simple KDE Connect use case

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.

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

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?

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

dfaure mentioned this in T12340: Install all parts into plugins/kf5/parts, and add JSON metadata to them.Dec 8 2019, 10:41 PM

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

Meanwhile this review request is about application lookup :)

dfaure mentioned this in T12173: KService: provide solution to migrate away from KServiceTypeTrader/KMimeTypeTrader for loading plugins and parts.Jan 2 2020, 8:34 AM

dfaure mentioned this in D26690: Make "Default Applications" in mimeapps.list the preferred applications.Jan 15 2020, 11:23 PM

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

Harbormaster completed remote builds in B21303: Diff 73800.Jan 17 2020, 10:04 PM

aacid added inline comments.Jan 17 2020, 11:21 PM

src/services/kapplicationtrader.h
57	all this "constraint" definition needs updating

dfaure mentioned this in D26740: Add KService::isSubseq as a public API for filtering queries..Jan 18 2020, 8:50 AM

Integrate isSubsequence, fix docu, reword commit log

Harbormaster completed remote builds in B21339: Diff 73858.Jan 19 2020, 9:25 AM

aacid added inline comments.Jan 19 2020, 11:52 AM

src/services/kapplicationtrader.h
57 ↗	(On Diff #70787)	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.Jan 19 2020, 5:51 PM

dfaure added inline comments.

src/services/kapplicationtrader.h
57 ↗	(On Diff #70787)	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.

Simplify docu for query()

Harbormaster completed remote builds in B21360: Diff 73890.Jan 19 2020, 5:51 PM

dhaumann added inline comments.Jan 21 2020, 7:37 AM

src/services/kapplicationtrader.cpp
88 ↗	(On Diff #70787)	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 ↗	(On Diff #70787)	Please document this typedef so that FilterFunc will be clickable in the generated API documentation.
81 ↗	(On Diff #70787)	method without training e.
91 ↗	(On Diff #70787)	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.Jan 22 2020, 8:31 AM

dfaure added inline comments.

src/services/kapplicationtrader.cpp
88 ↗	(On Diff #70787)	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 ↗	(On Diff #70787)	LOL, I'm trying but sometimes the French in me takes over :)
91 ↗	(On Diff #70787)	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.

Improve docu

Harbormaster completed remote builds in B21466: Diff 74073.Jan 22 2020, 8:40 AM

broulik added inline comments.Jan 22 2020, 8:55 AM

src/services/kapplicationtrader.cpp
88 ↗	(On Diff #70787)	cppreference says it does: Relative order of the elements that remain is preserved and the physical size of the container is unchanged

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

Harbormaster completed remote builds in B21641: Diff 74334.Jan 24 2020, 10:06 PM

improve unittest, let the name match more queries

Harbormaster completed remote builds in B21642: Diff 74335.Jan 24 2020, 10:11 PM

ping?

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

+1 Makes sense.

This revision is now accepted and ready to land.Feb 4 2020, 7:01 PM

dfaure closed this revision.Feb 4 2020, 10:49 PM

		Path
M		autotests/CMakeLists.txt (3 lines)
A	M	autotests/kapplicationtradertest.cpp (308 lines)
M		autotests/kservicetest.h (1 line)
M		autotests/kservicetest.cpp (29 lines)
M		src/CMakeLists.txt (2 lines)
A	M	src/services/kapplicationtrader.h (104 lines)
A	M	src/services/kapplicationtrader.cpp (135 lines)
M		src/services/kmimetypetrader.cpp (4 lines)
M		src/services/kservicefactory.cpp (10 lines)
M		src/services/kservicefactory_p.h (3 lines)
M		src/services/kservicetypetrader.h (2 lines)
M		src/sycoca/ksycocaentry.h (1 line)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	70785	8f9faed		Dec 2 2019, 9:37 PM	★	★
Diff 2	70787	8f9faed	add missing "get" (this bug was in kmimetypetrader.h already, blatant proof of…	Dec 2 2019, 9:58 PM	★	★
Diff 3	73800	64155c0	Remove self()	Jan 17 2020, 10:04 PM	★	★
Diff 4	73858	9849bf7	Integrate isSubsequence, fix docu, reword commit log	Jan 19 2020, 9:25 AM	★	★
Diff 5	73890	9849bf7	Simplify docu for query()	Jan 19 2020, 5:51 PM	★	★
Diff 6	74073	9849bf7	Improve docu	Jan 22 2020, 8:31 AM	★	★
Diff 7	74334	9849bf7	Port to erase(remove_if), add unittest for OnlyShowIn, which showed…	Jan 24 2020, 10:06 PM	★	★
Diff 8	74335	9849bf7	improve unittest, let the name match more queries	Jan 24 2020, 10:11 PM	★	★

New query mechanism for applications: KApplicationTrader
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 73800

autotests/CMakeLists.txt

autotests/kapplicationtradertest.cpp

autotests/kservicetest.h

autotests/kservicetest.cpp

src/CMakeLists.txt

src/services/kapplicationtrader.h

src/services/kapplicationtrader.cpp

src/services/kmimetypetrader.cpp

src/services/kservicefactory.cpp

src/services/kservicefactory_p.h

src/services/kservicetypetrader.h

src/sycoca/ksycocaentry.h

New query mechanism for applications: KApplicationTraderClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 73800

autotests/CMakeLists.txt

autotests/kapplicationtradertest.cpp

autotests/kservicetest.h

autotests/kservicetest.cpp

src/CMakeLists.txt

src/services/kapplicationtrader.h

src/services/kapplicationtrader.cpp

src/services/kmimetypetrader.cpp

src/services/kservicefactory.cpp

src/services/kservicefactory_p.h

src/services/kservicetypetrader.h

src/sycoca/ksycocaentry.h

New query mechanism for applications: KApplicationTrader
ClosedPublic
Actions

Revision Contents
Changeset List