Update: Improve subtitle selection algorithm #129
Conversation
| @@ -511,34 +512,57 @@ class Settings extends EventEmitter { | |||
| } | |||
|
|
|||
| /** | |||
| * Returns whether subtitles are available or not | |||
There was a problem hiding this comment.
@return {boolean} Are subtitles available
There was a problem hiding this comment.
Added the type info. But "Are subtitles available" is self-explanatory. I guarantee you no one's going to be ambiguous about that one. Looks like we don't have a convention of always having a description for the return statements anyway.
src/lib/viewers/media/Settings.js
Outdated
| this.toggleToSubtitle = 0; // An index into the subtitles track list. Initialize with the first subtitle in list | ||
| this.subtitles = []; | ||
| this.locale = undefined; | ||
| this.toggleToSubtitle = undefined; // An index into the subtitles list. Initialize with sentinel value |
There was a problem hiding this comment.
@JustinHoldstock suggested that we start documenting all of the properties of a class at the top of the function. That way we know all of them beforehand and can document what they are for and don't have ones declared dynamically that a dev would need to search around for the definition of.
The syntax is:
class Foo {
/**
* Whether settings are open or not
*
* @property {boolean}
*/
visible;
/**
* Array of supported subtitles
*
* @property {Array}
*/
subtitles;
...
constructor() { }
FYI @pramodsum @JustinHoldstock @jeremypress ^
We already do this in the UI Kits and it's enforced through Flow. I can't find a comparable ESLint rule that would enforce the same (I'm not sure if there is one?) - if you could investigate @JustinHoldstock that'd be great, thanks!
We already do this in Preview.js
There was a problem hiding this comment.
Will do this in a separate commit afterwards. This helped me find a bug btw - this.locale should actually be this.language.
src/lib/viewers/media/Settings.js
Outdated
| this.chooseOption('subtitles', this.toggleToSubtitle.toString()); | ||
| } else { // Do some intelligent selection: Prefer user's language if exists, then fallback to English, then fallback to first subtitle in list |
There was a problem hiding this comment.
We may want to turn on prettier, which some other teams are using to automatically format code.
src/lib/viewers/media/Settings.js
Outdated
| let idx = [].findIndex.call(this.subtitles, (subtitle) => subtitle === this.language); | ||
| if (idx === -1) { | ||
| // Fall back to English if user's language doesn't exist | ||
| idx = [].findIndex.call(this.subtitles, (subtitle) => subtitle === 'English'); |
There was a problem hiding this comment.
this.subtitles.findIndex, findIndex is polyfilled in polyfill.js
When selecting subtitles from scratch (toggle-on button) we were previously selecting the first subtitle in the list. Instead, in this commit we will go by the user's locale - if the user's language isn't available, we fall back to English, and if English isn't available, we fall back to the first language in the subtitles menu. This is similar to what YouTube does.
When selecting subtitles from scratch (toggle-on button) we were
previously selecting the first subtitle in the list. Instead, in this
commit we will go by the user's locale - if the user's language isn't
available, we fall back to English, and if English isn't available, we
fall back to the first language in the subtitles menu. This is similar
to what YouTube does.