D10803: handle more tags in taglibextractor

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:7f9de32eedff: handle more tags in taglibextractor 
(authored by astippich).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D10803?vs=30956=31950#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30956=31950

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

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/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D10803: handle more tags in taglibextractor

[David Faure]

dfaure added a comment.


  No. As soon as the RC1 tags are there, the message freeze is lifted.
  
  In case of a bugfix that comes in after RC1, I cherry-pick it into the 
release branch, so that master doesn't have to stay frozen for a week.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D10803: handle more tags in taglibextractor

[Matthieu Gallien]

mgallien added a comment.


  In D10803#244389 , @astippich 
wrote:
  
  > ping. Can I push? I'm asking again because I don't want to mess up anything 
before the frameworks release, and this one contains string changes
  
  
  I am not sure. I tried asking on #kde-devel.
  
  @dfaure should we wait after the final v5.45 tag to push this diff ?

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  ping. Can I push? I'm asking again because I don't want to mess up anything 
before the frameworks release, and this one contains string changes

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D10803: handle more tags in taglibextractor

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibextractortest.cpp:82
> Hmm, I disagree since they are simple and they don't leave room for ambiguity

Maybe just add a Prefix or Suffix for all QStrings?
Same applies to any Int values, you can never tell if the right property has 
been fetched.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: bruns, vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, 
astippich, spoorun, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  @mgallien this should be safe to land now since KF 5.45 has been tagged, 
right?

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, bruns, alexeymin

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

To: astippich, mgallien, ngraham, michaelh
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, astippich, 
spoorun, bruns, alexeymin

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh accepted this revision.
michaelh added a comment.
This revision is now accepted and ready to land.


  Thank you.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 30956.
astippich added a comment.


  - remove duplicate genre property for mp3

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30955=30956

BRANCH
  enhance_taglib

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

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/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 30955.
astippich marked an inline comment as done.
astippich added a comment.


  - fix formatting issues during rebase

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30951=30955

BRANCH
  enhance_taglib

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

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/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich marked 3 inline comments as done.
astippich added a comment.


  In D10803#237133 , @michaelh wrote:
  
  > Except for the white space stuff I'm still fine with this.
  
  
  Oops, sorry, messed that up during rebase.

INLINE COMMENTS

> michaelh wrote in taglibextractor.cpp:304
> Nitpick: These comments aren't really informative. The tagname is explicit 
> enough. 
> I don't mind if you leave them in.

You're right, I copied them from MP3s, but they make no sense for APE and OGG 
tags

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

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


  Except for the white space stuff I'm still fine with this.

INLINE COMMENTS

> taglibextractor.cpp:237
>  data.discNumber = itDiscNumber->second.toInt();
> +}   TagLib::MP4::ItemListMap::Iterator itCompilation = 
> allTags.find("cpil");
> +if (itCompilation != allTags.end()) {

New line please

> taglibextractor.cpp:304
>  
> +// Genre.
>  itMPC = lstMusepack.find("GENRE");

Nitpick: These comments aren't really informative. The tagname is explicit 
enough. 
I don't mind if you leave them in.

> taglibextractor.cpp:659
>  extractOgg(stream, mimeType, data);
> +
>  }

Unrelated white-space change

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  Okay, I changed my mind, I rebased on top of master. Since the other revision 
needs more time and the new properties behave like the old ones, I'd like to 
end this long journey and land this revision if no-one objects :)

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 30951.
astippich added a comment.


  - update value types

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30950=30951

BRANCH
  enhance_taglib

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

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/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 30950.
astippich added a comment.


  - rebase on top of master

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=28467=30950

BRANCH
  enhance_taglib

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

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/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  All of a sudden :)
  I've been quiet about this because I'd like to resolve the issues regarding 
strings/stringlists first, so if you have some spare time, please have a look 
at D11365 . I will rebase after this one 
landed

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.


  Thanks for the heads-up, Matthieu. Looks good and works well for me too!

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh accepted this revision.
michaelh added a comment.


  Tooltips cope well

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a reviewer: michaelh.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Matthieu Gallien]

mgallien accepted this revision.
mgallien added a comment.
This revision is now accepted and ready to land.


  I am really sorry. I had missed that you had modified the diff to only 
include the safe set of changes. I should really apologize.
  Thanks for your work, it is very much appreciated.

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a comment.


  In D10694#222086 , @astippich 
wrote:
  
  > Also, since this discussion also applies for the taglibextractor: Is a 
string list preferred for new properties in KFileMetadata when multiple entries 
are possible? I think right now most of the properties are using a single 
concatenated string.
  
  
  Looks like except for composer it is a string list when persons are involved. 
In general, I think, going from stringlist -> string causes less pain/errors 
than the other way around.

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a comment.


  I played around a bit with your previous diff (lyrics included). And modified 
