D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-10 Thread James Smith
smithjd added a comment.


  This test fails:
  
  FAIL!  : TagLibExtractorTest::testMp4(mp4) Compared values are not the same
  
Actual   (resultMp4.properties().value(Property::Rating).toInt()): 0
Expected (8) : 8
Loc: 
[/home/enderw/Code/kfilemetadata/autotests/taglibextractortest.cpp(381)]
  
  Is this a new failure?

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-10 Thread Alexander Stippich
This revision was automatically updated to reflect the committed changes.
Closed by commit R286:649555ee3182: Rewrite the taglib extractor to use the 
generic PropertyMap interface (authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D18826?vs=53587=53599#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18826?vs=53587=53599

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

AFFECTED FILES
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-10 Thread Stefan Brüns
bruns accepted this revision.
bruns added a comment.
This revision is now accepted and ready to land.


  Thanks, looks good to me now.

REPOSITORY
  R286 KFileMetaData

BRANCH
  arcpatch-D18826

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-10 Thread Alexander Stippich
astippich updated this revision to Diff 53587.
astippich marked an inline comment as done.
astippich added a comment.


  - spelling fixes

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18826?vs=53585=53587

BRANCH
  arcpatch-D18826

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-10 Thread Alexander Stippich
astippich marked 3 inline comments as done.
astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibextractor.cpp:364
> This intermediate list is not required, you can directly call result->add() 
> for each attribute in lstASF.

After further investigation there is no need to look for more than one entry, 
since there cannot be duplicated keys. This is also what TagLib does 
internally. So I changed it to be more inline with the generic extraction.

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-10 Thread Alexander Stippich
astippich updated this revision to Diff 53585.
astippich added a comment.


  - update

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18826?vs=52890=53585

BRANCH
  arcpatch-D18826

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-08 Thread Stefan Brüns
bruns requested changes to this revision.
bruns added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> taglibextractor.cpp:364
> +lstASF = asfTags->attribute("Author");
> +QStringList authors;
>  for (const auto& attribute : qAsConst(lstASF)) {

This intermediate list is not required, you can directly call result->add() for 
each attribute in lstASF.

> taglibextractor.cpp:376
>  lstASF = asfTags->attribute("WM/Writer");
> +QStringList lyricists;
>  for (const auto& attribute : qAsConst(lstASF)) {

This intermediate list is not required, you can directly call `result->add()` 
for each attribute in lstASF.

> taglibextractor.cpp:392
> +if (!lstASF.isEmpty()) {
> +const auto attribute = lstASF.front();
> +result->add(Property::Publisher, 
> TStringToQString(attribute.toString()).trimmed());

Why only the first element?

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-08 Thread Alexander Stippich
astippich added a comment.


  ping

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-01 Thread Alexander Stippich
astippich added inline comments.

INLINE COMMENTS

> bruns wrote in taglibextractor.cpp:208
> Can you also use the same style as above, i.e. `const auto` and `for( : )`?

Oh, damn copy

> smithjd wrote in taglibextractor.cpp:195
> lstASF = asfTags->attribute("WM/Writer");
> ...
> 
> The existing code does this.

I wanted to avoid the extra querying, but apparently I already did the same for 
other special cases, so I did as you suggested

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-03-01 Thread Alexander Stippich
astippich updated this revision to Diff 52890.
astippich marked 8 inline comments as done.
astippich added a comment.


  - rebase on master
  - implement review comments
  - cleanup includes

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18826?vs=52679=52890

BRANCH
  rewrite_taglib_extractor

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-27 Thread Stefan Brüns
bruns added inline comments.

INLINE COMMENTS

> taglibextractor.cpp:184
> +if (savedProperties.contains("COMPOSER")) {
> +const auto  composersString = 
> TStringToQString(savedProperties["COMPOSER"].toString(";")).trimmed();
> +const auto composers = contactsFromString(composersString);

doubled space

> taglibextractor.cpp:208
> +QStringList conductors = contactsFromString(conductorString);
> +foreach(const QString& arr, conductors) {
> +result->add(Property::Conductor, arr);

Can you also use the same style as above, i.e. `const auto` and `for( : )`?

> taglibextractor.cpp:215
> +QStringList arrangers = contactsFromString(arrangerString);
> +foreach(const QString& arr, arrangers) {
> +result->add(Property::Arranger, arr);

dito, and below ...

> taglibextractor.cpp:285
> +/* Special handling because TagLib::PropertyMap matches "TPUB" to "LABEL"
> + * Insert manually for Publisher */
> +lstID3v2 = Id3Tags->frameListMap()["TPUB"];

`*/` on a separate line

> taglibextractor.cpp:300
> +   a 5 stars rating to a range of 0-255 for MP3.
> +   Match it to baloo rating with a range of 0 - 10 */
> +lstID3v2 = Id3Tags->frameListMap()["POPM"];

`*/` on separate line, leading `*` on other lines

> taglibextractor.cpp:300
> +   a 5 stars rating to a range of 0-255 for MP3.
> +   Match it to baloo rating with a range of 0 - 10 */
> +lstID3v2 = Id3Tags->frameListMap()["POPM"];

s/Match/Map/

> taglibextractor.cpp:330
> +   with a range of 0 to 100 (stored in steps of 10) and make it 
> compatible
> +   with baloo rating with a range from 0 to 10 */
> +TagLib::MP4::ItemListMap::Iterator itRating = allTags.find("rate");

dito

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-26 Thread James Smith
smithjd added inline comments.

INLINE COMMENTS

> astippich wrote in taglibextractor.cpp:195
> I wanted to do so first, but that will require to also put the PropertyMap 
> into the extractAsfTags method, which I think is not worth it.

lstASF = asfTags->attribute("WM/Writer");
...

The existing code does this.

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-26 Thread Alexander Stippich
astippich updated this revision to Diff 52679.
astippich added a comment.


  - remove now unnecessary include

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18826?vs=52675=52679

BRANCH
  rewrite_taglib_extractor

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-26 Thread Alexander Stippich
astippich marked an inline comment as done.
astippich added inline comments.

INLINE COMMENTS

> smithjd wrote in taglibextractor.cpp:195
> Can this be moved into the asf-specific extractions?

I wanted to do so first, but that will require to also put the PropertyMap into 
the extractAsfTags method, which I think is not worth it.

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-26 Thread Alexander Stippich
astippich updated this revision to Diff 52675.
astippich added a comment.


  - return early if map is empty

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D18826?vs=51128=52675

BRANCH
  rewrite_taglib_extractor

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-24 Thread James Smith
smithjd added inline comments.

INLINE COMMENTS

> taglibextractor.cpp:109
> +{
> +if (savedProperties.contains("TITLE")) {
> +result->add(Property::Title, 
> TStringToQString(savedProperties["TITLE"].toString()).trimmed());

This could return early if the property map is empty.

> taglibextractor.cpp:195
>  }
> -
> -itApe = lstApe.find("PERFORMER");
> -if (itApe != lstApe.end()) {
> -if (!data.performer.isEmpty()) {
> -data.performer += ", ";
> +/* Lyricist is called "WRITER" for wma/asf files */
> +if (savedProperties.contains("WRITER")) {

Can this be moved into the asf-specific extractions?

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-17 Thread Alexander Stippich
astippich added a comment.


  ping. I know this is quite a large diff, but it fixes a potential crash

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-07 Thread Alexander Stippich
astippich edited the test plan for this revision.

REPOSITORY
  R286 KFileMetaData

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

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


D18826: Rewrite the taglib extractor to use the generic PropertyMap interface

2019-02-07 Thread Alexander Stippich
astippich created this revision.
astippich added reviewers: ngraham, bruns, mgallien.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
astippich requested review of this revision.

REVISION SUMMARY
  Rewrite the taglib extractor to use taglib's
  PropertyMap. Since this largely unifies the handling of the
  different tag formats, but not quite, a lot of code is removed.
  The resulting code is also faster. Additionally, this avoids the
  usage of a FileRef object, which fixes a potential crash due to
  a known bug in taglib.
  
  BUG: 403902

REPOSITORY
  R286 KFileMetaData

BRANCH
  rewrite_taglib_extractor

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h

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