Useful tag mapping information:

https://wiki.hydrogenaud.io/index.php?title=Tag_Mapping

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

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.

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.

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

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.

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.

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.

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.

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.

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.

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

In D16579#416446, @smithjd wrote:

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

I just double-checked, TagLib allows both. It converts the "Disc" field to the "Disc Number" in the property map, but also successfully reads from "Disc Number" field directly.
Anyway, most of this special handling of mimetypes goes away with D18826, where TagLib will do the conversion automatically.

Anyway, most of this special handling of mimetypes goes away with D18826, where TagLib will do the conversion automatically.

The test files should reflect the most common tag fieldnames; 'albumartist' is used in the APEv2 test files instead of 'album artist'.

In D16579#418256, @smithjd wrote:

Anyway, most of this special handling of mimetypes goes away with D18826, where TagLib will do the conversion automatically.

The test files should reflect the most common tag fieldnames; 'albumartist' is used in the APEv2 test files instead of 'album artist'.

I have no problem with only adjusting the test files, but please make sure that the new files do not break the embeddimagedatatest.

Two reviewers had open "Changes Requested" statuses on this patch when you committed it, and from the latest comments, it's not clear that their concerns were addressed first. Can you explain your thought process regarding why you felt it was appropriate to ignore your reviewers as well as KDE community norms and commit this anyway?

In D16579#428790, @ngraham wrote:

Two reviewers had open "Changes Requested" statuses on this patch when you committed it, and from the latest comments, it's not clear that their concerns were addressed first. Can you explain your thought process regarding why you felt it was appropriate to ignore your reviewers as well as KDE community norms and commit this anyway?

The scope has shrunk to just the test files since D18826 made the original code changes and the requested changes obsolete, and the test file changes were ack'ed.

Your comment is unfounded.

In D16579#428807, @smithjd wrote:

In D16579#428790, @ngraham wrote:

Two reviewers had open "Changes Requested" statuses on this patch when you committed it, and from the latest comments, it's not clear that their concerns were addressed first. Can you explain your thought process regarding why you felt it was appropriate to ignore your reviewers as well as KDE community norms and commit this anyway?

The scope has shrunk to just the test files since D18826 made the original code changes and the requested changes obsolete, and the test file changes were ack'ed.

If that's the case, you should have asked the reviewers who had requested changes whether the current state of the patch was merge-able so they they could turn their Changes Requested statuses into Accepted statuses. We have a review process for a reason, and you are expected to follow the rules.

The change was ack'ed in this review, and this review closed by the commit.

There was no code that required further review, only binary changes. The posted concerns were addressed by D18826.

If you really are that concerned that this review is cleaned up, request yourself that the reviewers that requested changes clear their request. I don't need to ask them to when the request is no longer valid. Otherwise, please at least read the full review closely before complaining about events you weren't consulted on or involved with, and have imo little reason to force a re-visit of.

In D16579#428835, @smithjd wrote:

The change was ack'ed in this review, and this review closed by the commit.

There was no code that required further review, only binary changes. The posted concerns were addressed by D18826.

If you really are that concerned that this review is cleaned up, request yourself that the reviewers that requested changes clear their request. I don't need to ask them to when the request is no longer valid. Otherwise, please at least read the full review closely before complaining about events you weren't consulted on or involved with, and have imo little reason to force a re-visit of.

Hello, the tone of your comment is not appropriate.

Please avoid this.

I agree with @ngraham, you should have asked both commenters if they were in agreement with the content and scope of the review before landing it. This is the way reviews work.

The change, as you pushed it, was never open for review, so there was not even the chance to review.

The new test files change the tested tag from one allowed variant to another. Both variants exist in the wild, and you changed the test coverage to the one you seemed to be more important. A good change would have increased the coverage.

In D16579#428993, @bruns wrote:

The change, as you pushed it, was never open for review, so there was not even the chance to review.

Diff 44708 included these changes for review. No one requested changes then, and they were ack'ed since: https://phabricator.kde.org/D16579?vs=on&id=44708&whitespace=ignore-most#420650 .

Hello, the tone of your comment is not appropriate.

You might as well have said, 'the change as you pushed it, fulfilled my request for changes, and there was not even a chance to lift it'. That's now praise instead of misinformation.

from the latest comments, it's not clear that their concerns were addressed first.

Also misinformation: https://phabricator.kde.org/D16579?vs=on&id=44708&whitespace=ignore-most#420650. There was also no dissension during the time since this comment was posted and the review was closed.

If any subset of review changes were already in principle agreed upon and with no requested changes, AND the code requested changed is to be removed from the final patch, AND the changes are binary-only, and the differences not rendered on the review ticket:

A) Further review is unnecessary, if only to allow the reviewers to cancel their (obsolete) change requests before it is closed;
B) I'm not aware where it is required that ALL reviewers must cancel their requested changes before the review is closed. The review could have been just as simply closed without the commit hook and the agreed repository changes made without referencing the review in the commit message, of course ignoring the fabulous background this review brought before it was turned into an argument over why it was closed with 2 reviewers still left to cancel their (superfluous) involvement as they see fit.