PICARD-1877: Support language for lyrics tags #1650
Conversation
Lyrics tags will be set as lyrcis[:lang[:desc]], where lang can also be empty to default to 'xxx'.
| if match: | ||
| lang = match.group('lang') or default_language | ||
| desc = match.group('desc') or '' | ||
| return (lang, desc) |
There was a problem hiding this comment.
Perhaps return lang.lower() here, to simplify comparisons elsewhere.
| @@ -159,3 +159,36 @@ def parse_comment_tag(name): # noqa: E302 | |||
| lang = match.group(1) | |||
| desc = desc[4:] | |||
| return (lang, desc) | |||
|
|
|||
|
|
|||
| RE_LYRICS_TAG = re.compile('^(?P<name>[^:]+)(?::(?P<lang>[a-zA-Z]{3})?)?(?::(?P<desc>.*))?$') | |||
There was a problem hiding this comment.
In case of 'lyrics:eng' it will return empty desc and lang='eng', but in case of 'lyrics:123' it will return empty lang and '123' as desc. Is this the expected behavior?
>>> match = RE_LYRICS_TAG.match('a:123')
>>> match.groups()
('a', None, '123')
>>> match = RE_LYRICS_TAG.match('a:eng')
>>> match.groups()
('a', 'eng', None)
>>> match = RE_LYRICS_TAG.match('a:eng:')
>>> match.groups()
('a', 'eng', '')There was a problem hiding this comment.
It kind of was intentional, using the second element as language only if it is a valid 3 letter language code (as this is what is also supported in ID3). It also serves as a fallback if one should indeed use a lyrics:something tag with something being the description. But my thinking was also that lyrics with description are probably not that common, but setting the language for lyrics is more important. That's probably a bit different to how it is with comments!?
Anyway, this is really messy and I'm unsure how to proceed. Ideally I would like both comment and lyrics the same, ideally without breaking someones existing use case. But maybe we need to break things a bit here.
There was a problem hiding this comment.
I'm not against breaking changes if required to improve overall user experience on the long term. It just have to be well documented.
|
any progress on this pull request ? |
Summary
Support loading the language for ID3 lyrics tags. This avoids Picard overwriting the lyrics on existing USLT tags with "xxx" (which is mutagen's default if no language is given).
Problem
Picard overwrites existing language information on USLT tags.
Solution
Lyrics tags will be set as lyrcis[:lang[:desc]], where lang can also be empty to default to 'xxx'.
Note: This is very similar to how comments behave, but different. I tried to completely unify it, but it is difficult if we want to keep backward compatibility and compatibility with other formats.
The differences are, how partial and empty values will be treated.
For lyrics only ID3 supports descriptions and languages, for all other tag formats it gets converted to just a plain lyrics tag. In ID3 the following are valid:
lyrics(default language xxx and no description)lyrics:lng:desc(language and description)lyrics:lng(only language)lyrics::description(only description)lyrics:something that is not a 3 char language code(only description, because the only value is never a valid language code; this is mainly to provide some backward compatibility should someone use this format ins scripts)For comments it is different, as we supported description before:
comment(default language eng and no descriptioncomment:des(default language eng and short description)comment:lng:des(language + description)This is because we introduced the language support later and support plain "comment:description" also for e.g. APEv2. We probably should unify this, but it requires a bit of refactoring to existing formats and tests and is a small backward incompatible change.