-
Notifications
You must be signed in to change notification settings - Fork 23
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
Support for extra out-of-region languages #36
Conversation
@@ -1251,8 +1307,7 @@ class NflxMultiSubsManager { | |||
|
|||
const movieChanged = manifest.movieId !== this.lastMovieId; | |||
if (!movieChanged) { | |||
//console.log(`Ignored: manifest ${manifest.movieId} already loaded`); | |||
return; | |||
console.log(`Manifest ${manifest.movieId} updated`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if there is better way to load updated manifest. Current behavior seems not only reloads manifest but also re-download subtitles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be possible to refactor in such a way that it doesn't re-download if the manifest is already active, but I think it is OK for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found out that this causes another problem. If secondaryLanguageMode
is audio
, this reset secondary subtitle back to the one that matches audio even if user has selected another one. My plan to solve this issue is:
- keep
rank
for each subtitle ingSubtitles
- insert newly-loaded subtitle into
gSubtitles
byrank
. keep other subtitles unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried this is adding a bit too much complexity. Maybe the better way to solve that issue is to just skip the auto language selection if the manifest is updated? Could reuse !movieChanged
to check for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took another approach, which also reused !movieChanged
to skip the auto language selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! This is working really well, only the way it uses secondaryLanguageLastUsed
could use some polishing. Please see the notes I added in the code. Thanks!
@@ -1251,8 +1307,7 @@ class NflxMultiSubsManager { | |||
|
|||
const movieChanged = manifest.movieId !== this.lastMovieId; | |||
if (!movieChanged) { | |||
//console.log(`Ignored: manifest ${manifest.movieId} already loaded`); | |||
return; | |||
console.log(`Manifest ${manifest.movieId} updated`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be possible to refactor in such a way that it doesn't re-download if the manifest is already active, but I think it is OK for now.
…btitle this make extra subtitle appears as a secondary subtitle option after user selects it as primary subtitle
resolves #29 partially?
see commit messages for details