D12156: implement reading of rating tag

[Alexander Stippich]

This revision was automatically updated to reflect the committed changes.
Closed by commit R286:0b0f2d2eb36d: implement reading of rating tag (authored 
by astippich).

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=35242=35938

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D12156: implement reading of rating tag

[Matthieu Gallien]

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


  I hope application developers will not complain about the need to support a 
second api to get ratings. I am still unsure this the best way to add this 
support.

REPOSITORY
  R286 KFileMetaData

BRANCH
  rating

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

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

D12156: implement reading of rating tag

[Alexander Stippich]

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


  - update comments

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=34398=35242

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien added a subscriber: cgilles.
mgallien added a comment.


  @cgilles Are you somehow using KFileMetaData (seems correct if one looks at 
the Debian package dependencies) ?
  This diff is about adding a way to read ratings from "tags" native to a 
specific file format.
  
  This would be in addition to the current ratings that are independent from 
the file types.
  
  Can you comment on this patch with a digikam point of view ?
  
  I am wondering if we should not find a way to make this more transparent for 
users of the API. This is why I am asking here.

REPOSITORY
  R286 KFileMetaData

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

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

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  any comments on the previous discussion?

REPOSITORY
  R286 KFileMetaData

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

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

D12156: implement reading of rating tag

[Alexander Stippich]

astippich updated this revision to Diff 34398.
astippich added a comment.
Restricted Application edited subscribers, added: Baloo, kde-frameworks-devel; 
removed: Frameworks.


  - rebase on master
  - implement rating tag for m4a

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=32591=34398

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.m4a
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

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

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  I agree, there are several problems coming from the fact that we basically 
have two sources for the same property, which can potentially conflict. Btw, we 
already have the same issue with the comment property.
  
  In D12156#257888 , @bruns wrote:
  
  > I see three problems here:
  >
  > - The current "Rating" is handled specially, you can e.g. select Rating: 4 
or more in Dolphins "Find ..." dialog. If I have a file with an embeddedrating 
of 4.5 (normalized), I would expect it to show up. Probably the right thing 
would be to teach Baloo to treat embeddedrating the same way as a rating stored 
in an xattr, although I am not sure.
  > - Inconsistent data - the xattr rating may differ from the embedded rating. 
The one from the xattr should likely be preferred, but what is exposed in the 
file information widget?
  
  
  I want to leave that decision up to the application, and this patch is 
actually the first step towards that.
  
  > - Writing/updating the rating - we should remove any xattr rating, to avoid 
any ambiguities which one is the current one.
  
  I think one should write to both properties, so that existing applications 
relying on xattr continue to work. Also keep in mind that (at least with this 
patch) there are only rating tags for audio files, I don't know if such things 
exist for other documents.
  
  > The XAttr rating is retrieved with the basicindexingjob, and there is 
currently no possibility to "merge" data from the extractors in a different way 
than just creating the union. So in the database we have either
  > 
  > - Two, probably different, "rating" tags. Searching would match if any 
matches, and the widget would show an arbitrary one
  > - Two independent tags. Every search for a rating becomes `if ("rating" == 
r OR "embeddedrating" == r)`
  
  Hmm, haven't really thought about searching in baloo. But imho we should keep 
both ratings, since we don't know which is the "right" or more important one.
  
  In D12156#258436 , @mgallien wrote:
  
  > I would like to have only one rating in the KFileMetaData API that would 
transparently use the most appropriate way to store and read the rating.
  
  
  xattr tags have a completely different code path than the extractors. One 
could wire that up, but this would need to adapt all extractors and to teach 
the applications using KFileMetaData not to query the xattr separately anymore.
  
  > That would be:
  > 
  > - music audio file with a rating tag = we read the tag and write both tag 
and xattr attribute to maximize compatibility ;
  > - music audio file with only xattr rating attribute = we read the xattr 
attribute and write both tag and xattr attribute to maximize compatibility ;
  > - any file with only xattr rating attribute = we read the xattr attribute 
