Subtitles #117

bhh1988 · 2017-05-16T01:28:39Z

No description provided.

jeremypress · 2017-05-16T17:33:48Z

src/lib/viewers/media/DashViewer.js

+     */
+    handleSubtitle() {
+        const subtitleIdx = parseInt(cache.get('media-subtitles'), 10);
+        if (subtitleIdx >= 0 && subtitleIdx < this.textTracks.length) {


nit: could just check for if (this.textTracts[subtitileIdx]) {

jeremypress · 2017-05-16T17:35:46Z

src/lib/viewers/media/DashViewer.js

@@ -279,9 +300,21 @@ class DashViewer extends VideoBaseViewer {
    addEventListenersForMediaControls() {
        super.addEventListenersForMediaControls();
        this.mediaControls.addListener('qualitychange', this.handleQuality);
+        this.mediaControls.addListener('subtitlechange', this.handleSubtitle);


This needs to be removed like qualitychange is. You could also just call removeAllListeners in the destroy method. https://nodejs.org/api/events.html#events_emitter_removealllisteners_eventname

Thanks. Removing subtitlechange event in destroy. Can follow up with another commit later to refactor things to call removeAllListeners, since that seems generally better practice.

What's your thinking on removeAllListeners vs removeListener? Typically I like explicitly removing only what I add since that lowers the probability you're stomping through some other listener.

but if it's in a function like destroy(), is there still a risk?

Probably not.

jeremypress · 2017-05-16T17:39:35Z

src/lib/viewers/media/DashViewer.js

+     * @return {void}
+     */
+    loadSubtitles() {
+        this.textTracks = this.player.getTextTracks().sort((track1, track2) => { return track1.id - track2.id; });


nit: if you omit the '{}' for the function body in a one-liner like this there is an implicit return.

this.player.getTextTracks().sort((track1, track2) => track1.id - track2.id); looks a little cleaner.

cool. Done.

jeremypress · 2017-05-16T17:40:29Z

src/lib/viewers/media/DashViewer.js

+    loadSubtitles() {
+        this.textTracks = this.player.getTextTracks().sort((track1, track2) => { return track1.id - track2.id; });
+        if (this.textTracks.length > 0) {
+            this.mediaControls.initSubtitles(this.textTracks.map((track) => { return track.language; }));


Same here, can remove the {} and return if you'd like.

jeremypress · 2017-05-16T17:42:53Z

src/lib/viewers/media/MediaControls.html

@@ -35,6 +35,10 @@
        <label class="bp-media-controls-label bp-media-controls-timecode">00:00</label>
        <span class="bp-media-controls-label">&nbsp;&#47;&nbsp;</span>
        <label class="bp-media-controls-label bp-media-controls-duration">00:00</label>
+        <button class="bp-media-controls-btn bp-media-cc-icon">
+            <span class="bp-media-controls-cc-text">CC</span>


nit: rename this to "bp-media-cc-icon-text" to match with the 'icon-on' and parent button?

jeremypress · 2017-05-16T17:46:49Z

src/lib/viewers/media/MediaControls.js

+     */
+    toggleSubtitles() {
+        this.show();
+        this.emit('togglesubtitles');


nit: I've had trouble following this myself but I believe our convention is to emit absolutely last in a function. Here it wouldn't really make a difference but if we ran some time intensive code after it could cause issues.

Makes sense, good catch. Done.

jeremypress · 2017-05-16T17:52:44Z

src/lib/viewers/media/Settings.js

+        this.settingsEl.style.width = `${menu.offsetWidth + 18}px`;
+        // height = n * $item-height + 2 * $padding (see Settings.scss)
+        // where n is the number of displayed items in the menu
+        const sumHeight = [].reduce.call(menu.children, (sum, child) => { return sum + child.offsetHeight; }, 0);


nit: can avoid the return here

jeremypress · 2017-05-16T17:54:17Z

src/lib/viewers/media/Settings.js

+            if (prevValue !== newValue) {
+                this.toggleToSubtitle = prevValue;
+            }
+            this.containerEl.classList.remove(CLASS_SETTINGS_SUBTITLES_ON);


nit: new line

Not sure what you mean here.

We generally add a line break after if blocks before other chunks of code

jeremypress · 2017-05-16T17:55:28Z

src/lib/viewers/media/Settings.js

+        const subtitlesSubMenu = this.settingsEl.querySelector('.bp-media-settings-menu-subtitles');
+        subtitles.forEach((subtitle, idx) => {
+            insertTemplate(subtitlesSubMenu, SUBTITLES_SUBITEM_TEMPLATE.replace(/{{language}}/g, subtitle).replace(/{{dataValue}}/g, idx));
+        });


nit: I usually add new lines after any {} block

Not sure what you mean here.

Done, I think. Lemme know if I did it wrong.

jeremypress · 2017-05-16T17:57:56Z

src/lib/viewers/media/Settings.js

+        });
+        this.containerEl.classList.remove(CLASS_SETTINGS_SUBTITLES_UNAVAILABLE);
+        const subsCache = cache.get('media-subtitles');
+        if (subsCache !== null && subsCache !== '-1') { // Last video watched with subtitles, so turn them on here too


nit: doing if (subscache) checks for falsey values, so it would catch things like null and undefined.

Yea but it also catches things like empty string or 0. 0 in particular can be a problem because it's a valid index into the subtitle track list (though it should always be a string, so we should be fine, but just in case...)

tonyjin · 2017-05-16T21:51:41Z

src/lib/viewers/media/DashViewer.js

@@ -279,9 +300,21 @@ class DashViewer extends VideoBaseViewer {
    addEventListenersForMediaControls() {
        super.addEventListenersForMediaControls();
        this.mediaControls.addListener('qualitychange', this.handleQuality);
+        this.mediaControls.addListener('subtitlechange', this.handleSubtitle);


What's your thinking on removeAllListeners vs removeListener? Typically I like explicitly removing only what I add since that lowers the probability you're stomping through some other listener.

tonyjin · 2017-05-16T22:01:38Z

src/lib/viewers/media/Settings.js

+            if (prevValue !== newValue) {
+                this.toggleToSubtitle = prevValue;
+            }
+            this.containerEl.classList.remove(CLASS_SETTINGS_SUBTITLES_ON);


We generally add a line break after if blocks before other chunks of code

tonyjin · 2017-05-16T22:04:19Z

src/lib/viewers/media/Settings.js

+    toggleSubtitles() {
+        if (this.subtitlesOn) {
+            this.chooseOption('subtitles', '-1');
+            this.subtitlesOn = false;


Can we infer that subtitles are on / off without adding a property? (checking what option is set already, checking a shaka player property?, etc)

Good tip. I'm going with checking the menu to see if it's off.

tonyjin · 2017-05-16T22:04:33Z

src/lib/viewers/media/Settings.js

+        const subtitlesSubMenu = this.settingsEl.querySelector('.bp-media-settings-menu-subtitles');
+        subtitles.forEach((subtitle, idx) => {
+            insertTemplate(subtitlesSubMenu, SUBTITLES_SUBITEM_TEMPLATE.replace(/{{language}}/g, subtitle).replace(/{{dataValue}}/g, idx));
+        });


tonyjin · 2017-05-16T22:05:24Z

src/lib/viewers/media/Settings.js

+        const subsCache = cache.get('media-subtitles');
+        if (subsCache !== null && subsCache !== '-1') { // Last video watched with subtitles, so turn them on here too
+            this.toggleSubtitles();
+        }


Same thing with line breaks - we generally prefer less dense-looking code.

tonyjin · 2017-05-17T00:00:35Z

src/lib/viewers/media/Settings.js

+     * @private
+     * @return {boolean}
+     */
+    subtitlesOn() {


areSubtitlesOn() - the current name is ambiguous between turning subtitles on vs checking if they're on

tonyjin · 2017-05-17T00:01:58Z

src/lib/viewers/media/Settings.js

+     * @param {string} type - The submenu (e.g. speed, quality, subtitles)
+     * @return {HTMLElement} The sub-item html element from the Settings menu that's selected
+     */
+    selectedOption(type) {


getSelectedOption(type) - function names should contain verbs if you're retrieving a value.

priyajeet · 2017-05-17T01:04:19Z

src/lib/viewers/media/Settings.js

-                                this.showSubMenu('speed');
-                            } else if (curNode.getAttribute('data-type') === 'quality') {
-                                this.showSubMenu('quality');
+                            const dataType = curNode.getAttribute('data-type');


You can combine the if blocks and reduce arrowness of the function.

Combining two if-blocks but leaving the top one, just for protection.

priyajeet · 2017-05-17T01:07:05Z

src/lib/viewers/media/Settings.js

@@ -80,6 +87,21 @@ const SETTINGS_TEMPLATE = `<div class="bp-media-settings">
            <div class="bp-media-settings-value">${__('media_quality_auto')}</div>
        </div>
    </div>
+    <div class="bp-media-settings-menu-subtitles bp-media-settings-menu bp-media-settings-is-hidden" role="menu">
+        <div class="bp-media-settings-sub-item bp-media-settings-sub-item-subtitles" data-type="menu" tabindex="0" role="menuitem" aria-haspopup="true">


If you are working on media stuff later, make all these classes variables.
We are repeating so many strings that we can just do <div class="${CLASSNAME}">.
Should reduce file size.

ok i'll keep that in mind next time

priyajeet · 2017-05-17T01:08:13Z

src/lib/viewers/media/DashViewer.js

            this.mediaControls.removeListener('qualitychange', this.handleQuality);
+            this.mediaControls.removeListener('subtitlechange', this.handleSubtitle);


Why arent the last 2 in removeAllListeners()?

Yea great question.... @jeremypress ?? I didn't notice removeAllListeners is already being called. Are these calls necessary?

priyajeet · 2017-05-17T01:23:54Z

src/lib/viewers/media/DashViewer.js

+            this.emit('subtitlechange', track.language);
+        } else {
+            this.player.setTextTrackVisibility(false);
+            this.emit('subtitlechange', __('off'));


Do not localize event data, its for developers using the preview sdk and not for end users.

priyajeet · 2017-05-17T01:25:42Z

src/lib/viewers/media/DashViewer.js

+     */
+    handleSubtitle() {
+        const subtitleIdx = parseInt(cache.get('media-subtitles'), 10);
+        if (this.textTracks[subtitleIdx] !== undefined) {


unnecessary !== undefined
if (this.textTracks[subtitleIdx]) should suffice, with it reveresed

empty string

priyajeet · 2017-05-17T01:28:13Z

src/lib/viewers/media/DashViewer.js

+     */
+    loadSubtitles() {
+        this.textTracks = this.player.getTextTracks().sort((track1, track2) => track1.id - track2.id);
+        if (this.textTracks.length > 0) {


Should we be > 1?
As in should we show the menu at all if there is only 1 track?

Yes, because then they can see what language it is for instance

Added subtitles to the video menu. The subtitles menu only shows if the video is detected to have subtitles in it. This is only supported in the DashViewer. There is also a CC button for toggling subtitles on/off

jeremypress reviewed May 16, 2017

View reviewed changes

tonyjin reviewed May 16, 2017

View reviewed changes

tonyjin reviewed May 17, 2017

View reviewed changes

tonyjin approved these changes May 17, 2017

View reviewed changes

priyajeet reviewed May 17, 2017

View reviewed changes

New: Support closed-captions/subtitles

bd4029d

Added subtitles to the video menu. The subtitles menu only shows if the video is detected to have subtitles in it. This is only supported in the DashViewer. There is also a CC button for toggling subtitles on/off

bhh1988 merged commit 4c56532 into box:master May 17, 2017

		this.mediaControls.removeListener('qualitychange', this.handleQuality);
		this.mediaControls.removeListener('subtitlechange', this.handleSubtitle);

Subtitles #117

Subtitles #117

Conversation

bhh1988 commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyajeet May 17, 2017 •

edited

Loading

priyajeet May 17, 2017 •

edited

Loading