From 561cb73012244e5f050587136597564a7f6ddc32 Mon Sep 17 00:00:00 2001 From: bhh1988 Date: Tue, 23 May 2017 13:25:07 -0700 Subject: [PATCH] Update: Improve subtitle selection algorithm (#129) 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. --- src/lib/viewers/media/DashViewer.js | 2 +- src/lib/viewers/media/MediaControls.js | 5 +- src/lib/viewers/media/Settings.js | 38 +++++++++++-- .../media/__tests__/DashViewer-test.js | 7 ++- .../viewers/media/__tests__/Settings-test.js | 55 ++++++++++++++++++- 5 files changed, 92 insertions(+), 15 deletions(-) diff --git a/src/lib/viewers/media/DashViewer.js b/src/lib/viewers/media/DashViewer.js index d00ee7a0a..a9381b99d 100644 --- a/src/lib/viewers/media/DashViewer.js +++ b/src/lib/viewers/media/DashViewer.js @@ -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))); } } /** diff --git a/src/lib/viewers/media/MediaControls.js b/src/lib/viewers/media/MediaControls.js index 5cd846462..b4ddc160d 100644 --- a/src/lib/viewers/media/MediaControls.js +++ b/src/lib/viewers/media/MediaControls.js @@ -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); } } diff --git a/src/lib/viewers/media/Settings.js b/src/lib/viewers/media/Settings.js index 355104510..e32ae01ac 100644 --- a/src/lib/viewers/media/Settings.js +++ b/src/lib/viewers/media/Settings.js @@ -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(); } @@ -510,6 +511,18 @@ class Settings extends EventEmitter { return selected.getAttribute('data-value') !== '-1'; } + /** + * Returns whether subtitles are available or not + * + * @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 * @@ -518,8 +531,19 @@ 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()); } } @@ -527,18 +551,20 @@ class Settings 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 - 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(); diff --git a/src/lib/viewers/media/__tests__/DashViewer-test.js b/src/lib/viewers/media/__tests__/DashViewer-test.js index 4c3b34e86..9b0fe1905 100644 --- a/src/lib/viewers/media/__tests__/DashViewer-test.js +++ b/src/lib/viewers/media/__tests__/DashViewer-test.js @@ -34,6 +34,7 @@ describe('lib/viewers/media/DashViewer', () => { } }, container: containerEl, + location: { locale: 'en-US' }, representation: { content: { url_template: 'url' @@ -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(); @@ -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(); @@ -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(); diff --git a/src/lib/viewers/media/__tests__/Settings-test.js b/src/lib/viewers/media/__tests__/Settings-test.js index 386ef5ea8..b5ca22847 100644 --- a/src/lib/viewers/media/__tests__/Settings-test.js +++ b/src/lib/viewers/media/__tests__/Settings-test.js @@ -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'); }); @@ -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(); @@ -610,8 +611,8 @@ 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'; @@ -619,6 +620,42 @@ describe('lib/viewers/media/Settings', () => { 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()', () => { @@ -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'); }); @@ -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');