D25698: New query mechanism for applications: KApplicationTrader
dfaure closed this revision. REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
apol accepted this revision. apol added a comment. This revision is now accepted and ready to land. +1 Makes sense. REPOSITORY R309 KService BRANCH kapplicationtrader REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson, apol Cc: apol, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
aacid added a comment. 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 :) REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure added a comment. ping? REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure updated this revision to Diff 74335. dfaure added a comment. improve unittest, let the name match more queries REPOSITORY R309 KService CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25698?vs=74334=74335 BRANCH kapplicationtrader REVISION DETAIL https://phabricator.kde.org/D25698 AFFECTED FILES autotests/CMakeLists.txt autotests/kapplicationtradertest.cpp autotests/kservicetest.cpp src/CMakeLists.txt src/services/kapplicationtrader.cpp src/services/kapplicationtrader.h src/services/kmimetypetrader.cpp src/services/kservicefactory.cpp src/services/kservicefactory_p.h src/services/ktraderparsetree.cpp src/services/ktraderparsetree_p.h src/sycoca/ksycocaentry.h To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure updated this revision to Diff 74334. dfaure added a comment. Port to erase(remove_if), add unittest for OnlyShowIn, which showed inconsistencies => now removed from results of both methods. REPOSITORY R309 KService CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25698?vs=74073=74334 BRANCH kapplicationtrader REVISION DETAIL https://phabricator.kde.org/D25698 AFFECTED FILES autotests/CMakeLists.txt autotests/kapplicationtradertest.cpp autotests/kservicetest.cpp src/CMakeLists.txt src/services/kapplicationtrader.cpp src/services/kapplicationtrader.h src/services/kmimetypetrader.cpp src/services/kservicefactory.cpp src/services/kservicefactory_p.h src/services/ktraderparsetree.cpp src/services/ktraderparsetree_p.h src/sycoca/ksycocaentry.h To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
broulik added inline comments. INLINE COMMENTS > dfaure wrote in kapplicationtrader.cpp:87 > Order is very important here, it's the order of preference. > > But doesn't erase(remove_if()) preserve order? I thought it did. cppreference says it does: > Relative order of the elements that remain is preserved and the physical size > of the container is unchanged REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure updated this revision to Diff 74073. dfaure added a comment. Improve docu REPOSITORY R309 KService CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25698?vs=73890=74073 BRANCH kapplicationtrader REVISION DETAIL https://phabricator.kde.org/D25698 AFFECTED FILES autotests/CMakeLists.txt autotests/kapplicationtradertest.cpp autotests/kservicetest.cpp src/CMakeLists.txt src/services/kapplicationtrader.cpp src/services/kapplicationtrader.h src/services/kmimetypetrader.cpp src/services/kservicefactory.cpp src/services/kservicefactory_p.h src/services/ktraderparsetree.cpp src/services/ktraderparsetree_p.h src/sycoca/ksycocaentry.h To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure marked 2 inline comments as done. dfaure added inline comments. INLINE COMMENTS > dhaumann wrote in kapplicationtrader.cpp:87 > 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 :-) Order is very important here, it's the order of preference. But doesn't erase(remove_if()) preserve order? I thought it did. > dhaumann wrote in kapplicationtrader.h:80 > method without training e. LOL, I'm trying but sometimes the French in me takes over :) > dhaumann wrote in kapplicationtrader.h:90 > Maybe mention when this function is useful? For me it looks like a private > helper function. Why is it public? 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. REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dhaumann added inline comments.
INLINE COMMENTS
> kapplicationtrader.cpp:87
> +KService::List::iterator it = lst.begin();
> +while (it != lst.end()) {
> +KService::Ptr serv = *it;
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 :-)
> kapplicationtrader.h:48
> +{
> +using FilterFunc = std::function;
> +
Please document this typedef so that FilterFunc will be clickable in the
generated API documentation.
> kapplicationtrader.h:80
> + *
> + * This a convenience methode for queryByMimeType(mimeType).at(0), with
> a check for empty.
> + *
method without training e.
> kapplicationtrader.h:90
> +/**
> + * Returns true if @p pattern matches a subsequence of the string @p
> text.
> + * For instance the pattern "libremath" matches the text "LibreOffice
> Math", assuming
Maybe mention when this function is useful? For me it looks like a private
helper function. Why is it public?
REPOSITORY
R309 KService
REVISION DETAIL
https://phabricator.kde.org/D25698
To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure updated this revision to Diff 73890. dfaure marked an inline comment as done. dfaure added a comment. Simplify docu for query() REPOSITORY R309 KService CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25698?vs=73858=73890 BRANCH kapplicationtrader REVISION DETAIL https://phabricator.kde.org/D25698 AFFECTED FILES autotests/CMakeLists.txt autotests/kapplicationtradertest.cpp autotests/kservicetest.cpp src/CMakeLists.txt src/services/kapplicationtrader.cpp src/services/kapplicationtrader.h src/services/kmimetypetrader.cpp src/services/kservicefactory.cpp src/services/kservicefactory_p.h src/services/ktraderparsetree.cpp src/services/ktraderparsetree_p.h src/sycoca/ksycocaentry.h To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure marked 3 inline comments as done. dfaure added inline comments. INLINE COMMENTS > aacid wrote in kapplicationtrader.h:56 > 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? 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. REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
aacid added inline comments. INLINE COMMENTS > kapplicationtrader.h:56 > + * true for all services, this would return the complete list of all > + * installed applications (slow). > + * 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? REPOSITORY R309 KService REVISION DETAIL https://phabricator.kde.org/D25698 To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D25698: New query mechanism for applications: KApplicationTrader
dfaure updated this revision to Diff 73858. 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. dfaure edited the test plan for this revision. dfaure added reviewers: aacid, davidedmundson. dfaure removed subscribers: aacid, dhaumann. dfaure added a comment. Integrate isSubsequence, fix docu, reword commit log REPOSITORY R309 KService CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D25698?vs=73800=73858 BRANCH kapplicationtrader REVISION DETAIL https://phabricator.kde.org/D25698 AFFECTED FILES autotests/CMakeLists.txt autotests/kapplicationtradertest.cpp autotests/kservicetest.cpp src/CMakeLists.txt src/services/kapplicationtrader.cpp src/services/kapplicationtrader.h src/services/kmimetypetrader.cpp src/services/kservicefactory.cpp src/services/kservicefactory_p.h src/services/ktraderparsetree.cpp src/services/ktraderparsetree_p.h src/sycoca/ksycocaentry.h To: dfaure, broulik, mart, vkrause, nicolasfella, aacid, davidedmundson Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns, dhaumann
