From 5917ed063da1969259c0c4bb21dbc2ac4e5af4eb Mon Sep 17 00:00:00 2001 From: ataki Date: Thu, 5 Feb 2015 15:40:23 -0800 Subject: [PATCH] hard start/stop times to video player This pull request makes the video player module respect the video start/stop times specified by the instructor as a hard start/stops, i.e. the video treats the start time as the beginning of the video and the stop time as the end. Previously, the video module displayed the full video and displayed a jquery ui slider range representing the start and end times. This new functionality invalidates two previous acceptance tests for the video module: - slider visible test - finish time video acceptance test. These tests are removed from the common acceptance tests. --- .../xmodule/js/spec/video/sjson_spec.js | 44 ++++++++++++-- .../js/spec/video/video_caption_spec.js | 4 ++ .../js/spec/video/video_control_spec.js | 25 ++++---- .../spec/video/video_progress_slider_spec.js | 2 + .../xmodule/xmodule/js/src/video/00_sjson.js | 50 +++++++++++++++- .../xmodule/js/src/video/03_video_player.js | 11 ++-- .../xmodule/js/src/video/04_video_control.js | 5 +- .../js/src/video/06_video_progress_slider.js | 36 +++++------- .../xmodule/js/src/video/09_video_caption.js | 58 ++++++++++++++++--- .../tests/video/test_studio_video_module.py | 2 - .../tests/video/test_video_times.py | 25 -------- 11 files changed, 178 insertions(+), 84 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/video/sjson_spec.js b/common/lib/xmodule/xmodule/js/spec/video/sjson_spec.js index b827246104..1f4bef8939 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/sjson_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/sjson_spec.js @@ -5,6 +5,8 @@ function (Sjson) { describe('Sjson', function () { var data = jasmine.stubbedCaption, sjson; + var videoStops = [0, 3120, 6270, 8490, 21620, 24920]; + var OUT_OF_BOUNDS_STOP = 10024920; beforeEach(function() { sjson = new Sjson(data); @@ -23,12 +25,42 @@ function (Sjson) { }); it('search returns a correct caption index', function () { - expect(sjson.search(0)).toEqual(-1); - expect(sjson.search(3120)).toEqual(1); - expect(sjson.search(6270)).toEqual(2); - expect(sjson.search(8490)).toEqual(2); - expect(sjson.search(21620)).toEqual(4); - expect(sjson.search(24920)).toEqual(5); + expect(sjson.search(videoStops[0])).toEqual(0); + expect(sjson.search(videoStops[1])).toEqual(1); + expect(sjson.search(videoStops[2])).toEqual(2); + expect(sjson.search(videoStops[3])).toEqual(2); + expect(sjson.search(videoStops[4])).toEqual(4); + expect(sjson.search(videoStops[5])).toEqual(5); + }); + + it('search returns the last entry for a value outside the bounds of the array', function() { + expect(sjson.search(OUT_OF_BOUNDS_STOP)).toEqual(sjson.getCaptions().length - 1); + }); + + it('search returns the first entry for a negative index in the array', function() { + expect(sjson.search(-1)).toEqual(0); + }); + + it('search only searches through a subrange of times if start / end times are specified', function () { + var start = videoStops[2] - 100; + var end = videoStops[5] - 100; + var results = sjson.filter(start, end); + var expectedLength = results.captions.length - 1; + + expect(sjson.search(videoStops[0], start, end)).toEqual(0); + expect(sjson.search(videoStops[1], start, end)).toEqual(0); + expect(sjson.search(videoStops[2], start, end)).toEqual(0); + expect(sjson.search(videoStops[3], start, end)).toEqual(0); + expect(sjson.search(OUT_OF_BOUNDS_STOP, start, end)).toEqual(expectedLength); + }); + + it('filters results correctly given a start and end time', function () { + var start = videoStops[1] - 100; + var end = videoStops[4] - 100; + var results = sjson.filter(start, end); + + expect(results.start.length).toEqual(3); + expect(results.captions.length).toEqual(3); }); }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js index 6420cfffa7..39778d7ba6 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js @@ -799,6 +799,10 @@ state.videoCaption.updatePlayTime(25.000); expect(state.videoCaption.currentIndex).toEqual(5); + // To test speed, don't use start / end times. + state.config.startTime = 0; + state.config.endTime = null; + // Flash mode state.speed = '2.0'; spyOn(state, 'isFlashMode').andReturn(true); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_control_spec.js index 21cd0587ef..98569620c2 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_control_spec.js @@ -278,7 +278,8 @@ describe('constructor with end-time', function () { it( - 'saved position is 0, timer slider and VCR set to 0:00', + 'saved position is 0, timer slider and VCR set to 0:00 ' + + 'and ending at specified end-time', function () { var duration, sliderEl, expectedValue; @@ -301,7 +302,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:00 / 1:00'); + expect(expectedValue).toHaveText('0:00 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(0); @@ -335,7 +336,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:15 / 1:00'); + expect(expectedValue).toHaveText('0:15 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(15); @@ -369,7 +370,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:00 / 1:00'); + expect(expectedValue).toHaveText('0:00 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(0); @@ -403,7 +404,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:00 / 1:00'); + expect(expectedValue).toHaveText('0:00 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(0); @@ -438,7 +439,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:00 / 1:00'); + expect(expectedValue).toHaveText('0:00 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(0); @@ -450,7 +451,7 @@ describe('constructor with start-time and end-time', function () { it( - 'saved position is 0, timer slider and VCR set to start-time', + 'saved position is 0, timer slider and VCR set to appropriate start and end times', function () { var duration, sliderEl, expectedValue; @@ -474,7 +475,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:10 / 1:00'); + expect(expectedValue).toHaveText('0:10 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(10); @@ -509,7 +510,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:15 / 1:00'); + expect(expectedValue).toHaveText('0:15 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(15); @@ -544,7 +545,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:10 / 1:00'); + expect(expectedValue).toHaveText('0:10 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(10); @@ -579,7 +580,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:10 / 1:00'); + expect(expectedValue).toHaveText('0:10 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(10); @@ -614,7 +615,7 @@ runs(function () { expectedValue = $('.video-controls').find('.vidtime'); - expect(expectedValue).toHaveText('0:10 / 1:00'); + expect(expectedValue).toHaveText('0:10 / 0:20'); expectedValue = sliderEl.slider('option', 'value'); expect(expectedValue).toBe(10); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js index 629b6e3833..d7a5685719 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_progress_slider_spec.js @@ -26,6 +26,8 @@ expect(state.videoProgressSlider.slider).toBe('.slider'); expect($.fn.slider).toHaveBeenCalledWith({ range: 'min', + min: 0, + max: null, change: state.videoProgressSlider.onChange, slide: state.videoProgressSlider.onSlide, stop: state.videoProgressSlider.onStop diff --git a/common/lib/xmodule/xmodule/js/src/video/00_sjson.js b/common/lib/xmodule/xmodule/js/src/video/00_sjson.js index b140a2e31c..2ba222d6db 100644 --- a/common/lib/xmodule/xmodule/js/src/video/00_sjson.js +++ b/common/lib/xmodule/xmodule/js/src/video/00_sjson.js @@ -27,14 +27,24 @@ function() { return sjson.text.length; }; - var search = function (time) { + function search(time, startTime, endTime) { var start = getStartTimes(), max = size() - 1, min = 0, + results, index; - if (time < start[min]) { - return -1; + // if we specify a start and end time to search, + // search the filtered list of captions in between + // the start / end times. + // Else, search the unfiltered list. + if (typeof startTime !== 'undefined' && + typeof endTime !== 'undefined') { + results = filter(startTime, endTime); + start = results.start; + max = results.captions.length - 1; + } else { + start = getStartTimes(); } while (min < max) { index = Math.ceil((max + min) / 2); @@ -51,10 +61,44 @@ function() { return min; }; + function filter(start, end) { + var filteredTimes = []; + var filteredCaptions = []; + var startTimes = getStartTimes(); + var captions = getCaptions(); + var currentStartTime; + var i; + + if (startTimes.length !== captions.length) { + return []; + } + + // if end is null, then it's been set to + // some erroneous value, so filter using the + // entire array as long as it's not empty + if (end === null && startTimes.length) { + end = startTimes[startTimes.length - 1]; + } + + for (i = 0; i < startTimes.length; i++) { + currentStartTime = startTimes[i]; + if (currentStartTime >= start && currentStartTime <= end) { + filteredTimes.push(currentStartTime); + filteredCaptions.push(captions[i]); + } + } + + return { + 'start': filteredTimes, + 'captions': filteredCaptions + }; + } + return { getCaptions: getCaptions, getStartTimes: getStartTimes, getSize: size, + filter: filter, search: search }; }; diff --git a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js index bd9e9d2772..b0c0454408 100644 --- a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js +++ b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js @@ -320,7 +320,6 @@ function (HTML5Video, Resizer) { // When the video will start playing again from the start, the // start-time and end-time will come back into effect. this.videoPlayer.goToStartTime = true; - this.videoPlayer.stopAtEndTime = true; } this.videoPlayer.player.playVideo(); @@ -340,11 +339,9 @@ function (HTML5Video, Resizer) { // than end-time. Also, we must make sure that this is only done // once per video playing from start to end. if ( - this.videoPlayer.stopAtEndTime && this.videoPlayer.endTime !== null && this.videoPlayer.endTime <= this.videoPlayer.currentTime ) { - this.videoPlayer.stopAtEndTime = false; this.videoPlayer.pause(); @@ -463,9 +460,6 @@ function (HTML5Video, Resizer) { // After the user seeks, the video will start playing from // the sought point, and stop playing at the end. this.videoPlayer.goToStartTime = false; - if (time > this.videoPlayer.endTime || this.videoPlayer.endTime === null) { - this.videoPlayer.stopAtEndTime = false; - } this.videoPlayer.seekTo(time); this.videoPlayer.log( @@ -769,7 +763,6 @@ function (HTML5Video, Resizer) { videoPlayer.endTime <= videoPlayer.startTime || videoPlayer.endTime >= duration ) { - videoPlayer.stopAtEndTime = false; videoPlayer.endTime = null; } else if (this.isFlashMode()) { videoPlayer.endTime /= Number(this.speed); @@ -825,6 +818,10 @@ function (HTML5Video, Resizer) { duration = this.videoPlayer.duration(), youTubeId; + if (this.config.endTime !== null) { + duration = Math.min(this.config.endTime, duration); + } + this.trigger( 'videoProgressSlider.updatePlayTime', { diff --git a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js index 084afc6932..c269ac4210 100644 --- a/common/lib/xmodule/xmodule/js/src/video/04_video_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/04_video_control.js @@ -296,7 +296,10 @@ function () { } function updateVcrVidTime(params) { - this.videoControl.vidTimeEl.html(Time.format(params.time) + ' / ' + Time.format(params.duration)); + var duration = (this.config.endTime !== null) ? this.config.endTime : params.duration; + // in case endTime is accidentally specified as being greater than the video + duration = Math.min(duration, params.duration); + this.videoControl.vidTimeEl.html(Time.format(params.time) + ' / ' + Time.format(duration)); } }); diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js index 2e20df0fbe..a267bcf9c1 100644 --- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js @@ -94,6 +94,8 @@ function () { this.videoProgressSlider.slider = this.videoProgressSlider.el .slider({ range: 'min', + min: this.config.startTime, + max: this.config.endTime, slide: this.videoProgressSlider.onSlide, stop: this.videoProgressSlider.onStop }); @@ -147,25 +149,6 @@ function () { // with actual starting and ending point of the video. rangeParams = getRangeParams(start, end, duration); - - if (!this.videoProgressSlider.sliderRange) { - this.videoProgressSlider.sliderRange = $('
', { - 'class': 'ui-slider-range ' + - 'ui-widget-header ' + - 'ui-corner-all ' + - 'slider-range' - }) - .css({ - left: rangeParams.left, - width: rangeParams.width - }); - - this.videoProgressSlider.sliderProgress - .after(this.videoProgressSlider.sliderRange); - } else { - this.videoProgressSlider.sliderRange - .css(rangeParams); - } } function getRangeParams(startTime, endTime, duration) { @@ -183,6 +166,10 @@ function () { var time = ui.value, duration = this.videoPlayer.duration(); + if (this.config.endTime !== null) { + duration = Math.min(this.config.endTime, duration); + } + this.videoProgressSlider.frozen = true; // Remember the seek to value so that we don't repeat ourselves on the @@ -235,8 +222,15 @@ function () { } function updatePlayTime(params) { - var time = Math.floor(params.time), - duration = Math.floor(params.duration); + var time = Math.floor(params.time); + // params.duration could accidentally be construed as a floating + // point double. Since we're displaying this number, round down + // to nearest second + var duration = Math.floor(params.duration); + + if (this.config.endTime !== null) { + duration = Math.min(this.config.endTime, duration); + } if ( this.videoProgressSlider.slider && diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 2643520029..27608e99a5 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -195,6 +195,42 @@ function (Sjson, AsyncProcess) { this.onMouseEnter(); }, + /** + * @desc Gets the correct start and end times from the state configuration + * + * @returns {array} if [startTime, endTime] are defined + */ + getStartEndTimes: function () { + // due to the way config.startTime/endTime are + // processed in 03_video_player.js, we assume + // endTime can be an integer or null, + // and startTime is an integer > 0 + var config = this.state.config; + var startTime = config.startTime * 1000; + var endTime = (config.endTime !== null) ? config.endTime * 1000 : null; + return [startTime, endTime]; + }, + + /** + * @desc Gets captions within the start / end times stored within this.state.config + * + * @returns {object} {start, captions} parallel arrays of + * start times and corresponding captions + */ + getBoundedCaptions: function () { + // get start and caption. If startTime and endTime + // are specified, filter by that range. + var times = this.getStartEndTimes(); + var results = this.sjson.filter.apply(this.sjson, times); + var start = results.start; + var captions = results.captions; + + return { + 'start': start, + 'captions': captions + }; + }, + /** * @desc Fetch the caption file specified by the user. Upon successful * receipt of the file, the captions will be rendered. @@ -244,9 +280,9 @@ function (Sjson, AsyncProcess) { data: data, success: function (sjson) { self.sjson = new Sjson(sjson); - - var start = self.sjson.getStartTimes(), - captions = self.sjson.getCaptions(); + var results = self.getBoundedCaptions(); + var start = results.start; + var captions = results.captions; if (self.loaded) { if (self.rendered) { @@ -622,11 +658,12 @@ function (Sjson, AsyncProcess) { * */ play: function () { + var startAndCaptions, start, end; if (this.loaded) { if (!this.rendered) { - var start = this.sjson.getStartTimes(), - captions = this.sjson.getCaptions(); - + startAndCaptions = this.getBoundedCaptions(); + start = startAndCaptions.start; + captions = startAndCaptions.captions; this.renderCaption(start, captions); } @@ -652,6 +689,9 @@ function (Sjson, AsyncProcess) { */ updatePlayTime: function (time) { var state = this.state, + startTime, + endTime, + params, newIndex; if (this.loaded) { @@ -660,7 +700,11 @@ function (Sjson, AsyncProcess) { } time = Math.round(time * 1000 + 100); - newIndex = this.sjson.search(time); + var times = this.getStartEndTimes(); + // if start and end times are defined, limit search. + // else, use the entire list of video captions + params = [time].concat(times); + newIndex = this.sjson.search.apply(this.sjson, params); if ( typeof newIndex !== 'undefined' && diff --git a/common/test/acceptance/tests/video/test_studio_video_module.py b/common/test/acceptance/tests/video/test_studio_video_module.py index 707a9ad726..48266f1cce 100644 --- a/common/test/acceptance/tests/video/test_studio_video_module.py +++ b/common/test/acceptance/tests/video/test_studio_video_module.py @@ -320,5 +320,3 @@ class CMSVideoTest(CMSVideoBaseTest): self.save_unit_settings() self.video.click_player_button('play') - - self.assertTrue(self.video.is_slider_range_visible) diff --git a/common/test/acceptance/tests/video/test_video_times.py b/common/test/acceptance/tests/video/test_video_times.py index 26c949ff72..4c6b76bb32 100644 --- a/common/test/acceptance/tests/video/test_video_times.py +++ b/common/test/acceptance/tests/video/test_video_times.py @@ -160,28 +160,3 @@ class VideoTimesTest(VideoBaseTest): self.video.wait_for_state('pause') self.assertIn(self.video.position, ('0:35', '0:36')) - - @flaky # TODO fix this, see TNL-1619 - def test_video_finish_time_with_seek(self): - """ - Scenario: Finish Time works for Youtube video. - Given it has a video in "Youtube" mode with end-time at 1:00, the video starts playing from 2:15 - And I seek video to "2:15" position - And I click video button "play" - And I wait until video stop playing - Then I see video slider at "2:20" position - """ - data = {'end_time': '00:01:00'} - self.metadata = self.metadata_for_mode('youtube', additional_data=data) - - # go to video - self.navigate_to_video() - - self.video.seek('2:15') - - self.video.click_player_button('play') - - # wait until video stop playing - self.video.wait_for_state('finished') - - self.assertEqual(self.video.position, '2:20')