fix(taglib): parse both id3 and aliased tags, as lyrics appears to be mapped to lyrics-xxx #4310
Conversation
… mapped to lyrics-xxx
There was a problem hiding this comment.
Summary of Changes
Hello @kgarner7, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a bug in how taglib handles lyrics metadata, specifically when both generic ID3-style lyrics and format-specific or aliased lyrics tags are present. The core change ensures that all available lyrics are correctly parsed and combined, preventing data loss or duplication, and is backed by comprehensive new test cases across various audio formats.
Highlights
- Lyrics Parsing Logic Enhancement: I've updated the
processPairMappingfunction inmodel/metadata/metadata.goto ensure that both ID3-style lyrics tags (e.g.,lyrics:xxx) and any aliased lyrics tags (likeunsyncedlyrics) are always parsed and combined. Previously, there was a potential for one to be preferred over the other, leading to incomplete lyrics data. Now, ID3-based tags are always collected first, and then aliased tags are appended, ensuring comprehensive lyrics extraction. - Refined Lyrics Tag Aliases: In
resources/mappings.yaml, I've removed specific format-dependent lyrics aliases (©lyrandwm/lyrics). This change was made becausetaglibconsistently maps these to the genericlyrics:xxxtag. By removing these redundant aliases, the parsing logic is simplified, and potential duplication or conflicts in lyrics data are avoided, aligning withtaglib's actual behavior. - Expanded Lyrics Test Coverage: I've added a significant suite of new end-to-end tests in
adapters/taglib/end_to_end_test.goto thoroughly validate the improved lyrics parsing. These tests cover various scenarios, including fetching both synced and unsynced lyrics from mixed FLAC files, handling USLT and SYLT tags in MP3s, and a parameterized test table that verifies correct lyrics extraction across a wide range of audio formats (FLAC, M4A, OGG, WMA, WV, WAV, AIFF), ensuring the fix works universally.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where both ID3 and aliased lyrics tags were not being parsed simultaneously. The logic change in model/metadata/metadata.go is sound, and the accompanying update to resources/mappings.yaml is consistent with the fix. The new tests in adapters/taglib/end_to_end_test.go are comprehensive and cover various formats and scenarios. I've added a few suggestions to improve the maintainability and consistency of the new test code by refactoring duplicated logic.
Description
Taglib appears to map certain lyrics tags to
lyrics:xxx, or in the case of at least WMA (wm/lyrics) and M4a (@lyr), duplicates those values. For items which havelyrics:xxx(mapped by whatever tag) and an aliased field (unsyncedlyrics), the two could not be parsed at the same time.Now, always parse ID3 tags for pairs (this is primarily relevant for lyrics). Because of the duplicate behavior for wma and m4a, also remove those format-specific items from the tags.
Related Issues
Type of Change
Checklist
Please review and check all that apply:
How to Test
make test PKG=./adapters/taglib
Screenshots / Demos (if applicable)
Additional Notes
...why does taglib do this? I can't see it from the source. Discussion: https://discord.com/channels/671335427726114836/1391091057911005255