D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich abandoned this revision.

REPOSITORY
  R286 KFileMetaData

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

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

D11365: also test for value types in taglibextractortest and fix errors

[Stefan Brüns]

bruns added inline comments.
Restricted Application added a subscriber: kde-frameworks-devel.

INLINE COMMENTS

> mgallien wrote in taglibextractor.cpp:389
> Are you sure we need this cast ? I do not think we will ever need to have 
> negative track numbers added. The original type is unsigned int and should 
> exactly convey the fact that we do not expect negative numbers.

For CDs, there are hidden tracks, IIRC these have negative numbers.

REPOSITORY
  R286 KFileMetaData

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

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

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich planned changes to this revision.
astippich added a comment.


  In D11365#236274 , @michaelh wrote:
  
  > This patch should be split.
  >
  > 1. Test more properties
  > 2. Change return types of ...
  >
  >   Also 'fix errors' in the title is misleading because currently 
kfilemetadata works well.
  
  
  Sure, I can do that if it is not a problem that the new tests do not pass 
(temporarily). It is probably the best idea to create thorough tests for the 
way we'd like KFileMetaData/taglib to work, as they are lacking in several ways 
(and hence created the confusion I have had) . Then we can start fixing the 
errors.
  Can we reach a consent how it should behave in the end? e.g. should the 
result match the valueType in propertyinfo, should there be multiple properties 
with single strings or stringlists for multiple values? Any other concerns?
  @mgallien, what's your opinion here?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Michael Heidelbach]

michaelh added a comment.


  Since we're struggling with the same issues: T8196 
 and D10694 


REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Michael Heidelbach]

michaelh requested changes to this revision.
michaelh added a comment.


  This patch should be split.
  
  1. Test more properties
  2. Change return types of ...
  
  Also 'fix errors' in the title is misleading because currently kfilemetadata 
works well.

INLINE COMMENTS

> astippich wrote in taglibextractor.cpp:363
> That was actually the point for this. In propertyinfo, it says that this 
> property is a stringlist, but it was actually never one, just a simple 
> string. 
> For multiple values, there will be multiple entries in the property map with 
> a single string.  This is not considered within Elisa at least. We could also 
> go the other way and adjust the properties to match the behavior. 
> @michaelh agreed that stringlists are easier to handle

You have to ensure baloo searching still works, when string lists contains more 
than 1 items. I think it will break.

Generally IMO returning a string list is the natural thing to do here. 
On the other hand: Why is `PropertyInfo` announcing the value type when calling 
`.type()` on a QVariant would be sufficient. There must be a reason for using 
`PropertyMap` and giving a type hint in `PropertyInfo`. Unless somebody knows 
the reason this will need some investigation and a concerted action if we 
choose to change it.
I've put D10694  on hold because of that.

> astippich wrote in taglibextractor.cpp:393
> This one is a litte bit more tricky, as the year property could theoretically 
> also be negative (B.C.). Not really relevant for music, but maybe for written 
> documents, and still only very rarily :)

It depends on how you define 'releaseYear' if you think of it as year published 
negative dates can be ruled out. ;-)

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added inline comments.

INLINE COMMENTS

> mgallien wrote in taglibextractor.cpp:363
> Is it really needed to push directly a list ?
> You are ignoring the fact that the original developer intended add to be 
> called once for each value. I would prefer to keep doing that until we can be 
> absolutely sure that nothing breaks if we change that.

That was actually the point for this. In propertyinfo, it says that this 
property is a stringlist, but it was actually never one, just a simple string. 
For multiple values, there will be multiple entries in the property map with a 
single string.  This is not considered within Elisa at least. We could also go 
the other way and adjust the properties to match the behavior. 
@michaelh agreed that stringlists are easier to handle

> mgallien wrote in taglibextractor.cpp:389
> Are you sure we need this cast ? I do not think we will ever need to have 
> negative track numbers added. The original type is unsigned int and should 
> exactly convey the fact that we do not expect negative numbers.

I changed it so it matches its associated value type, I could also change the 
type in propertyinfo to uint

> mgallien wrote in taglibextractor.cpp:393
> idem

This one is a litte bit more tricky, as the year property could theoretically 
also be negative (B.C.). Not really relevant for music, but maybe for written 
documents, and still only very rarily :)

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Matthieu Gallien]

mgallien requested changes to this revision.
mgallien added a comment.
This revision now requires changes to proceed.


  Please fix the issues.

INLINE COMMENTS

> taglibextractor.cpp:363
> +if (!genresList.isEmpty()) {
> +result->add(Property::Genre, genresList);
>  }

Is it really needed to push directly a list ?
You are ignoring the fact that the original developer intended add to be called 
once for each value. I would prefer to keep doing that until we can be 
absolutely sure that nothing breaks if we change that.