and write the xattr attribute ;
  > 
  >   Does this sound reasonable and feasible ?
  
  That was the plan anyway, but to let this be handled in the applications. I 
think this is especially important if we have conflicting ratings, and the user 
should be able to decide which one to use. I don't think that a library such as 
KFileMetaData should do that decision, it should present all available 
information.
  
  Let me know what you think. I prefer to let KFileMetaData just present all 
information and let the applications handle the difficulties. If you guys think 
that this is not the way to go, I can also try to implement this inside 
KFileMetaData.

REPOSITORY
  R286 KFileMetaData

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

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

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien added a comment.


  In D12156#254055 , @astippich 
wrote:
  
  > In D12156#252575 , @mgallien 
wrote:
  >
  > > If I have correctly understood the ideas behind the conception of Baloo, 
we should probably prefer to store the rating with a "native" solution instead 
of the xattr one that is not portable outside of software using KFileMetaData.
  > >
  > > We have to stay compatible with older versions of KFileMetaData. So we 
should push the same rating to both properties and try to read it from both 
properties in the cases where only one exists.
  > >
  > > Could you try to modify your patch to do that ?
  >
  >
  > I don't understand. Which patch to you want me to modify? This one here 
only adds the ability to read the rating embedded in the tags in addition to 
the xattr rating. It's up to the application to decide what to do with the 
information. 
  >  The patch for baloo-widgets just hides this new rating tag to avoid 
duplicate entries in e.g. dolphin, and actually preserves the current behavior 
this way. Baloo caches the embedded rating anyways. 
  >  When KFileMetaData gains the ability to write the tags, one should write 
to both properties, but that is currently not possible.
  
  
  I would like to have only one rating in the KFileMetaData API that would 
transparently use the most appropriate way to store and read the rating.
  That would be:
  
  - music audio file with a rating tag = we read the tag and write both tag and 
xattr attribute to maximize compatibility ;
  - music audio file with only xattr rating attribute = we read the xattr 
attribute and write both tag and xattr attribute to maximize compatibility ;
  - any file with only xattr rating attribute = we read the xattr attribute and 
write the xattr attribute ;
  
  Does this sound reasonable and feasible ?

REPOSITORY
  R286 KFileMetaData

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

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

D12156: implement reading of rating tag

[Stefan Brüns]

bruns added a comment.


  In D12156#254055 , @astippich 
wrote:
  
  > In D12156#252575 , @mgallien 
wrote:
  >
  > > If I have correctly understood the ideas behind the conception of Baloo, 
we should probably prefer to store the rating with a "native" solution instead 
of the xattr one that is not portable outside of software using KFileMetaData.
  > >
  > > We have to stay compatible with older versions of KFileMetaData. So we 
should push the same rating to both properties and try to read it from both 
properties in the cases where only one exists.
  > >
  > > Could you try to modify your patch to do that ?
  >
  >
  > I don't understand. Which patch to you want me to modify? This one here 
only adds the ability to read the rating embedded in the tags in addition to 
the xattr rating. It's up to the application to decide what to do with the 
information. 
  >  The patch for baloo-widgets just hides this new rating tag to avoid 
duplicate entries in e.g. dolphin, and actually preserves the current behavior 
this way. Baloo caches the embedded rating anyways. 
  >  When KFileMetaData gains the ability to write the tags, one should write 
to both properties, but that is currently not possible.
  
  
  I see three problems here:
  
  - The current "Rating" is handled specially, you can e.g. select Rating: 4 or 
more in Dolphins "Find ..." dialog. If I have a file with an embeddedrating of 
4.5 (normalized), I would expect it to show up. Probably the right thing would 
be to teach Baloo to treat embeddedrating the same way as a rating stored in an 
xattr, although I am not sure.
  - Inconsistent data - the xattr rating may differ from the embedded rating. 
The one from the xattr should likely be preferred, but what is exposed in the 
file information widget?
  - Writing/updating the rating - we should remove any xattr rating, to avoid 
any ambiguities which one is the current one.
  
  The XAttr rating is retrieved with the basicindexingjob, and there is 
