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&id=31950#toc

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10803?vs=30956&id=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&id=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&id=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&id=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&id=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&id=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

[Alexander Stippich]

astippich added a comment.


  In D10803#216677 , @michaelh wrote:
  
  > In D10803#216419 , @astippich 
wrote:
  >
  > > While I think you have a point here, KFileMetaData sticked to singular 
names before, e.g. author, where also multiple persons are possible. So I think 
we should keep that behavior.
  >
  >
  > We're stuck with that. Nevertheless, it's wrong IMO. All I'm saying is: 
Before simply repeat this error, let's give it some thought.
  
  
  Well, if we would go to plurals, it would be wrong the other way if there are 
only single entries . Also, I think the properties don't have to plural, they 
are internal. The labels of them should adjust for the singular or plural case. 
That said, I don't think we can achieve that without breaking the programming 
interface, so we're stuck with that.
  
  >> To be honest, I don't know which tooltip you mean. Can you post a picture? 
Is this an option I have to switch on?
  > 
  > Configure Dolphin > General > Show Tooltips
  >  Currently tooltips are a little broken. see D9973 

  
  Okay, so here is a screenshot. Looks not so nice with lots of entries. Imho 
there should be a maximum number of properties, and a scrollbar should be used 
if that threshold is exceeded.
  
  F5736836: Screenshot_20180302_205701.png 

  
  >> I think it behaves quite well and reasonable with many tags as it shows a 
scrollbar. I don't think that there is a better solution.
  > 
  > I haven't tested this. So I don't really know. And you're probably right. I 
forgot that the choice of properties depends on the file type. A screenshot of 
the dialog would be nice (large enough to display everthing).
  
  The infopanel looks perfectly fine:
  
  F5736838: Screenshot_20180302_204919.png 

  
  >>> 1. The property names should be choosen to be as generic as possible to 
make them reusable by other file types.
  >> 
  >> I went through the properties again and only found that we might want to 
use the "generator" property instead of a new "encodedby" property. Do you have 
any other suggestion?
  > 
  > Not yet. To illustrate what I'm thinking of:
  >  Conductor and Director (of a Movie) are analogous. It would be nice to 
have a term covering both. . Ditto Lyricist and Screenwriter, and maybe more.
  >  I don't know if that is possible - at least I have no idea yet. We should 
just give it some (more) thought.
  >  Or the other way around: 
  >  I find the distinction between 'Release Year' for media and 'Creation 
Date' (which should correctly be 'Publication Date') in for EBooks annoying and 
confusing. We should not repeat that, if possible.
  
  I agree that we should think that through. Once committed, we're stuck with 
that for the time being. In this specific case, I would actually argue that the 
conductor and director are different. Imho there is only limited potential to 
find tags that work for everything. Video, Music, text etc. are different after 
all.
  
  > The property names are important for searching with baloo. Everything comes 
together here. (I'll elaborate this on demand)
  >  The code you are working on exemplifies pretty well, that everybody is 
brewing his own soup when is comes to metadata and tags. And this is only 
audio! 
  >  I'm not saying you're doing this. But instead of just throwing in what is 
there we should agree on a concept how KDE is handling metadata. This is not 
the place to discuss that, though. I haven't this much thought yet it's  just a 
feeling we might get into trouble in the future.
  
  We pretty much have to cope with whats available out there in the wild as de 
facto standard. We're only consumers here.

INLINE COMMENTS

> michaelh wrote in taglibextractortest.cpp:82
> 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)

Hmm, I disagree since they are simple and they don't leave room for ambiguity

> michaelh wrote in taglibextractor.cpp:132
> Lyrics are not metadata. Unless a song is about itself ;-)

But they are commonly stored as metadata tags in the audio file.

> michaelh wrote in properties.h:302
> Could you give an example for what 'opus' is describing?

https://en.wikipedia.org/wiki/Opus_number

> michaelh wrote in properties.h:307
> Could you give an example for what 'label' is describing?

https://en.wikipedia.org/wiki/Record_label

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.


  What do you think of using https://schema.org/AudioObject Vocabulary?

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#216419 , @astippich 
wrote:
  
  > While I think you have a point here, KFileMetaData sticked to singular 