some `test.ogg` and `test.mp3`. Lyrics is not metadata, but if we include them 
they will be searchable:
  
$ baloosearch lyrics:carefree
~/samplefiles/test.ogg
~/samplefiles/test.mp3
Elapsed: 0.76411 msecs
  
  which is very nice, IMO. Will work in KRunner too.
  Info panel looks good.
  Tooltips: buggy. see here 

  
  I'm a bit sorry, that I'm so back and forth with this. Finding a proper 
compromise takes time.
  
  In conclusion for me:
  Singular is OK
  For a first shot go for
  
  - Performer
  - Conductor
  - Ensemble
  - (and maybe) Arranger
  
  Fixing the tooltips is only half-done. Let me finish fixing them and then do 
the rest of the tags.
  
  Side note:
  There is a big advantage of using extended attributes for Rating: It does not 
write to the file, hence file times are preserved. Otherwise that information 
will be lost. Rating is the most volatile tag, changing over time.

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich updated this revision to Diff 28467.
astippich added a comment.


  - remove lyrics and map encodedby to generator

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=28300=28467

BRANCH
  enhance_taglib

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

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/properties.h
  src/propertyinfo.cpp

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

D10803: handle more tags in taglibextractor

[Nathaniel Graham]

ngraham added a comment.


  Phew, what a conversation!
  
  For the issue of the rating, perhaps we could sync the tag metadata with the 
baloo metadata, if the baloo rating metadata is already empty. I might even go 
so far as to overwrite the baloo rating metadata with the tag rating metadata; 
I can't think of a use case for wanting to maintain two separate rating values 
for the same file. And it would be really cool to be able to see and manipulate 
the rating in Dolphin, and have that rating reflected in your music player.
  
  For the issue of singular vs plural, is baloo-widgets smart enough to 
conditionally display a plural form of the string when there are multiple 
entries (e.g 
  "performer: X"; "performers X, Y, Z")? Could we make it smart enough? If not, 
let's stick with singular to avoid making our lives too difficult for now.
  
  For the issue of Dolphin's Tooltip, perhaps we should make it become 
multi-row when there's a huge amount of content, or the font size should 
automatically reduce in size. Or maybe the tooltip should only display content 
from among a curated selection of tags that we deem to be the most important, 
and we can rely on the Information panel using used to access the full set
  
  > Let's remove all the undecided stuff and land this one quickly for the 
benefit of everybody. Do you agree ?
  
  +1

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> taglibextractortest.cpp:82
>  
>  QCOMPARE(resultFlac.properties().value(Property::Title), 
> QVariant(QStringLiteral("Title")));
>  QCOMPARE(resultFlac.properties().value(Property::Artist), 
> QVariant(QStringLiteral("Artist")));

Just a hint: The code would be easier to read if the title of the sample file 
was
'I love you so much' or 'Desaster will come' just not the same as the propery 
name. (Same for most of the props below)

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a comment.


  In D10803#215270 , @mgallien wrote:
  
  > This is not necessary. You could use a QImage or a QPixmap. You can put 
them in a QVariant and they should be converted to an empty QString.
  
  
  Please consider writing a plugin for KIO:PreviewJob (for the covers) anyway. 
It would be more widely useable. I'd like to see a/the cover/s in Dolphin too.
   :-)  We could also argue a little about covers being metadata or primary 
data. For starters I'd consider a reference to a cover as metadata. Still :-)

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks

D10803: handle more tags in taglibextractor

[Matthieu Gallien]

mgallien added a comment.


  In D10803#214655 , @astippich 
wrote:
  
  > In D10803#213783 , @michaelh 
wrote:
  >
  > > In D10803#213767 , @astippich 
wrote:
  > >
  > > > I don't know dolphin works, but given how KFileMetadata is designed, 
the new tags should be ignored until explicit support is added in dolphin.
  > >
  > >
  > > Please try that. For unknown types baloo-widgets (responsible for 
infopanel/tooltips) falls back to `value.toString()`, no idea what will happen 
when you feed it with binary data. The cover properties return binary data, 
right?
  >
  >
  > The cover is binary data, right. And I think I was wrong. While I don't 
fully understand yet how baloo-widgets work, from what I've read all available 
properties are automatically queried. The binary data would then be converted 
to a string as you said. So adding the cover this way may not be a good 
solution. A different interface should be created for image/binary data.
  >  I think I will split this into two diffs and do the cover images 
separately.
  
  
  This is not necessary. You could use a QImage or a QPixmap. You can put them 
in a QVariant and they should be converted to an empty QString.
  As far as I understand, in Elisa, we would need to access them with 
QQuickImageProvider. All in all, well done job.
  
  I am not sure for the rating given that there is also a rating (the one used 
in Elisa) that is stored as an extended file system attribute independent from 
the file mime type. One limit of this rating is that it is specific to the 
KFileMetaData API users. Do you have an idea ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a comment.


  A thumbnail extractor might be better suited for this job.
  On my system(tumbleweed) there is a `kde-audio-thumbnail.` I don't know 
