-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(languages): expand language config to accept arrays #9542
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
base: main
Are you sure you want to change the base?
Conversation
|
Related to #1591 |
|
Incremental code coverage: 69.70% |
|
FYI there was a discussion in topic linked and instead of your proposal, we were thinking about more configurable objects, see #1591 (comment), #1591 (comment) & #1591 (comment) |
|
I appreciate the effort, but I'm not confident this would hold up over time. I would strongly prefer reducing the set of streams using a list of preferences (see the linked comments from @tykus160). Allowing multiple languages introduces questions around multiple roles, codecs, and their possible combinations. The recent text track simplification likely makes integration more straightforward. We should be careful introducing this as a proper v5 API as we'll have to maintain backwards compatibility for a while. @KimKyuHoi, would you be willing to experiment with the proposed idea's in #1591 as opposed to solely allowing multiple languages? |
|
Yes, we can break backwards compatibility with the introduction of a next major version. I'm sorry I'm expanding scope so much for a change that should be relatively straight forward, but I feel we shall take advantage of the fact that we're going to bump a major version in order to get track preferences right from the get go. Nonetheless, we're here to help. @tykus160, do you think implementing this in I'd love to hear everyone else's opinion though! |
|
@matvp91 I think it's a good suggestion. Then, why don't we open a new issue, create a relation, and proceed further? I'm making this suggestion because I'm going to be taking charge of it anyway. I was concerned that if we proceeded with the existing PR, the resulting diff files would be too large. |
|
@KimKyuHoi what works best for you, I'm OK either way. |
Previously, the player only accepted a single preferred language for audio and text tracks. This was limiting because browsers provide a list of preferred languages through navigator.languages, not just one.
For example, when a user has their browser set to ['ko-KR', 'ko', 'en-US', 'en'], the player could only use the first one ('ko-KR'). If the content didn't have Korean audio or subtitles, the player had no way to know that English would be an acceptable fallback.
Now the player accepts an array of languages and checks them in order:
The config fields are renamed to reflect this change:
The demo app now uses navigator.languages directly, so users get sensible defaults based on their browser settings without any manual configuration.
BREAKING CHANGE: preferredAudioLanguage (string) → preferredAudioLanguages (string[])
BREAKING CHANGE: preferredTextLanguage (string) → preferredTextLanguages (string[])