currently no possibility to "merge" data from the extractors in a different way 
than just creating the union. So in the database we have either
  
  - Two, probably different, "rating" tags. Searching would match if any 
matches, and the widget would show an arbitrary one
  - Two independent tags. Every search for a rating becomes `if ("rating" == r 
OR "embeddedrating" == r)`

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  ping @mgallien

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#252575 , @mgallien wrote:
  
  > If I have correctly understood the ideas behind the conception of Baloo, we 
should probably prefer to store the rating with a "native" solution instead of 
the xattr one that is not portable outside of software using KFileMetaData.
  >
  > We have to stay compatible with older versions of KFileMetaData. So we 
should push the same rating to both properties and try to read it from both 
properties in the cases where only one exists.
  >
  > Could you try to modify your patch to do that ?
  
  
  I don't understand. Which patch to you want me to modify? This one here only 
adds the ability to read the rating embedded in the tags in addition to the 
xattr rating. It's up to the application to decide what to do with the 
information. 
  The patch for baloo-widgets just hides this new rating tag to avoid duplicate 
entries in e.g. dolphin, and actually preserves the current behavior this way. 
Baloo caches the embedded rating anyways. 
  When KFileMetaData gains the ability to write the tags, one should write to 
both properties, but that is currently not possible.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Stefan Brüns]

bruns added a comment.


  In D12156#252575 , @mgallien wrote:
  
  > In D12156#249994 , @astippich 
wrote:
  >
  > > In D12156#249977 , @michaelh 
wrote:
  > >
  > > > In D12156#249971 , 
@astippich wrote:
  > > >
  > > > > In D12156#249951 , 
@michaelh wrote:
  > > > >
  > > > > > It's for elisa I guess, could you please elaborate how POPM/RATING 
is going to be used and why xattr are not applicable?
  > > > >
  > > > >
  > > > > It will be used as a fallback when there is no xattr rating set or 
available (e.g. on Windows) so that users who have rated their music with other 
players can still see their previous ratings. 
  > > > >  It will also useful for managing ratings on other platforms when 
writing support is added.
  > > >
  > > >
  > > > I that case I would suggest to use xattr only on systems that support 
it, and use POPM/RATING on those that don't. I afraid users will find it 
confusing to have two ways of rating a file. Or is rating via 
dolphin/baloo-widgets not planned?
  > > >  Because extractors are called in sequence it would also be possible to 
create a dedicate taglibRatingExtractor (much easier to apply build 
conditionals).
  > >
  > >
  > > We still need the ability to read the tag, since users expect to see the 
rating in music players.
  > >  For baloo-widgets I created D12157 
  >
  >
  > If I have correctly understood the ideas behind the conception of Baloo, we 
should probably prefer to store the rating with a "native" solution instead of 
the xattr one that is not portable outside of software using KFileMetaData.
  >
  > We have to stay compatible with older versions of KFileMetaData. So we 
should push the same rating to both properties and try to read it from both 
properties in the cases where only one exists.
  >
  > Could you try to modify your patch to do that ?
  
  
  Your interpretation is correct, Baloo works as a cache, storing data in a 
normalized form, to speed up lookup. It is not a permanent data store.
  
  Storing/updating metadata inside the file has the benefit it works 
independent of the filesystem capabities. On the other hand, one risks damaging 
the file, and synchronization between devices may become more difficult.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Matthieu Gallien]

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


  In D12156#249994 , @astippich 
wrote:
  
  > In D12156#249977 , @michaelh 
wrote:
  >
  > > In D12156#249971 , @astippich 
wrote:
  > >
  > > > In D12156#249951 , @michaelh 
wrote:
  > > >
  > > > > It's for elisa I guess, could you please elaborate how POPM/RATING is 
going to be used and why xattr are not applicable?
  > > >
  > > >
  > > > It will be used as a fallback when there is no xattr rating set or 
available (e.g. on Windows) so that users who have rated their music with other 
players can still see their previous ratings. 
  > > >  It will also useful for managing ratings on other platforms when 
writing support is added.
  > >
  > >
  > > I that case I would suggest to use xattr only on systems that support it, 
