D16579: Musepack disk number field name is DISC.

[James Smith]

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


  - 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.

REPOSITORY
  R286 KFileMetaData

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

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
  autotests/taglibextractortest.cpp
  src/extractors/taglibextractor.cpp

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

D16579: Musepack disk number field name is DISC.

[Stefan Brüns]

bruns added a comment.


  In D16579#352406 , @smithjd wrote:
  
  > In D16579#352282 , @bruns wrote:
  >
  > > In D16579#352279 , @smithjd 
wrote:
  > >
  > > > I would instead recommend a tag editor that properly tags APE files, 
such as puddletag. APE-using formats are less mainstream than id3 using 
formats. Users with APE-using formats usually know WHY they use their format of 
choice, and will know there are potential support shortfalls such as 
non-compliant tagging software, or software that allows to circumvent the 
accepted tagging standard, and will know how to mitigate or avoid such issues. 
I must also point out that stricter tag field adherence is better for APE-using 
formats in particular, and is better for all tag-using formats in general.
  > >
  > >
  > > Good luck "educating" every APE user, they will really like to be told 
they should do what you consider right.
  >
  >
  > ...and we should probably rip out *EVERY* other format except for mp3 with 
its larger installed base and because it's standard fields are more supported 
by other software.
  >
  > This is a *fix* for APE tag support, not an argument over why there are 
*de-facto* standard tag fields or why certain users can screw up with powerful 
software, or why standardization is a problem and goal, especially in a tag 
format that permits arbitrary field names. There are 2 sources attached to this 
review and anecdotal evidence as to what a number of tag programs can or do 
write into APE tags. This is enough for an educated guess as to a broad 
consensus on standardized APE field names.
  >
  > I'm also not in favour of compatibility with alternatives to the de-facto 
APE field names. We should not be encouraging alternatives to well-established, 
documented field names. It's also potential burden on the writers of other 
software and also on the users of APE tags, and detrimental to the APE-using 
format's ecosystem.
  
  
  Ever heard of:
  
  > Be conservative in what you do, be liberal in what you accept from others
  
  You deliberately break other users files. The only consensus you have is with 
yourself.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd added a comment.


  In D16579#352282 , @bruns wrote:
  
  > In D16579#352279 , @smithjd 
wrote:
  >
  > > I would instead recommend a tag editor that properly tags APE files, such 
as puddletag. APE-using formats are less mainstream than id3 using formats. 
Users with APE-using formats usually know WHY they use their format of choice, 
and will know there are potential support shortfalls such as non-compliant 
tagging software, or software that allows to circumvent the accepted tagging 
standard, and will know how to mitigate or avoid such issues. I must also point 
out that stricter tag field adherence is better for APE-using formats in 
particular, and is better for all tag-using formats in general.
  >
  >
  > Good luck "educating" every APE user, they will really like to be told they 
should do what you consider right.
  
  
  ...and we should probably rip out *EVERY* other format except for mp3 with 
its larger installed base and because it's standard fields are more supported 
by other software.
  
  This is a *fix* for APE tag support, not an argument over why there are 
*de-facto* standard tag fields or why certain users can screw up with powerful 
software, or why standardization is a problem and goal, especially in a tag 
format that permits arbitrary field names. There are 2 sources attached to this 
review and anecdotal evidence as to what a number of tag programs can or do 
write into APE tags. This is enough for an educated guess as to a broad 
consensus on standardized APE field names.
  
  I'm also not in favour of compatibility with alternatives to the de-facto APE 
field names. We should not be encouraging alternatives to well-established, 
documented field names. It's also potential burden on the writers of other 
software and also on the users of APE tags, and detrimental to the APE-using 
format's ecosystem.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[Stefan Brüns]

bruns added a comment.


  In D16579#352279 , @smithjd wrote:
  
  > I would instead recommend a tag editor that properly tags APE files, such 
as puddletag. APE-using formats are less mainstream than id3 using formats. 
Users with APE-using formats usually know WHY they use their format of choice, 
and will know there are potential support shortfalls such as non-compliant 
tagging software, or software that allows to circumvent the accepted tagging 
standard, and will know how to mitigate or avoid such issues. I must also point 
out that stricter tag field adherence is better for APE-using formats in 
particular, and is better for all tag-using formats in general.
  
  
  Good luck "educating" every APE user, they will really like to be told they 