> taglibextractor.cpp:370
> +const QStringList artists = contactsFromString(artistString);
> +result->add(Property::Artist, artists);
>  

idem

> taglibextractor.cpp:374
> +const QStringList composers = contactsFromString(composersString);
> +result->add(Property::Composer, composers);
>  

idem

> taglibextractor.cpp:385
> +const QStringList albumArtists = 
> contactsFromString(albumArtistsString);
> +result->add(Property::AlbumArtist, albumArtists);
>  }

idem

> taglibextractor.cpp:389
>  if (tags->track()) {
> -result->add(Property::TrackNumber, tags->track());
> +result->add(Property::TrackNumber, 
> static_cast(tags->track()));
>  }

Are you sure we need this cast ? I do not think we will ever need to have 
negative track numbers added. The original type is unsigned int and should 
exactly convey the fact that we do not expect negative numbers.

> taglibextractor.cpp:393
>  if (tags->year()) {
> -result->add(Property::ReleaseYear, tags->year());
> +result->add(Property::ReleaseYear, 
> static_cast(tags->year()));
>  }

idem

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added a comment.


  It's just testing each file for all available properties, one after another. 
This revision just adds missing bits to the existing test. Refactoring should 
be done separately imho

INLINE COMMENTS

> michaelh wrote in propertyinfo.cpp:98
> Looks unrelated to me.

It's not, composer should really be treated the same as e.g. artist or 
lyricist, as it can have multiple entries.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Michael Heidelbach]

michaelh added a comment.


  It is very hard for me understand what you are actually testing. Please  
(re-)consider to organize like below  (or similar). It would be closer to 
consumer's point of view. What do I get when I ask `kfilemetadata` for the 
artist of `test.m4a`?
  
void TagLibExtractorTest::testArtist_data()
{
QTest::addColumn("path");
QTest::addColumn("mime");
QTest::addColumn("prop");
QTest::addColumn("value");
QTest::addColumn("valueList");
QTest::addColumn("type");

QTest::addRow("m4a")
<< QFINDTESTDATA("samplefiles/test.m4a")
<< QStringLiteral( "audio/mp4")
<< Property::Artist
<< QStringLiteral("Artist")
<< QStringList{QStringLiteral("Artist")}
<< PropertyInfo(Property::Artist).valueType()
;
  
  ... more files
  }
  
void  TagLibExtractorTest::testArtist()
{
QFETCH(QString, path);
QFETCH(QString, mime);
QFETCH(KFileMetaData::Property::Property, prop);
QFETCH(QString, value);
QFETCH(QStringList, valueList);
QFETCH(QVariant::Type, type);
QScopedPointer plugin(new TagLibExtractor(this));
SimpleExtractionResult extracted(path, mime);
plugin->extract();
QCOMPARE(extracted.properties().value(prop).type(), type);
QCOMPARE(extracted.properties().value(prop), value);
QCOMPARE(extracted.properties().value(prop), valueList);

}

INLINE COMMENTS

> propertyinfo.cpp:98
>  d->displayName = i18nc("@label", "Composer");
> -d->valueType = QVariant::String;
> +d->valueType = QVariant::StringList;
>  d->shouldBeIndexed = false;

Looks unrelated to me.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added a comment.


  any feedback on this? I would like to land this and then get going with 
D10803  again

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added reviewers: mgallien, michaelh.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo, mgallien, michaelh
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added a comment.


  In D11365#227241 , @michaelh wrote:
  
  > Just in case you didn't use data-driven tests on purpose: 
https://doc.qt.io/qt-5/qttestlib-tutorial2-example.html
  
  
  Thanks for the hint, that's new for me. Anyways, there are subtle differences 
for each file and there will be more in the future, so I think it is better to 
leave it unchanged.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Michael Heidelbach]

michaelh added a comment.


  Just in case you didn't use data-driven tests on purpose: 
https://doc.qt.io/qt-5/qttestlib-tutorial2-example.html

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo
Cc: michaelh, #frameworks, ashaposhnikov, astippich, spoorun, nicolasfella, 
ngraham, alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich added reviewers: Frameworks, Baloo.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, #frameworks, #baloo
Cc: #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin

D11365: also test for value types in taglibextractortest and fix errors

[Alexander Stippich]

astippich created this revision.
Restricted Application added projects: Frameworks, Baloo.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  taglib tests never tested for the expected value type. add this to the tests 
and fix the resulting errors. also add some more metadata to the samplefiles

TEST PLAN
  Thorough testing with baloo and all users of KFileMetaData

REPOSITORY
  R286 KFileMetaData

BRANCH
  taglib_test

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mp3
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/propertyinfo.cpp

To: astippich
Cc: #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella, ngraham, 
alexeymin