and use POPM/RATING on those that don't. I afraid users will find it confusing 
to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not 
planned?
  > >  Because extractors are called in sequence it would also be possible to 
create a dedicate taglibRatingExtractor (much easier to apply build 
conditionals).
  >
  >
  > We still need the ability to read the tag, since users expect to see the 
rating in music players.
  >  For baloo-widgets I created D12157 
  
  
  If I have correctly understood the ideas behind the conception of Baloo, we 
should probably prefer to store the rating with a "native" solution instead of 
the xattr one that is not portable outside of software using KFileMetaData.
  
  We have to stay compatible with older versions of KFileMetaData. So we should 
push the same rating to both properties and try to read it from both properties 
in the cases where only one exists.
  
  Could you try to modify your patch to do that ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#249977 , @michaelh wrote:
  
  > In D12156#249971 , @astippich 
wrote:
  >
  > > In D12156#249951 , @michaelh 
wrote:
  > >
  > > > It's for elisa I guess, could you please elaborate how POPM/RATING is 
going to be used and why xattr are not applicable?
  > >
  > >
  > > It will be used as a fallback when there is no xattr rating set or 
available (e.g. on Windows) so that users who have rated their music with other 
players can still see their previous ratings. 
  > >  It will also useful for managing ratings on other platforms when writing 
support is added.
  >
  >
  > I that case I would suggest to use xattr only on systems that support it, 
and use POPM/RATING on those that don't. I afraid users will find it confusing 
to have two ways of rating a file. Or is rating via dolphin/baloo-widgets not 
planned?
  >  Because extractors are called in sequence it would also be possible to 
create a dedicate taglibRatingExtractor (much easier to apply build 
conditionals).
  
  
  We still need the ability to read the tag, since users expect to see the 
rating in music players.
  For baloo-widgets I created D12157 

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Michael Heidelbach]

michaelh added a comment.


  In D12156#249971 , @astippich 
wrote:
  
  > In D12156#249951 , @michaelh 
wrote:
  >
  > > It's for elisa I guess, could you please elaborate how POPM/RATING is 
going to be used and why xattr are not applicable?
  >
  >
  > It will be used as a fallback when there is no xattr rating set or 
available (e.g. on Windows) so that users who have rated their music with other 
players can still see their previous ratings. 
  >  It will also useful for managing ratings on other platforms when writing 
support is added.
  
  
  I that case I would suggest to use xattr only on systems that support it, and 
use POPM/RATING on those that don't. I afraid users will find it confusing to 
have two ways of rating a file. Or is rating via dolphin/baloo-widgets not 
planned?
  Because extractors are called in sequence it would also be possible to create 
a dedicate taglibRatingExtractor.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#249951 , @michaelh wrote:
  
  > It's for elisa I guess, could you please elaborate how POPM/RATING is going 
to be used and why xattr are not applicable?
  
  
  It will be used as a fallback when there is no xattr rating set or available 
(e.g. on Windows) so that users who have rated their music with other players 
can still see their previous ratings. 
  It will also useful for managing ratings on other platforms when writing 
support is added.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Michael Heidelbach]

michaelh added a comment.


  It's for elisa I guess, could you please elaborate how POPM/RATING is going 
to be used and why xattr are not applicable?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  Those formulas didn't work with newly added tests with values from Winamp, 
Windows Media Player and MediaMonkey. I calculated a linear regression line and 
this one seems to work with some special case handling

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

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


  - simplify

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=32587=32591

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  code didn't get prettier, but that's the de-facto standard

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> astippich wrote in taglibextractor.cpp:222-234
> The thing here is that the most common implementation don't allow half star 
> ratings. Only Mediamonkey does it and that diverges from the rest. Also, the 
> values are not distributed equidistantly, but I'll look into a more flexible 
> solution.

There is a "half interval" at the bottom and the top, every other interval is 
64.

  rating = temp * 10 / 255 + 0.5;

should work reasonably well, or, if you prefer a smaller `0` interval:

  rating = temp * 9.5 / 255 + 0.99;

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: bruns, #frameworks, ashaposhnikov, michaelh, astippich, spoorun

