D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[Albert Astals Cid]

aacid added inline comments.

INLINE COMMENTS

> kapplicationtrader.h:57
> + *
> + * The @p constraint language is rather full.  The most common
> + * keywords are AND, OR, NOT, IN, and EXIST, all used in an

all this "constraint" definition needs updating

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: aacid, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure updated this revision to Diff 73800.
dfaure added a comment.


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

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25698?vs=70787=73800

BRANCH
  kapplicationtrader

REVISION DETAIL
  https://phabricator.kde.org/D25698

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kapplicationtradertest.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/kservicetypetrader.h
  src/sycoca/ksycocaentry.h

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: aacid, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure added a comment.


  Part-loading discussion continues in https://phabricator.kde.org/T12173.
  
  Meanwhile this review request is about application lookup :)

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: aacid, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[Albert Astals Cid]

aacid added a comment.


  > 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

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: aacid, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure added a comment.


  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("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?

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: aacid, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[Albert Astals Cid]

aacid added a comment.


  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

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: aacid, dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, 
bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[Nicolas Fella]

nicolasfella added a comment.


  Works fine for my rather simple KDE Connect use case
  
  +1

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure updated this revision to Diff 70787.
dfaure added a comment.


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

REPOSITORY
  R309 KService

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25698?vs=70785=70787

BRANCH
  kapplicationtrader

REVISION DETAIL
  https://phabricator.kde.org/D25698

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kapplicationtradertest.cpp
  autotests/kservicetest.cpp
  autotests/kservicetest.h
  src/CMakeLists.txt
  src/services/kapplicationtrader.cpp
  src/services/kapplicationtrader.h
  src/services/kservicetypetrader.h
  src/sycoca/ksycocaentry.h

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> kapplicationtrader.h:33
> + *
> + * Example: say that you want to the list of all applications that can 
> handle PNG images.
> + * The code would look like:

Typo: you want to _get_...

REPOSITORY
  R309 KService

REVISION DETAIL
  https://phabricator.kde.org/D25698

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: dhaumann, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25698: New class KApplicationTrader, to replace KMimeTypeTrader and KServiceTypeTrader

[David Faure]

dfaure created this revision.
dfaure added reviewers: broulik, mart, vkrause, nicolasfella.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  The old traders could lookup both applications and plugins, but plugins
  should nowadays be loaded via their JSON file, no desktop-file trader
  needed anymore.
  
  To get rid of plugins and servicetypes from the API, and keep
  only the application-lookup-related code, the older traders will be
  deprecated and their users should either switch to JSON for plugins 
(KPluginLoader)
  or KApplicationTrader for application desktop files.
  
  One API question remains:
   KApplicationTrader::self()->query("...") like the old traders, or
   KApplicationTrader::query("...") (static methods only, or making it a 
namespace, a bit procedural rather than OO), or
   KApplicationTrader().query("...") like QMimeDatabase/QFontDatabase (but with 
default ctors for performance) ?

TEST PLAN
  New unittest (partly based on kservicetest, but simplified and added new 
types of checks) passes

REPOSITORY
  R309 KService

BRANCH
  kapplicationtrader

REVISION DETAIL
  https://phabricator.kde.org/D25698

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/kapplicationtradertest.cpp
  autotests/kservicetest.cpp
  autotests/kservicetest.h
  src/CMakeLists.txt
  src/services/kapplicationtrader.cpp
  src/services/kapplicationtrader.h
  src/services/kservicetypetrader.h
  src/sycoca/ksycocaentry.h

To: dfaure, broulik, mart, vkrause, nicolasfella
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns