This patch removes non-standard APE field names for 'Album Artist' from the test files.
All tests should pass.
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 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.
...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.
- 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.
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
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.
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?
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.
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.
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.