PICARD-1092: Add support for preserving the SYLT tag #2373
Conversation
There was a problem hiding this comment.
This seems pretty good in principle.
I appreciate that the intent of this PR is to preserve (and display) ID3 SYLT tags, however the same concept applies equally to other tagging formats so, in addition to the comments below, I would also like to see support for loading and saving the same syncedlyrics metadata from other formats.
There was a problem hiding this comment.
Thank you, I appreciate your initiative very much. Adding synced lyrics support will need some work, so I hope you will stay with us to get this through. Not all need to be done as part of a single PR, but we at least need to identify what needs to be done next.
Most has already been said in the other reviews. I just want to stress three points:
-
Converting LRC to/from SYLT really needs to go into separate functions with a set of tests that handles different cases, including invalid ones.
-
Other formats should be handled as well. I don't know how this is currently done in those formats. Does anyone have some experience with synced lyrics used in Vorbis? Would we just store the LRC format there? ASF has this not useful gem in its documentation for WM/Lyrics_Synchronised:
For more information about how to use synchronized lyrics, see the documentation for the technology you are using.
However, I also think we can handle separate formats in separate PRs, especially if they are more complex. But then we should probably make sure the syncedlyrics tag is not yet written to all other formats to avoid issues later on.
-
The
syncedlyricstag needs to handle language from the beginning. I would suggest to follow my proposal on PICARD-2844. We should get this right this time from the beginning and avoid the mistakes done with the comment and lyrics tags.
twodoorcoupe
force-pushed
the
synced_lyrics_tag
branch
from
b0ad624 to
d386f25
Compare
|
Thank you all for the reviews! I've made the tag preserve the language, which should be easy to extend to the
When I was trying to put together a plugin to download synced lyrics for this request on the forum, I found out that most players just store the classic LRC format there. But I had a doubt about the mapping, because most players I've come across use |
|
When this is all sorted out and merged, will we need to update the Tag Mapping information in the documentation? If so, the updates should be entered in a PR updating https://github.com/metabrainz/picard-docs/blob/master/tag_mapping.py in the metabrainz/picard-docs project. |
twodoorcoupe
force-pushed
the
synced_lyrics_tag
branch
from
ed3e75d to
2f3ffee
Compare
|
I made it so the tag is not written to other formats for now like @phw suggested. |
There was a problem hiding this comment.
@twodoorcoupe This looks really good. I'm happy you tackled this issue. Let's merge this.
Would you be willing to update picard-docs as well, as @rdswift has outlined above?
Will do right away. |
|
Do you want my review or not? If you ask for it you might at least wait for me to do it before approving it. |
|
@Sophist-UK Yes please, your review would be very welcomed here.
Approval is after reviewing. I think this pull requests really benefits from multiple reviews. We don't need a competition who approves first. Currently this is still showing open review from you, so I would wait with merging until this is done. But if you don't want to review again or don't have time for it, just let us know. |
Summary
Allow to preserve the SYLT tag. Also add support for reading and writing the SYLT tag of ID3.
Problem
Like @phw said in the comments of the ticket, besides preserving the SYLT tag, it would be nice if Picard could both read and write this tag.
Solution
As it was suggested, this solution converts the SYLT tag to the LRC format for display, and it converts the LRC format back into the SYLT tag when saving.
Action