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

Translate the ISO-639-2/B codes to ISO-639-2/T. #13068

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

baka0815
Copy link

@baka0815 baka0815 commented Nov 19, 2024

Some languages differ in ISO-639-2/B and ISO-639-2/T. Those languages get not display properly, at least in the web UI (for example "German" is displayed as "Ger" because it's read as "ger" (ISO-639-2/B) and not "deu" (ISO-639-2/T)). With the addition of a small lookup table, those /B language codes get converted to /T language codes.
This enables 19 additional languages to be displayed correctly.

Before:
image
image

After:
image
image

Changes
Translate the ISO-639-2/B codes to ISO-639-2/T.

Issues
Fixes #12821

This enables 19 additional languages to be displayed correctly.
@crobibero
Copy link
Member

Please use a FrozenDictionary and place it in LocalizationManger.

@baka0815
Copy link
Author

@crobibero I didn't use a dictionary because the usage of r.ThreeLetterISOLanguageName.Equals(Language, StringComparison.OrdinalIgnoreCase) suggests that the case is not guaranteed to be upper- or lowercase. If Language is always lower case, then I can use a dictionary for a quicker lookup.

@crobibero
Copy link
Member

Just specify the comparer when creating the dictionary like so: .ToFrozenDictionary(StringComparer.OrdinalIgnoreCase)

@baka0815
Copy link
Author

The only place where the transformation from /B to /T would be used is inside the MediaStream class, which currently has no dependancy on LocalizationManager.
The translation itself is done via the .NET CultureInfo class and not the LocalizationManager, so I'm not sure moving the dictionary to the LocalizationManger is reasonable.

@photonconvergence
Copy link
Contributor

This should also resolve #7427 and partially resolve #6302

crobibero
crobibero previously approved these changes Nov 22, 2024
@crobibero
Copy link
Member

The test GetStreamScore_MediaStream_CorrectScore is failing

With the update that includes French I think we would need to also update iso6392.txt, and every user that has their preferred language set to French would need to resave (or write a migration for it) since it is saved as fre instead of fra

@crobibero crobibero dismissed their stale review November 22, 2024 14:19

changes needed

@baka0815

This comment was marked as outdated.

@baka0815
Copy link
Author

Ok, I got it now, sorry for the confusion.

I didn't want to change the property, just the display language. So using a local variable does the trick...

@nielsvanvelzen
Copy link
Member

Why do we need these values hardcoded in a dictionary when all this data is already in iso6392.txt?

@baka0815
Copy link
Author

I think the list is small enough, and I think there won't be many (any?) additional values. Reading from the text file would add much complexity for this. Or is the list already in some kind of structure in code?

@cvium
Copy link
Member

cvium commented Nov 23, 2024

I think the list is small enough, and I think there won't be many (any?) additional values. Reading from the text file would add much complexity for this. Or is the list already in some kind of structure in code?

public IEnumerable<CultureDto> GetCultures()

@baka0815
Copy link
Author

Ok, then I would need to get the list of cultures, iterate through the complete list (as it's not a dictionary) until I find an entry where ThreeLetterISOLanguageNames.Count > 1 and ThreeLetterISOLanguageNames[1] equals the language I'm searching.
That sounds not well optimized.

Can I add the functionality to the LocalizationManager then? As while loading the cultures would be the point where it's about free to check for the /B and /T variants and I could build the dictionary there to include all cases.
Then I would just need to know how to pass the LocalizationManager to the MediaStream.

@baka0815
Copy link
Author

DisplayTitle is called from DynamicHlsHelper and StreamInfo, both don't have a LocalizationManager.

MediaStream is initialized from BlurayDiscInfo, BdInfoExaminer, LiveTvMediaSourceProvider and other classes which also don't have a LocalizationManager.

So providing it in the constructor or converting DisplayTitle to a method with the LocalizationManager as a parameter would both mean a bit of a refactoring. Which way would be better or is there a third alternative I'm missing?

Changing the iso6932.txt from
ger|deu|de|German|allemand
to
deu|ger|de|German|allemand
seems to do nothing.

@baka0815
Copy link
Author

Ok, so what to do with this? I see three options:

  1. Merge as is. The list is small and there shouldn't be many (any) new additions
  2. Change the LocalizationManager and make DisplayTitle a method with the manager as parameter. Change DynamicHlsHelper and StreamInfo on the way
  3. Change the LocalizationManager and add it as a parameter to the constructor of MediaStream. Change BlurayDiscInfo, BdInfoExaminer, LiveTvMediaSourceProvider and others on the way

The change of the iso txt-file should also be done, I think. Currently the format is 0|1|2|3|4

where 0 and 1 are the three letter ISO 639 codes.
If 0 *and* 1 are set then 0 is the ISO 639-2/B code and 1 is the ISO 639-2/T (or ISO 639-3) code.
If only 0 is set then 1 is the ISO 639-2/T  (or ISO 639-3) code.
3 is the 2-letter ISO 639-1 code.
4 is the full language name in English.
5 is the full language name in French.
Examples:
fre|fra|fr|French|français
eng||en|English|anglais

@cvium @nielsvanvelzen @crobibero

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translation of stream language
7 participants