Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add non-standard multi-value audio tag support #5869

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

gnattu
Copy link
Member

@gnattu gnattu commented Aug 4, 2024

Changes

This adds the Audio Tag settings GUI to library settings for Music.

Issues

Depends on jellyfin/jellyfin#12385

@gnattu gnattu requested a review from a team as a code owner August 4, 2024 04:53
@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Aug 4, 2024

Cloudflare Pages deployment

Latest commit 9fb0c44
Status 🔄 Deploying...
Preview URL Not available
Type 🔀 Preview

View bot logs

@dmitrylyzo dmitrylyzo added the backend Requires work on the server to finish label Aug 4, 2024
Co-authored-by: Dmitry Lyzo <56478732+dmitrylyzo@users.noreply.github.com>
Copy link

sonarcloud bot commented Aug 4, 2024

@thornbill thornbill removed the backend Requires work on the server to finish label Sep 21, 2024
Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a UI perspective, everything is fine.

As for the server side, here performers are split by /. Maybe we should use custom delimiters there as well? Or not split at all (force users to use multiple Artist tags)?

Current behaviour:

mediaInfo.Artists: ["K/DA", "Madison Beer, (G)I-DLE, Lexie Liu, League of Legends, Jaira Burns, Seraphine"]
track.Artist: "K\u001fDA, Madison Beer, (G)I-DLE, Lexie Liu, League of Legends, Jaira Burns, Seraphine"
performers: ["K", "DA, Madison Beer, (G)I-DLE, Lexie Liu, League of Legends, Jaira Burns, Seraphine"]

@gnattu
Copy link
Member Author

gnattu commented Sep 23, 2024

From a UI perspective, everything is fine.

As for the server side, here performers are split by /. Maybe we should use custom delimiters there as well? Or not split at all (force users to use multiple Artist tags)?

Current behaviour:

mediaInfo.Artists: ["K/DA", "Madison Beer, (G)I-DLE, Lexie Liu, League of Legends, Jaira Burns, Seraphine"]
track.Artist: "K\u001fDA, Madison Beer, (G)I-DLE, Lexie Liu, League of Legends, Jaira Burns, Seraphine"
performers: ["K", "DA, Madison Beer, (G)I-DLE, Lexie Liu, League of Legends, Jaira Burns, Seraphine"]

It is so interesting that you can manually have \u001f in the tag... How did you even enter that?

I created files with / and it is not split. So I need extra info on how to reproduce your mysterious behavior

@dmitrylyzo
Copy link
Contributor

It is so interesting that you can manually have \u001f in the tag... How did you even enter that?

Not sure where it came from. I have 2F for / in K/DA (TPE1 tag).
Could it be from TagLib or something?

@gnattu
Copy link
Member Author

gnattu commented Sep 23, 2024

Not sure where it came from. I have 2F for / in K/DA (TPE1 tag).

Then it could be something magic happening for ASCII/Windows encoding to UTF-8 conversion. Because the internal separator is not intended for external use and I explicitly picked a control character in UTF-8 so that it is very very unlikely to be inserted by our users. The conversion thing is only my guess though, I don't know how the 2F becomes 1F exactly.

@gnattu
Copy link
Member Author

gnattu commented Sep 23, 2024

If my guessing is correct, you change private const char InternalValueSeparator = '\u001F'; to private const char InternalValueSeparator = '\u001E'; and that may workaround your issue. And if that is the case let's hope nothing will be converted to \u001E then.

@thornbill thornbill added the enhancement Improve existing functionality or small fixes label Sep 23, 2024
@thornbill thornbill added this to the v10.10.0 milestone Sep 23, 2024
@thornbill thornbill merged commit a2676c2 into jellyfin:master Sep 23, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants