D16579: Fix APE tag extraction

[James Smith]

smithjd added a comment.


  I realise that with all the fields referenced here having at least one 
standard fieldname parsed now that complaining may seem to be flogging a dead 
horse, but I'd like to re-iterate that identically-purposed fields should not 
be encouraged. This is at best a specialized re-purposing that really shouldn't 
be. Maintaining a branch / fork for this is probably appropriate.
  
  Lastly, I'd also like to point out the TagLib documentation, that 
specifically mentions a conversion from 'Album Artist' in 'AlbumArtist' for 
compatibility with other tags: 
https://taglib.org/api/classTagLib_1_1APE_1_1Tag.html#af77a10659fbb0018228420ad6de501e1.
 'Disc' is also converted from APE, but you have to dig into the code to find 
this: https://github.com/taglib/taglib/blob/master/taglib/ape/apetag.cpp#L211

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Fix APE tag extraction

[James Smith]

smithjd updated this revision to Diff 52185.
smithjd added a comment.


  - Use the de-facto Album Artist field name for APE tags.
  
  Re-base.

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16579?vs=44708=52185

BRANCH
  master-musepackFixes (branched from master)

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

AFFECTED FILES
  autotests/samplefiles/test.ape
  autotests/samplefiles/test.mpc
  autotests/samplefiles/test.wv
  src/extractors/taglibextractor.cpp

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

D16579: Fix APE tag extraction

[James Smith]

smithjd added a comment.


  In D16579#352887 , @astippich 
wrote:
  
  > You're doing the exact opposite of what we're asking for.
  >  Look, I'd love to merge the bug fix for the DISC property. But we need 
compatibility.
  >  I'll give you another reason: KFileMetaData has basically required that 
users use the DISCNUMBER field until now. And now you want to change that 
without providing any suitable transition. That's not user-friendly.
  
  
  Despite the large amount of tagger information attached to this review, it's 
still impossible to find a tagger that defaults to an alternative to the DISC 
field name for APE, Some taggers implement the ability to add arbitrary fields 
to APE tags, and nothing is stopping anyone from populating a DISCNUMBER field, 
though ignoring the more-established DISC field name is counterintuitive and 
this should be regarded as a specialized use-case of the APE tag. Similarly the 
non-standard AlbumArtist alternative to the Album Artist APE field name has 
been accepted (albeit as an alternative to an established field name) since 
about the time the disk number property was introduced. Because one of the 
primary motivations behind multimedia tagging is seemless portability, 
specialized application of APE tags should not be encouraged over existing 
standard field names.
  
  Forcing compliance with established tag field names is lower maintenance, and 
not the end of the world for KDE APE tag users. Forcing these users to adopt 
established field names to make full use of the KDE metadata infrastructure is 
a responsible thing to do anyway, and users already understand they must put 
effort into their tags to reap the full benefit anyway. Ending the parsing of 
field names that have widely-implemented counterparts should be effected with 
no regard for compatibility with *any* specialised application of APE tags, and 
with *no* transition period. KDE is very specific about the types of metadata 
it can make use of and requiring users to make the extra effort to get better 
use of its capabilities is not unreasonable. It is also not unreasonable for 
the user to expect that this effort would be maximally re-useable elsewhere by 
default.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Fix APE tag extraction

[Alexander Stippich]

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


  You're doing the exact opposite of what we're asking for.
  Look, I'd love to merge the bug fix for the DISC property. But we need 
compatibility.
  I'll give you another reason: KFileMetaData has basically required that users 
use the DISCNUMBER field until now. And now you want to change that without 
providing any suitable transition. That's not user-friendly.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Fix APE tag extraction

[Stefan Brüns]

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


  Stop breaking code!

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Fix APE tag extraction

[James Smith]

smithjd added a comment.


  In D16579#352788 , @smithjd wrote:
  
  > - Use the de-facto Album Artist field name for APE tags.
  >
  >   Since the discussion around this patch has also included the Album Artist 
field, add the changes required for this field to this review.
  
  
  Also adding that EasyTag uses the 'Album Artist' field name. Not sure about 
picard, but the above-linked documentation seems to suggest this.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Fix APE tag extraction

[James Smith]

smithjd retitled this revision from "Musepack disk number field name is DISC." 
to "Fix APE tag extraction".
smithjd edited the summary of this revision.
smithjd edited the test plan for this revision.
smithjd added a reviewer: mgallien.

REPOSITORY
  R286 KFileMetaData

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

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