D12156: implement reading of rating tag

[Alexander Stippich]

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


  - implement reading of half star ratings for mp3 and add tests

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12156?vs=31987=32587

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/mp3_rating/testMM.mp3
  autotests/samplefiles/mp3_rating/testMM1.mp3
  autotests/samplefiles/mp3_rating/testMM10.mp3
  autotests/samplefiles/mp3_rating/testMM2.mp3
  autotests/samplefiles/mp3_rating/testMM3.mp3
  autotests/samplefiles/mp3_rating/testMM4.mp3
  autotests/samplefiles/mp3_rating/testMM5.mp3
  autotests/samplefiles/mp3_rating/testMM6.mp3
  autotests/samplefiles/mp3_rating/testMM7.mp3
  autotests/samplefiles/mp3_rating/testMM8.mp3
  autotests/samplefiles/mp3_rating/testMM9.mp3
  autotests/samplefiles/mp3_rating/testWMP.mp3
  autotests/samplefiles/mp3_rating/testWMP1.mp3
  autotests/samplefiles/mp3_rating/testWMP2.mp3
  autotests/samplefiles/mp3_rating/testWMP3.mp3
  autotests/samplefiles/mp3_rating/testWMP4.mp3
  autotests/samplefiles/mp3_rating/testWMP5.mp3
  autotests/samplefiles/test.flac
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.ogg
  autotests/samplefiles/test.opus
  autotests/taglibextractortest.cpp
  autotests/taglibextractortest.h
  src/extractors/taglibextractor.cpp
  src/extractors/taglibextractor.h
  src/properties.h
  src/propertyinfo.cpp

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12156: implement reading of rating tag

[Alexander Stippich]

astippich added a comment.


  In D12156#249223 , @mgallien wrote:
  
  > Thanks.
  >  Fix one issue and we should be good to go.
  >  Another nice thing is that we should have a portable way to have ratings 
in Elisa that could be read or write somewhere else.
  
  
  Writing the tags must be implemented first for that :)

INLINE COMMENTS

> mgallien wrote in taglibextractor.cpp:222-234
> You can and should be using all intermediate values. If I remember correctly, 
> in dolphin, you can set all values between 0 and 10. This is done by half 
> blue stars.
> Can you use modulo instead of embedded if ?

The thing here is that the most common implementation don't allow half star 
ratings. Only Mediamonkey does it and that diverges from the rest. Also, the 
values are not distributed equidistantly, but I'll look into a more flexible 
solution.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12156: implement reading of rating tag

[Matthieu Gallien]

mgallien added a comment.


  Thanks.
  Fix one issue and we should be good to go.
  Another nice thing is that we should have a portable way to have ratings in 
Elisa that could be read or write somewhere else.

INLINE COMMENTS

> taglibextractor.cpp:222-234
> +if (temp == 0) {
> +data.rating = 0;
> +} else if (temp >= 1 && temp <= 31) {
> +data.rating = 2;
> +} else if (temp >= 32 && temp <= 95) {
> +data.rating = 4;
> +} else if (temp >= 96 && temp <= 159) {

You can and should be using all intermediate values. If I remember correctly, 
in dolphin, you can set all values between 0 and 10. This is done by half blue 
stars.
Can you use modulo instead of embedded if ?

REPOSITORY
  R286 KFileMetaData

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

To: astippich, mgallien, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, bruns

D12156: implement reading of rating tag

[Alexander Stippich]

astippich edited the summary of this revision.

REPOSITORY
  R286 KFileMetaData

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

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

D12156: implement reading of rating tag

[Alexander Stippich]

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

REVISION SUMMARY
  allows to read the rating stored in the metadata for idv2, vorbis and ape tags

REPOSITORY
  R286 KFileMetaData

BRANCH
  rating

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

AFFECTED FILES
  autotests/samplefiles/test.flac
  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, michaelh
Cc: #frameworks, ashaposhnikov, michaelh, astippich, spoorun, ngraham, bruns, 
alexeymin