names before, e.g. author, where also multiple persons are possible. So I think 
we should keep that behavior.
  
  
  We're stuck with that. Nevertheless, it's wrong IMO. All I'm saying is: 
Before simply repeat this error, let's give it some thought.
  
  > To be honest, I don't know which tooltip you mean. Can you post a picture? 
Is this an option I have to switch on?
  
  Configure Dolphin > General > Show Tooltips
  Currently tooltips are a little broken. see D9973 

  
  > I think it behaves quite well and reasonable with many tags as it shows a 
scrollbar. I don't think that there is a better solution.
  
  I haven't tested this. So I don't really know. And you're probably right. I 
forgot that the choice of properties depends on the file type. A screenshot of 
the dialog would be nice (large enough to display everthing).
  
  >> 1. The property names should be choosen to be as generic as possible to 
make them reusable by other file types.
  > 
  > I went through the properties again and only found that we might want to 
use the "generator" property instead of a new "encodedby" property. Do you have 
any other suggestion?
  
  Not yet. To illustrate what I'm thinking of:
  Conductor and Director (of a Movie) are analogous. It would be nice to have a 
term covering both. . Ditto Lyricist and Screenwriter, and maybe more.
  I don't know if that is possible - at least I have no idea yet. We should 
just give it some (more) thought.
  Or the other way around: 
  I find the distinction between 'Release Year' for media and 'Creation Date' 
(which should correctly be 'Publication Date') in for EBooks annoying and 
confusing. We should not repeat that, if possible.
  
  The property names are important for searching with baloo. Everything comes 
together here. (I'll elaborate this on demand)
  The code you are working on exemplifies pretty well, that everybody is 
brewing his own soup when is comes to metadata and tags. And this is only 
audio! 
  I'm not saying you're doing this. But instead of just throwing in what is 
there we should agree on a concept how KDE is handling metadata. This is not 
the place to discuss that, though. I haven't this much thought yet it's  just a 
feeling we might get into trouble in the future.

INLINE COMMENTS

> taglibextractor.cpp:132
> +TagLib::String label;
> +TagLib::String lyrics;
> +TagLib::String author;

Lyrics are not metadata. Unless a song is about itself ;-)

> properties.h:168
>   */
> +//KF6 TODO: fix typo Langauge->Language
>  Langauge,

This may become a plural in KF6, should we decide on it.

> properties.h:302
> + */
> +Opus,
> +

Could you give an example for what 'opus' is describing?

> properties.h:307
> + */
> +Label,
> +

Could you give an example for what 'label' is describing?

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 added a comment.


  In D10803#216118 , @michaelh wrote:
  
  > With this many tags we need to be more considerate because it affects other 
applications which will need to adapt.
  >
  > 1. Plural or singular. E.g. If it was 'performers' the property could 
reused by video files. (In KF6 the same should apply to 'Languages')
  
  
  While I think you have a point here, KFileMetaData sticked to singular names 
before, e.g. author, where also multiple persons are possible. So I think we 
should keep that behavior.
  
  > 1. If there is too much information in them the tooltips in dolphin will 
refuse to display (not enough room). This behaviour depends on the position of 
the hovered file relative to the screen. For the same file tooltip sometimes 
displays sometimes not. That will most likely confuse users.
  
  To be honest, I don't know which tooltip you mean. Can you post a picture? Is 
this an option I have to switch on?
  
  > 1. The property selection dialog for dolphin's infopanel currently is neat 
and small. If the list of items in it becomes too long users will have a hard 
time to find their way through it. We have to check if it needs another layout.
  
  I think it behaves quite well and reasonable with many tags as it shows a 
scrollbar. I don't think that there is a better solution.
  
  > 1. The property names should be choosen to be as generic as possible to 
make them reusable by other file types.
  
  I went through the properties again and only found that we might want to use 
the "generator" property instead of a new "encodedby" property. Do you have any 
other suggestion?

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 reviewer: ngraham.

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.


  With this many tags we need to be more considerate because it affects other 
applications which will need to adapt.
  
  1. Plural or singular. E.g. If it was 'performers' the property could reused 
by video files. (In KF6 the same should apply to 'Languages')
  2. If there is too much information in them the tooltips in dolphin will 