anything more about it.
  Maybe `ffmpeg-thumbnails` can do that?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  In D10803#213783 , @michaelh wrote:
  
  > In D10803#213767 , @astippich 
wrote:
  >
  > > I don't know dolphin works, but given how KFileMetadata is designed, the 
new tags should be ignored until explicit support is added in dolphin.
  >
  >
  > Please try that. For unknown types baloo-widgets (responsible for 
infopanel/tooltips) falls back to `value.toString()`, no idea what will happen 
when you feed it with binary data. The cover properties return binary data, 
right?
  
  
  The cover is binary data, right. And I think I was wrong. While I don't fully 
understand yet how baloo-widgets work, from what I've read all available 
properties are automatically queried. The binary data would then be converted 
to a string as you said. So adding the cover this way may not be a good 
solution. A different interface should be created for image/binary data.
  I think I will split this into two diffs and do the cover images separately.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks

D10803: handle more tags in taglibextractor

[Matthieu Gallien]

mgallien added a comment.


  I have created T8079  to work on baloo 
database update when extractors are modified and returned different data.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a comment.


  In D10803#213767 , @astippich 
wrote:
  
  > I don't know dolphin works, but given how KFileMetadata is designed, the 
new tags should be ignored until explicit support is added in dolphin.
  
  
  Please try that. For unknown types baloo-widgets (responsible for 
infopanel/tooltips) falls back to `value.toString()`, no idea what will happen 
when you feed it with binary data. The cover properties return binary data, 
right?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks, kmorwinski

D10803: handle more tags in taglibextractor

[Matthieu Gallien]

mgallien added a subscriber: dfaure.
mgallien added inline comments.

INLINE COMMENTS

> astippich wrote in properties.h:168
> I noticed the typo as well, but I think this is a matter of API 
> compatibility, so I left it unchanged. Matthieu should know.

It is part of the source compatibility promise for a major version of 
KFileMetaData. However, you can add a new one and push the data to both 
properties. This way, we could deprecate the one with the typo. @dfaure do you 
have another solution ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: dfaure, michaelh, ngraham, #frameworks, kmorwinski

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  I don't know dolphin works, but given how KFileMetadata is designed, the new 
tags should be ignored until explicit support is added in dolphin.

INLINE COMMENTS

> michaelh wrote in properties.h:164
> ?

Oops, seems like a wrong copy & paste :)  will fix

> michaelh wrote in properties.h:168
> Will we break ABI with correct 'Language'?

I noticed the typo as well, but I think this is a matter of API compatibility, 
so I left it unchanged. Matthieu should know.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: michaelh, ngraham, #frameworks, kmorwinski

D10803: handle more tags in taglibextractor

[Michael Heidelbach]

michaelh added a comment.


  Nice! How do the infopanel or tooltips of dolphin look with this?

INLINE COMMENTS

> properties.h:164
>  /**
> - * The language the document is written in. This directly maps to the
> + * The language the document is written in. Thiof a media file.s 
> directly maps to the
>   * 'dc:language' tag from DublinCore. We do NOT employ any language

?

> properties.h:168
>   */
>  Langauge,
>  

Will we break ABI with correct 'Language'?

> propertyinfo.cpp:586
>  { QStringLiteral("linecount"), Property::LineCount },
>  { QStringLiteral("language"), Property::Langauge },
>  { QStringLiteral("copyright"), Property::Copyright },

Language?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: michaelh, ngraham, #frameworks, kmorwinski

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.


  First of all, please review carefully for ABI compatibility, I do not know 
what to watch for that.
  
  So I decided to walk into the minefield that tags are. Everyone implements it 
differently. I've implemented some tags that seem to be supported across a wide 
range of applications (Kid3, Amarok, Kodi... ) but no guarantee that they will 
work everywhere. Some tags were requested on LWN.net in the comments for the 
first alpha (regarding classical music), but most of them only work for Vorbis 
comments as the others lack support.
  The album cover image is currently passed as a QByteArray which then needs to 
be further processed by the application. I don't know if that is the best 
approach, but I think is the most generic one. Comments welcome.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: #frameworks, michaelh

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich created this revision.
astippich added a reviewer: mgallien.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
astippich requested review of this revision.

REVISION SUMMARY
  adds the ability to read more tags through taglib when supported by the 
format. prominent feature is the support for reading (front) cover files mp3, 
flac, ogg, opus and mp4. tests including sample files are adjusted for the new 
features.
  Prerequisite for T6255 

REPOSITORY
  R286 KFileMetaData

BRANCH
  enhance_taglib

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

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

To: astippich, mgallien
Cc: #frameworks, michaelh