D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

bruns added a comment.


  Fixed ...

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: bcooksley, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Ben Cooksley]

bcooksley added a comment.


  You can rely on shared-mime-info being present on all platforms (Linux, 
FreeBSD, Windows, Android) yes.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: bcooksley, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

bruns added a comment.


  In D16593#354311 , @bcooksley 
wrote:
  
  > As noted in the build log on the CI system, kfilemetadata doesn't depend on 
kcoreaddons so you therefore cannot rely on the kde5.xml file being available:
  >
  >   [kf5-qt5 SUSEQt5.9] Running shell script
  >+ python3 -u ci-tooling/helpers/prepare-dependencies.py --product 
Frameworks --project kfilemetadata --branchGroup kf5-qt5 --environment 
production --platform SUSEQt5.9 --installTo /home/jenkins//install-prefix/
  >Retrieving: Frameworks-extra-cmake-modules-kf5-qt5
  >Retrieving: Frameworks-ki18n-kf5-qt5
  >Retrieving: Frameworks-karchive-kf5-qt5
  >
  > The CI system is working correctly in this case and as designed, only 
allowing actual dependencies of a project to be used during it's build and 
testing.
  
  
  Obviously the tests require a sufficient dataset for a working QMimeDatabase. 
This hasn't changed, only the extent of required mime types has. Up until now, 
the implicitly installed definitions (shared-mime-info?) did the job.
  
  So, which mime-types are guaranteed by the CI system to be available? Can one 
rely on shared-mime-info? What about Windows/macOS?
  
  There are dozens of mime types in shared-mime-info which are sub-class-of 
application/xml, see:
  `grep -E 'mime-type |sub-class-of.*application/xml'  
/usr/share/mime/packages/freedesktop.org.xml | grep -B1 sub-class-of | head `

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: bcooksley, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Ben Cooksley]

bcooksley added a comment.


  As noted in the build log on the CI system, kfilemetadata doesn't depend on 
kcoreaddons so you therefore cannot rely on the kde5.xml file being available:
  
[kf5-qt5 SUSEQt5.9] Running shell script
+ python3 -u ci-tooling/helpers/prepare-dependencies.py --product 
Frameworks --project kfilemetadata --branchGroup kf5-qt5 --environment 
production --platform SUSEQt5.9 --installTo /home/jenkins//install-prefix/
Retrieving: Frameworks-extra-cmake-modules-kf5-qt5
Retrieving: Frameworks-ki18n-kf5-qt5
Retrieving: Frameworks-karchive-kf5-qt5
  
  The CI system is working correctly in this case and as designed, only 
allowing actual dependencies of a project to be used during it's build and 
testing.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: bcooksley, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

bruns added a subscriber: bcooksley.
bruns added a comment.


  In D16593#353801 , @astippich 
wrote:
  
  > This or the commit before causes CI to fail the ExtractorCollectionTest on 
Qt 5.9, 5.10, and Windows
  
  
  Most likely the new check in D16592 . It 
fails to find an extractor for application/x-kvtml
  
  Apparently it either does not find "application/x-kvtml" at all, or it is not 
registered as subtype of application/xml.
  
  This may be a setup issue of the CI system - @bcooksley ?
  
  Currently it relies on the application/x-kvtml entry from 
mime/packages/kde5.xml, which is part of kcoreaddons. Should be possible to 
switch to another entry if this is not available.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: bcooksley, kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, 
astippich, spoorun, ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Alexander Stippich]

astippich added a comment.


  This or the commit before causes CI to fail the ExtractorCollectionTest on Qt 
5.9, 5.10, and Windows

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
bruns marked 2 inline comments as done.
Closed by commit R286:497a69ca846b: [ExtractorCollection] Use only best 
matching extractor plugin (authored by bruns).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16593?vs=44658&id=44772

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

AFFECTED FILES
  autotests/extractorcollectiontest.cpp
  src/extractorcollection.cpp
  src/extractorcollection.h

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Alexander Stippich]

astippich accepted this revision.
astippich added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

> bruns wrote in extractorcollection.cpp:163
> http://doc.qt.io/qt-5/qdebug.html#setAutoInsertSpaces is true by default.

I meant the code, not the output :)

> bruns wrote in extractorcollection.cpp:164
> > doesn't returning here cause immediate return after the first matching 
> > extractor for the mimetype has been found? that doesn't fit to the first 
> > paragraph of the summary.
> 
> Yes, and this is the intended change - best matching. We don't want 
> text/plain if we have a better one.

That's what I figured, the summary was a little bit misleading.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

bruns edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

bruns marked 2 inline comments as done.
bruns added inline comments.

INLINE COMMENTS

> astippich wrote in extractorcollection.cpp:163
> nitpick: extra space before "for"

http://doc.qt.io/qt-5/qdebug.html#setAutoInsertSpaces is true by default.

> astippich wrote in extractorcollection.cpp:164
> doesn't returning here cause immediate return after the first matching 
> extractor for the mimetype has been found? that doesn't fit to the first 
> paragraph of the summary.
> "Otherwise, all matches for all mime ancestors are returned, even if one 
> plugin is a much better match than any other." sounds to me that all plugins 
> of all ancestors are returned.

> doesn't returning here cause immediate return after the first matching 
> extractor for the mimetype has been found? that doesn't fit to the first 
> paragraph of the summary.

Yes, and this is the intended change - best matching. We don't want text/plain 
if we have a better one.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> extractorcollection.cpp:163
> +if (!plugins.isEmpty()) {
> +qCDebug(KFILEMETADATA_LOG) << "Using inherited mimetype" << 
> ancestor <<  "for" << mimetype;
> +return plugins;

nitpick: extra space before "for"

> extractorcollection.cpp:164
> +qCDebug(KFILEMETADATA_LOG) << "Using inherited mimetype" << 
> ancestor <<  "for" << mimetype;
> +return plugins;
>  }

doesn't returning here cause immediate return after the first matching 
extractor for the mimetype has been found? that doesn't fit to the first 
paragraph of the summary.
"Otherwise, all matches for all mime ancestors are returned, even if one plugin 
is a much better match than any other." sounds to me that all plugins of all 
ancestors are returned.

REPOSITORY
  R286 KFileMetaData

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

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams

D16593: [ExtractorCollection] Use only best matching extractor plugin

[Stefan Brüns]

bruns created this revision.
bruns added reviewers: Frameworks, astippich.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
bruns requested review of this revision.

REVISION SUMMARY
  If there is a direct match for a mime type, only the matching plugin(s)
  is (are) returned, inheritance is not considered. Otherwise, all matches
  for all mime ancestors are returned, even if one plugin is a much better
  match than any other.
  
  Up until now, the only extractor selected by inheritance was the plain
  text extractor, so there was one fuzzy matched plugin at most. With the
  XML extractor, the XML extractor should be preferred over the plain test
  extractor.
  
  Depends on D16592 

REPOSITORY
  R286 KFileMetaData

BRANCH
  master

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

AFFECTED FILES
  autotests/extractorcollectiontest.cpp
  src/extractorcollection.cpp
  src/extractorcollection.h

To: bruns, #frameworks, astippich
Cc: kde-frameworks-devel, #baloo, ashaposhnikov, michaelh, astippich, spoorun, 
ngraham, bruns, abrahams