should do what you consider right.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd added a comment.


  I would instead recommend a tag editor that properly tags APE files, such as 
puddletag. APE-using formats are less mainstream than id3 using formats. Users 
with APE-using formats usually know WHY they use their format of choice, and 
will know there are potential support shortfalls such as non-compliant tagging 
software, or software that allows to circumvent the accepted tagging standard, 
and will know how to mitigate or avoid such issues. I must also point out that 
stricter tag field adherence is better for APE-using formats in particular, and 
is better for all tag-using formats in general.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[Alexander Stippich]

astippich added a comment.


  In D16579#352255 , @smithjd wrote:
  
  > I would also argue that accepting values from tag field names that have 
identically-purposed, widely-acceptable alternatives is irresponsible. Changing 
your tags to meet the standard then is a more viable course of action.
  
  
  In an ideal world, yes of course. But it is just not going to happen that 
every user changes its tags according to the "standard" because KFileMetaData 
decided to stick to it. Instead, we get complaints or bug reports about it.
  Think about it from an end-user perspective, who doesn't know anything about 
the specific tags, and just wants to edit the tags of a music file. One opens 
kid3, clicks the 'Add' button, starts typing 'disc...' and kid3 then proposes 
'Disc Number'. The user uses it and wonders why it doesn't show up in Dolphin 
afterwards and complains.
  
  You can also argue that it's actually not a standard, it's an agreement. Both 
ape and vorbis tags allow to write arbitrary tags to the file. From that 
perspective, both are actually valid entries. Since it really is a matter of 3 
lines of code, please just add it.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd added a comment.


  I would also argue that accepting values from tag field names that have 
identically-purposed, widely-acceptable alternatives is irresponsible. Changing 
your tags to meet the standard then is a more viable course of action.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[Stefan Brüns]

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


  In D16579#352218 , @smithjd wrote:
  
  > In D16579#352191 , @astippich 
wrote:
  >
  > > Yes, maybe it's not widely used, but it is used. Kodi for example 
supports both reading from DISCNUMBER and DISC. If you use kid3 to edit the 
metadata of ape tags, the standard behavior is to actually to write to 
DISCNUMBER (and similar to ALBUMARTIST).
  > >  One thing I've learned when I digged into metadata of audio files is 
that there is no standard, and KFileMetaData should handle as much cases as 
possible. Since it is easy to query both, please add it.
  >
  >
  > kid3 allows arbitrary field names and the APE tag field names for DISC and 
ALBUM ARTIST aren't there by default. The corresponding tags for id3 are there 
and can be applied to APE tags, but they aren't widely used as valid APE fields.
  
  
  Even if not used widely, they are still used. There is no drawback supporting 
both, save a few lines of code.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd added a comment.


  In D16579#352191 , @astippich 
wrote:
  
  > In D16579#352147 , @smithjd 
wrote:
  >
  > > In D16579#351910 , @astippich 
wrote:
  > >
  > > > The ape tag tests fail with this patch, but the test is actually wrong 
in that regard. It tests for an empty disc number, which I haven't noticed 
before.
  > > >  I've found references to both DISCNUMBER and DISC, so the safest way 
is probably to check both.
  > > >  So please query both tags like it is already done for the album artist 
and adjust the taglibextractortest.
  > >
  > >
  > > DISCNUMBER doesn't seem to be a valid (or widely used) field for APEv2.
  > >
  > > More (Picard) information: https://picard.musicbrainz.org/docs/mappings
  > >
  > > Also it appears Picard, Puddletag and ffmpeg use 'Album Artist', not 
