PICARD-2468: Properly handle unspecified languages in ID3v2 comments

[Serial-ATA]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences:
  Picard no longer creates a duplicate COMM frame with the incorrect language when using lang='XXX' + desc=''.

Problem

Previously, a COMM frame with lang='XXX' desc='' would be saved as a separate frame with lang='eng' desc='XXX'. This is due to Picard supporting both comment:desc and comment:lang:desc syntax for comment tags.

• JIRA ticket (optional): PICARD-2468

Solution

A special case was added for lang='XXX', desc=''. I could not think of a better solution for the problem, as Picard normalizes all metadata keys. It would be more elegant if trailing colons were preserved.

Action

Possibly explore storing these comments as comment:XXX:, leaving the trailing colon as a marker for an empty description. This solution is not perfect. In the event that someone has comment:eng:XXX, all eng comments are simply stored as comment:desc. With this change, their language would be overwritten with XXX.

[phw]

Thanks a lot for this. I think as a workaround this probably makes sense and we could include it even in the next Picard 2.x release.

Essentially this is issue happens because the language notation got introduced as an afterthought (4d85c2e). Similar issues plague the still open PR #1650 for doing the same to the lyrics tag.

I guess we should reconsider this and make a harder cut for Picard 3, making the language not optional. I' thinking of having both colons be present even if language is empty, such as comment::desc. @zas what do you think?

[zas]

I guess we should reconsider this and make a harder cut for Picard 3, making the language not optional. I' thinking of having both colons be present even if language is empty, such as comment::desc.

Yes, I agree.

[phw]

Ok, let's go with this. The code itself is good to go. Please only add a test for this case to https://github.com/metabrainz/picard/blob/master/test/test_util_tags.py#L37

Also please rebase this branch. CI is currently failing due to changes to the Github Action Windows image, and this has been fixed on the master branch.

[Serial-ATA]

Hey @phw is there anything else I need to do for this?

[phw]

@Serial-ATA I think no, I just could not get back to this earlier.

From my side I'd say we merge this as it is. It fixes the common issue while being fully backward compatible. We can also merge it then into the 2.x branch for a future release of Picard 2.

I still think we should rework this to completely eliminate the ambiguity. I added PICARD-2844 for that.

[zas]

LGTM