refuse to display (not enough room). This behaviour depends on the position of 
the hovered file relative to the screen. For the same file tooltip sometimes 
displays sometimes not. That will most likely confuse users.
  3. The property selection dialog for dolphin's infopanel currently is neat 
and small. If the list of items in it becomes too long users will have a hard 
time to find their way through it. We have to check if it needs another layout.
  4. The property names should be choosen to be as generic as possible to make 
them reusable by other file types.

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

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


  - handle more tags and remove cover

REPOSITORY
  R286 KFileMetaData

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

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
Cc: vhanda, dfaure, michaelh, ngraham, #frameworks, ashaposhnikov, spoorun, 
nicolasfella, alexeymin

D10803: handle more tags in taglibextractor

[Matthieu Gallien]

mgallien added a subscriber: vhanda.
mgallien added a comment.


  In D10803#215766 , @astippich 
wrote:
  
  > 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.
  >>  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 ?
  > 
  > I think QImage is no different than QByteArray for baloo-widgets. With 
QByteArray, it shows a garbage string. With QImage, it still displays the label 
and an empty string afterwards. So not really a nice solution as can be seen in 
the picture. One could add a special method to ExtactionResult to query the 
picture data, but I think this breaks ABI. I think the issue here is 
baloo-widgets unconditionally displaying everything it gets while it should 
actually check for the type. Judging from a first glance, this should be 
possible, but since baloo widgets is in kde applications and has a different 
release schedule, one could still see the issue when a new kfilemetadata is 
used with an older kde applications release. Is this acceptable?
  > 
  > F5733004: Screenshot_20180227_233349.png 

  
  I am not really sure bout putting images inside the baloo database.
  
  @vhanda do you have any advice about this ? I am really hesitating handling 
all of this into final applications or directly inside baloo and in 
baloo-widgets for the benefits of all clients of baloo.
  
  > Regarding the rating, I'm also not entirely happy about this. I implemented 
it because I thought it would be a nice fallback for Elisa if the file has not 
been rated before (and maybe also for different operating systems). We also had 
this as a feature request somewhere. But there is no real standard regarding 
how the rating is stored and it is probably hard to get it right. So right know 
I'm inclined to remove that tag again. Thoughts?
  > 
  > Btw, lots of more tags incoming
  
  Let's remove all the undecided stuff and land this one quickly for the 
benefit of everybody. Do you agree ?
  
  > In D10803#215305 , @michaelh 
wrote:
  > 
  >> 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.
  > 
  > 
  > That would be a nice feature indeed.
  
  It could be shown by baloo-widgets if the data is stored by baloo. Let's wait 
a bit for possible help and feedback on this.

REPOSITORY
  R286 KFileMetaData

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

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

D10803: handle more tags in taglibextractor

[Alexander Stippich]

astippich added a comment.
Restricted Application added a project: Baloo.


  In D10803#215270 , @mgallien wrote:
  
  > 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 ?
  
  
  I think QImage is no different than QByteArray for baloo-widgets. With 
QByteArray, it shows a garbage string. With QImage, it still displays the label 
and an empty string afterwards. So not really a nice solution as can be seen in 
the picture. One could add a special method to ExtactionResult to query the 
picture data, but I think this breaks ABI. I think the issue here is 
baloo-widgets unconditionally displaying everything it gets while it should 
actually check for the type. Judging from a first glance, this should be 
possible, but since baloo widgets is in kde applications and has a different 
release schedule, one could still see the issue when a new kfilemetadata is 
used with an older kde applications release. Is this acceptable?
  
  F5733004: Screenshot_20180227_233349.png 

  
  Regarding the rating, I'm also not entirely happy about this. I implemented 
it because I thought it would be a nice fallback for Elisa if the file has not 
been rated before (and maybe also for different operating systems). We also had 
this as a feature request somewhere. But there is no real standard regarding 
how the rating is stored and it is probably hard to get it right. So right know 
I'm inclined to remove that tag again. Thoughts?
  
  Btw, lots of more tags incoming
  
  In D10803#215305 , @michaelh wrote:
  
  > 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.
  
  
  That would be a nice feature indeed.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien
Cc: 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