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.

Sign up for GitHub

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

Jump to bottom

Update: Improve subtitle selection algorithm #129

Merged
merged 2 commits into from
May 23, 2017
Merged

Update: Improve subtitle selection algorithm #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d6aabb
Update: Improve subtitle selection algorithm
May 20, 2017
111e32b
Merge branch 'master' into subtitle_preferences
bhh1988 May 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/lib/viewers/media/DashViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ class DashViewer extends VideoBaseViewer {
loadSubtitles() {
this.textTracks = this.player.getTextTracks().sort((track1, track2) => track1.id - track2.id);
if (this.textTracks.length > 0) {
this.mediaControls.initSubtitles(this.textTracks.map((track) => getLanguageName(track.language) || track.language));
this.mediaControls.initSubtitles(this.textTracks.map((track) => getLanguageName(track.language) || track.language), getLanguageName(this.options.location.locale.substring(0, 2)));
}
}
/**
Expand Down
5 changes: 3 additions & 2 deletions src/lib/viewers/media/MediaControls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,11 +805,12 @@ class MediaControls extends EventEmitter {
* Takes a list of subtitle names and populates the settings menu
*
* @param {Array} subtitles - A list of subtitle names as strings
* @param {string} language - Language of the user, as derived from their locale (e.g. en-US -> "English")
* @return {void}
*/
initSubtitles(subtitles) {
initSubtitles(subtitles, language) {
this.settings.addListener('subtitles', this.handleSubtitle);
this.settings.loadSubtitles(subtitles);
this.settings.loadSubtitles(subtitles, language);
}
}

Expand Down
38 changes: 32 additions & 6 deletions src/lib/viewers/media/Settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,10 @@ class Settings extends EventEmitter {

addActivationListener(this.settingsEl, this.menuEventHandler);
this.visible = false;
this.hasSubtitles = false;
this.containerEl.classList.add(CLASS_SETTINGS_SUBTITLES_UNAVAILABLE);
this.toggleToSubtitle = 0; // An index into the subtitles track list. Initialize with the first subtitle in list
this.subtitles = [];
this.language = undefined;
this.toggleToSubtitle = undefined; // An index into the subtitles list. Initialize with sentinel value
this.init();
}

Expand Down Expand Up @@ -510,6 +511,18 @@ class Settings extends EventEmitter {
return selected.getAttribute('data-value') !== '-1';
}

/**
* Returns whether subtitles are available or not
Copy link
Contributor

Choose a reason for hiding this comment

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

@return {boolean} Are subtitles available

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

*
* @return {boolean}
*/
hasSubtitles() {
if (this.settingsEl.querySelector(`[data-type="subtitles"][data-value="0"]${SELECTOR_SETTINGS_SUB_ITEM}`)) {
return true;
}
return false;
}

/**
* Toggles subtitles on/off
*
Expand All @@ -518,27 +531,40 @@ class Settings extends EventEmitter {
toggleSubtitles() {
if (this.areSubtitlesOn()) {
this.chooseOption('subtitles', '-1');
} else if (this.hasSubtitles) {
} else if (this.toggleToSubtitle !== undefined) {
this.chooseOption('subtitles', this.toggleToSubtitle.toString());
} else { // Do intelligent selection: Prefer user's language, fallback to English, then first subtitle in list
// Use the previewer's locale to determine preferred language
let idx = this.subtitles.findIndex((subtitle) => subtitle === this.language);
if (idx === -1) {
// Fall back to English if user's language doesn't exist
idx = this.subtitles.findIndex((subtitle) => subtitle === 'English');
if (idx === -1) {
idx = 0; // Fall back to first subtitle in list
}
}
this.chooseOption('subtitles', idx.toString());
}
}

/**
* Takes a list of subtitle names and populates the settings menu
*
* @param {Array} subtitles - A list of subtitle names as strings
* @param {string} language - The language of the user. Used in determining preferred subtitle when toggling
* @return {void}
*/
loadSubtitles(subtitles) {
loadSubtitles(subtitles, language) {
const subtitlesSubMenu = this.settingsEl.querySelector('.bp-media-settings-menu-subtitles');
subtitles.forEach((subtitle, idx) => {
this.subtitles = subtitles;
this.language = language;
this.subtitles.forEach((subtitle, idx) => {
insertTemplate(subtitlesSubMenu, SUBTITLES_SUBITEM_TEMPLATE.replace(/{{dataValue}}/g, idx));
const languageNode = subtitlesSubMenu.lastChild.querySelector('.bp-media-settings-value');
languageNode.textContent = subtitle;
});

this.containerEl.classList.remove(CLASS_SETTINGS_SUBTITLES_UNAVAILABLE);
this.hasSubtitles = true;
const subsCache = cache.get('media-subtitles');
if (subsCache !== null && subsCache !== '-1') { // Last video watched with subtitles, so turn them on here too
this.toggleSubtitles();
Expand Down
7 changes: 4 additions & 3 deletions src/lib/viewers/media/__tests__/DashViewer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ describe('lib/viewers/media/DashViewer', () => {
}
},
container: containerEl,
location: { locale: 'en-US' },
representation: {
content: {
url_template: 'url'
Expand Down Expand Up @@ -464,7 +465,7 @@ describe('lib/viewers/media/DashViewer', () => {
chinese
];
stubs.mockPlayer.expects('getTextTracks').returns(subs);
stubs.mockControls.expects('initSubtitles').withArgs(['Korean', 'Russian', 'English', 'Spanish', 'Chinese']);
stubs.mockControls.expects('initSubtitles').withArgs(['Korean', 'Russian', 'English', 'Spanish', 'Chinese'], 'English');

dash.loadSubtitles();

Expand All @@ -483,7 +484,7 @@ describe('lib/viewers/media/DashViewer', () => {
chinese
];
stubs.mockPlayer.expects('getTextTracks').returns(subs);
stubs.mockControls.expects('initSubtitles').withArgs(['Russian', 'Spanish', 'Korean', 'Chinese']);
stubs.mockControls.expects('initSubtitles').withArgs(['Russian', 'Spanish', 'Korean', 'Chinese'], 'English');

dash.loadSubtitles();

Expand All @@ -506,7 +507,7 @@ describe('lib/viewers/media/DashViewer', () => {
zero
];
stubs.mockPlayer.expects('getTextTracks').returns(subs);
stubs.mockControls.expects('initSubtitles').withArgs(['Russian', 'foo', 'und', '', 'doesntmatter', '0']);
stubs.mockControls.expects('initSubtitles').withArgs(['Russian', 'foo', 'und', '', 'doesntmatter', '0'], 'English');

dash.loadSubtitles();

Expand Down
55 changes: 52 additions & 3 deletions src/lib/viewers/media/__tests__/Settings-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('lib/viewers/media/Settings', () => {

it('should initialize as invisible and without subtitles', () => {
expect(settings.visible).to.be.false;
expect(settings.hasSubtitles).to.be.false;
expect(settings.hasSubtitles()).to.be.false;
expect(settings.areSubtitlesOn()).to.be.false;
expect(settings.containerEl).to.have.class('bp-media-settings-subtitles-unavailable');
});
Expand Down Expand Up @@ -602,6 +602,7 @@ describe('lib/viewers/media/Settings', () => {
describe('toggleSubtitles()', () => {
it('Should turn off subtitles if they were previously on', () => {
sandbox.stub(settings, 'chooseOption');
sandbox.stub(settings, 'hasSubtitles').returns(true);
sandbox.stub(settings, 'areSubtitlesOn').returns(true);

settings.toggleSubtitles();
Expand All @@ -610,15 +611,51 @@ describe('lib/viewers/media/Settings', () => {
});

it('Should turn on subtitles if they were previously off', () => {
settings.hasSubtitles = true;
sandbox.stub(settings, 'chooseOption');
sandbox.stub(settings, 'hasSubtitles').returns(true);
sandbox.stub(settings, 'areSubtitlesOn').returns(false);
settings.toggleToSubtitle = '2';

settings.toggleSubtitles();

expect(settings.chooseOption).to.be.calledWith('subtitles', '2');
});

it('Should prefer subtitle matching previewer language/locale', () => {
sandbox.stub(settings, 'chooseOption');
sandbox.stub(settings, 'hasSubtitles').returns(true);
sandbox.stub(settings, 'areSubtitlesOn').returns(false);
settings.subtitles = ['English', 'Spanish', 'Russian', 'French'];
settings.language = 'Spanish';

settings.toggleSubtitles();

expect(settings.chooseOption).to.be.calledWith('subtitles', '1');
});

it('Should prefer English subtitle if previewer language not in list', () => {
sandbox.stub(settings, 'chooseOption');
sandbox.stub(settings, 'hasSubtitles').returns(true);
sandbox.stub(settings, 'areSubtitlesOn').returns(false);
settings.subtitles = ['Spanish', 'Russian', 'English', 'French'];
settings.language = 'Mongolian';

settings.toggleSubtitles();

expect(settings.chooseOption).to.be.calledWith('subtitles', '2');
});

it('Should prefer first subtitle in list if previewer language not in list and English absent', () => {
sandbox.stub(settings, 'chooseOption');
sandbox.stub(settings, 'hasSubtitles').returns(true);
sandbox.stub(settings, 'areSubtitlesOn').returns(false);
settings.subtitles = ['Spanish', 'Russian', 'French'];
settings.language = 'Mongolian';

settings.toggleSubtitles();

expect(settings.chooseOption).to.be.calledWith('subtitles', '0');
});
});

describe('loadSubtitles()', () => {
Expand All @@ -628,7 +665,7 @@ describe('lib/viewers/media/Settings', () => {
settings.loadSubtitles(['English', 'Russian', 'Spanish']);

expect(subsMenu.children.length).to.equal(5); // Three languages, 'Off', and back to main menu
expect(settings.hasSubtitles).to.be.true;
expect(settings.hasSubtitles()).to.be.true;
expect(settings.containerEl).to.not.have.class('bp-media-settings-subtitles-unavailable');
});

Expand Down Expand Up @@ -672,6 +709,18 @@ describe('lib/viewers/media/Settings', () => {
});
});

describe('hasSubtitles()', () => {
it('Should be false before loading subtitles', () => {
expect(settings.hasSubtitles()).to.be.false;
});

it('Should be true after loading subtitles', () => {
settings.loadSubtitles(['English']);

expect(settings.hasSubtitles()).to.be.true;
});
});

describe('blurHandler()', () => {
it('should hide if click is outside of settings element', () => {
sandbox.stub(settings, 'hide');
Expand Down