'ALBUMARTIST'. And the link I provided 
(https://wiki.hydrogenaud.io/index.php?title=Tag_Mapping) also lists 'Album 
Artist' not 'ALBUMARTIST'. The unit test file has an 'ALBUMARTIST' field that 
should be changed to 'Album Artist'.
  >
  >
  > Yes, maybe it's not widely used, but it is used. Kodi for example supports 
both reading from DISCNUMBER and DISC. If you use kid3 to edit the metadata of 
ape tags, the standard behavior is to actually to write to DISCNUMBER (and 
similar to ALBUMARTIST).
  >  One thing I've learned when I digged into metadata of audio files is that 
there is no standard, and KFileMetaData should handle as much cases as 
possible. Since it is easy to query both, please add it.
  
  
  kid3 allows arbitrary field names and the APE tag field names for DISC and 
ALBUM ARTIST aren't there by default. The corresponding tags for id3 are there 
and can be applied to APE tags, but they aren't widely used as valid APE fields.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[Alexander Stippich]

astippich added a comment.


  In D16579#352147 , @smithjd wrote:
  
  > In D16579#351910 , @astippich 
wrote:
  >
  > > The ape tag tests fail with this patch, but the test is actually wrong in 
that regard. It tests for an empty disc number, which I haven't noticed before.
  > >  I've found references to both DISCNUMBER and DISC, so the safest way is 
probably to check both.
  > >  So please query both tags like it is already done for the album artist 
and adjust the taglibextractortest.
  >
  >
  > DISCNUMBER doesn't seem to be a valid (or widely used) field for APEv2.
  >
  > More (Picard) information: https://picard.musicbrainz.org/docs/mappings
  >
  > Also it appears Picard, Puddletag and ffmpeg use 'Album Artist', not 
'ALBUMARTIST'. And the link I provided 
(https://wiki.hydrogenaud.io/index.php?title=Tag_Mapping) also lists 'Album 
Artist' not 'ALBUMARTIST'. The unit test file has an 'ALBUMARTIST' field that 
should be changed to 'Album Artist'.
  
  
  Yes, maybe it's not widely used, but it is used. Kodi for example supports 
both reading from DISCNUMBER and DISC. If you use kid3 to edit the metadata of 
ape tags, the standard behavior is to actually to write to DISCNUMBER (and 
similar to ALBUMARTIST).
  One thing I've learned when I digged into metadata of audio files is that 
there is no standard, and KFileMetaData should handle as much cases as 
possible. Since it is easy to query both, please add it.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

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


  - Change the unit test to check if the APE 'disc' value is correct.

REPOSITORY
  R286 KFileMetaData

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D16579?vs=44607=44638

BRANCH
  master-musepackFixes (branched from master)

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

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd added a comment.


  In D16579#351910 , @astippich 
wrote:
  
  > The ape tag tests fail with this patch, but the test is actually wrong in 
that regard. It tests for an empty disc number, which I haven't noticed before.
  >  I've found references to both DISCNUMBER and DISC, so the safest way is 
probably to check both.
  >  So please query both tags like it is already done for the album artist and 
adjust the taglibextractortest.
  
  
  DISCNUMBER doesn't seem to be a valid (or widely used) field for APEv2.
  
  More (Picard) information: https://picard.musicbrainz.org/docs/mappings
  
  Also it appears Picard, Puddletag and ffmpeg use 'Album Artist', not 
'ALBUMARTIST'. And the link I provided 
(https://wiki.hydrogenaud.io/index.php?title=Tag_Mapping) also lists 'Album 
Artist' not 'ALBUMARTIST'. The unit test file has an 'ALBUMARTIST' field that 
should be changed to 'Album Artist'.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[Alexander Stippich]

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


  The ape tag tests fail with this patch, but the test is actually wrong in 
that regard. It tests for an empty disc number, which I haven't noticed before.
  I've found references to both DISCNUMBER and DISC, so the safest way is 
probably to check both.
  So please query both tags like it is already done for the album artist and 
adjust the taglibextractortest.

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd added a comment.


  Useful tag mapping information:
  
  https://wiki.hydrogenaud.io/index.php?title=Tag_Mapping

REPOSITORY
  R286 KFileMetaData

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

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

D16579: Musepack disk number field name is DISC.

[James Smith]

smithjd created this revision.
Herald added projects: Frameworks, Baloo.
Herald added subscribers: Baloo, kde-frameworks-devel.
smithjd requested review of this revision.

REVISION SUMMARY
  Fix the Musepack disk field.

REPOSITORY
  R286 KFileMetaData

BRANCH
  master-musepackFixes (branched from master)

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

AFFECTED FILES
  src/extractors/taglibextractor.cpp

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