diff --git a/CHANGELOG.rst b/CHANGELOG.rst index d41d47d750..fcce77bf4c 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Blades: Video player persist speed preferences between videos. BLD-237. + Blades: Change the download video field to a dropdown that will allow students to download the first source listed in the alternate sources. BLD-364. diff --git a/common/lib/xmodule/xmodule/js/fixtures/video.html b/common/lib/xmodule/xmodule/js/fixtures/video.html index a28d10422a..a0bf54d363 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/video.html +++ b/common/lib/xmodule/xmodule/js/fixtures/video.html @@ -4,8 +4,10 @@
0 and <= end time. - * - * An invalid start time will be reset to 0. An invalid end time will be - * set to `null`. It the task for the appropriate player API to figure out - * if start time and/or end time are greater than the length of the video. - */ - function checkStartEndTimes() { - this.config.startTime = parseInt(this.config.startTime, 10); - if (!isFinite(this.config.startTime) || this.config.startTime < 0) { - this.config.startTime = 0; - } - - this.config.endTime = parseInt(this.config.endTime, 10); - if ( - !isFinite(this.config.endTime) || - this.config.endTime <= this.config.startTime - ) { - this.config.endTime = null; - } + return __dfd__.promise(); } // function parseYoutubeStreams(state, youtubeStreams) @@ -595,22 +624,32 @@ function (VideoPlayer) { this.speeds = ($.map(this.videos, function (url, speed) { return speed; })).sort(); - - this.setSpeed($.cookie('video_speed')); } - function setSpeed(newSpeed, updateCookie) { - if (_.indexOf(this.speeds, newSpeed) !== -1) { + function setSpeed(newSpeed, updateStorage) { + // Possible speeds for each player type. + // flash = [0.75, 1, 1.25, 1.5] + // html5 = [0.75, 1, 1.25, 1.5] + // youtube html5 = [0.25, 0.5, 1, 1.5, 2] + var map = { + '0.25': '0.75', + '0.50': '0.75', + '0.75': '0.50', + '1.25': '1.50', + '2.0': '1.50' + }, + useSession = true; + + if (_.contains(this.speeds, newSpeed)) { this.speed = newSpeed; } else { - this.speed = '1.0'; + newSpeed = map[newSpeed]; + this.speed = _.contains(this.speeds, newSpeed) ? newSpeed : '1.0'; } - if (updateCookie) { - $.cookie('video_speed', this.speed, { - expires: 3650, - path: '/' - }); + if (updateStorage) { + this.storage.setItem('video_speed_' + this.id, this.speed, useSession); + this.storage.setItem('general_speed', this.speed, useSession); } } @@ -648,7 +687,11 @@ function (VideoPlayer) { } function getDuration() { - return this.metadata[this.youtubeId()].duration; + try { + return this.metadata[this.youtubeId()].duration; + } catch (err) { + return this.metadata[this.youtubeId('1.0')].duration; + } } /* @@ -662,8 +705,9 @@ function (VideoPlayer) { * * state.videoPlayer.pause({'param1': 10}); */ - function trigger(objChain, extraParameters) { - var i, tmpObj, chain; + function trigger(objChain) { + var extraParameters = Array.prototype.slice.call(arguments, 1), + i, tmpObj, chain; // Remember that 'this' is the 'state' object. tmpObj = this; @@ -685,7 +729,7 @@ function (VideoPlayer) { } } - tmpObj(extraParameters); + tmpObj.apply(this, extraParameters); return true; } 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 2fc434539a..cd47840646 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 @@ -324,7 +324,7 @@ function (HTML5Video, Resizer) { } } - function onSpeedChange(newSpeed, updateCookie) { + function onSpeedChange(newSpeed) { var time = this.videoPlayer.currentTime, methodName, youtubeId; @@ -347,7 +347,7 @@ function (HTML5Video, Resizer) { } ); - this.setSpeed(newSpeed, updateCookie); + this.setSpeed(newSpeed, true); if ( this.currentPlayerMode === 'html5' && @@ -376,6 +376,15 @@ function (HTML5Video, Resizer) { } this.el.trigger('speedchange', arguments); + + $.ajax({ + url: this.config.saveStateUrl, + type: 'POST', + dataType: 'json', + data: { + speed: newSpeed + }, + }); } // Every 200 ms, if the video is playing, we call the function update, via @@ -434,7 +443,7 @@ function (HTML5Video, Resizer) { end: true }); - if (this.config.show_captions) { + if (this.config.showCaptions) { this.trigger('videoCaption.pause', null); } @@ -466,7 +475,7 @@ function (HTML5Video, Resizer) { this.trigger('videoControl.pause', null); - if (this.config.show_captions) { + if (this.config.showCaptions) { this.trigger('videoCaption.pause', null); } @@ -495,7 +504,7 @@ function (HTML5Video, Resizer) { end: false }); - if (this.config.show_captions) { + if (this.config.showCaptions) { this.trigger('videoCaption.play', null); } @@ -579,7 +588,6 @@ function (HTML5Video, Resizer) { var key = value.toFixed(2).replace(/\.00$/, '.0'); _this.videos[key] = baseSpeedSubs; - _this.speeds.push(key); }); @@ -590,8 +598,8 @@ function (HTML5Video, Resizer) { currentSpeed: this.speed } ); - - this.setSpeed($.cookie('video_speed')); + this.setSpeed(this.speed); + this.trigger('videoSpeedControl.setSpeed', this.speed); } } 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 a89c459ca7..edea7bcdba 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 @@ -252,7 +252,7 @@ function () { } function captionURL() { - return '' + this.config.caption_asset_path + + return '' + this.config.captionAssetPath + this.youtubeId('1.0') + '.srt.sjson'; } @@ -356,7 +356,7 @@ function () { _this = this, autohideHtml5 = this.config.autohideHtml5; - this.elVideoWrapper.after(this.videoCaption.subtitlesEl); + this.container.after(this.videoCaption.subtitlesEl); this.el.find('.video-controls .secondary-controls') .append(this.videoCaption.hideSubtitlesEl); @@ -745,7 +745,7 @@ function () { 0.5 * this.videoControl.sliderEl.height() - 2 * paddingTop; } else { - return this.elVideoWrapper.height(); + return this.container.height(); } } diff --git a/common/lib/xmodule/xmodule/video_module.py b/common/lib/xmodule/xmodule/video_module.py index 2df3e11e27..c6a698c5f2 100644 --- a/common/lib/xmodule/xmodule/video_module.py +++ b/common/lib/xmodule/xmodule/video_module.py @@ -20,7 +20,6 @@ import datetime import copy from webob import Response -from django.http import Http404 from django.conf import settings from xmodule.x_module import XModule, module_attr @@ -31,7 +30,7 @@ from xmodule.contentstore.django import contentstore from xmodule.contentstore.content import StaticContent from xmodule.exceptions import NotFoundError from xblock.core import XBlock -from xblock.fields import Scope, String, Boolean, List, Integer, ScopeIds +from xblock.fields import Scope, String, Float, Boolean, List, Integer, ScopeIds from xmodule.fields import RelativeTime from xmodule.modulestore.inheritance import InheritanceKeyValueStore @@ -137,6 +136,15 @@ class VideoFields(object): scope=Scope.settings, default="" ) + speed = Float( + help="The last speed that was explicitly set by user for the video.", + scope=Scope.user_state, + ) + global_speed = Float( + help="Default speed in cases when speed wasn't explicitly for specific video", + scope=Scope.preferences, + default=1.0 + ) class VideoModule(VideoFields, XModule): @@ -178,10 +186,21 @@ class VideoModule(VideoFields, XModule): js_module_name = "Video" def handle_ajax(self, dispatch, data): - """This is not being called right now and we raise 404 error.""" + ACCEPTED_KEYS = ['speed'] + + if dispatch == 'save_user_state': + for key in data: + if hasattr(self, key) and key in ACCEPTED_KEYS: + setattr(self, key, json.loads(data[key])) + if key == 'speed': + self.global_speed = self.speed + + return json.dumps({'success': True}) + log.debug(u"GET {0}".format(data)) log.debug(u"DISPATCH {0}".format(dispatch)) - raise Http404() + + raise NotFoundError('Unexpected dispatch type') def get_html(self): track_url = None @@ -203,24 +222,26 @@ class VideoModule(VideoFields, XModule): track_url = self.runtime.handler_url(self, 'download_transcript') return self.system.render_template('video.html', { - 'youtube_streams': _create_youtube_string(self), - 'id': self.location.html_id(), - 'sub': self.sub, - 'sources': sources, - 'track': track_url, - 'display_name': self.display_name_with_default, + 'ajax_url': self.system.ajax_url + '/save_user_state', + 'autoplay': settings.FEATURES.get('AUTOPLAY_VIDEOS', False), # This won't work when we move to data that # isn't on the filesystem 'data_dir': getattr(self, 'data_dir', None), + 'display_name': self.display_name_with_default, 'caption_asset_path': caption_asset_path, - 'show_captions': json.dumps(self.show_captions), - 'start': self.start_time.total_seconds(), 'end': self.end_time.total_seconds(), - 'autoplay': settings.FEATURES.get('AUTOPLAY_VIDEOS', False), + 'id': self.location.html_id(), + 'show_captions': json.dumps(self.show_captions), + 'sources': sources, + 'speed': self.speed or self.global_speed, + 'start': self.start_time.total_seconds(), + 'sub': self.sub, + 'track': track_url, + 'youtube_streams': _create_youtube_string(self), # TODO: Later on the value 1500 should be taken from some global # configuration setting field. 'yt_test_timeout': 1500, - 'yt_test_url': settings.YOUTUBE_TEST_URL + 'yt_test_url': settings.YOUTUBE_TEST_URL, }) def get_transcript(self, subs_id): diff --git a/lms/djangoapps/courseware/features/video.feature b/lms/djangoapps/courseware/features/video.feature index 1ca82b02a1..39de72ba00 100644 --- a/lms/djangoapps/courseware/features/video.feature +++ b/lms/djangoapps/courseware/features/video.feature @@ -45,3 +45,24 @@ Feature: LMS.Video component Given the course has a Video component in HTML5_Unsupported_Video mode Then error message is shown And error message has correct text + + # 8 + Scenario: Video component stores speed correctly when each video is in separate sequence. + Given I am registered for the course "test_course" + And it has a video "A" in "Youtube" mode in position "1" of sequential + And a video "B" in "Youtube" mode in position "2" of sequential + And a video "C" in "Youtube" mode in position "3" of sequential + And I open the section with videos + And I select the "2.0" speed on video "A" + And I select the "0.50" speed on video "B" + When I open video "C" + Then video "C" should start playing at speed "0.50" + When I open video "A" + Then video "A" should start playing at speed "2.0" + And I reload the page + When I open video "A" + Then video "A" should start playing at speed "2.0" + When I open video "B" + Then video "B" should start playing at speed "0.50" + When I open video "C" + Then video "C" should start playing at speed "0.50" diff --git a/lms/djangoapps/courseware/features/video.py b/lms/djangoapps/courseware/features/video.py index 2f228853ab..c1c1437f34 100644 --- a/lms/djangoapps/courseware/features/video.py +++ b/lms/djangoapps/courseware/features/video.py @@ -15,6 +15,9 @@ HTML5_SOURCES_INCORRECT = [ 'https://s3.amazonaws.com/edx-course-videos/edx-intro/edX-FA12-cware-1_100.mp99' ] +coursenum = 'test_course' +sequence = {} + @step('when I view the (.*) it does not have autoplay enabled$') def does_not_autoplay(_step, video_type): assert(world.css_find('.%s' % video_type)[0]['data-autoplay'] == 'False') @@ -22,21 +25,48 @@ def does_not_autoplay(_step, video_type): @step('the course has a Video component in (.*) mode$') def view_video(_step, player_mode): - coursenum = 'test_course' - i_am_registered_for_the_course(step, coursenum) + + i_am_registered_for_the_course(_step, coursenum) # Make sure we have a video add_video_to_course(coursenum, player_mode.lower()) visit_scenario_item('SECTION') -def add_video_to_course(course, player_mode): +@step('a video "([^"]*)" in "([^"]*)" mode in position "([^"]*)" of sequential$') +def add_video(_step, player_id, player_mode, position): + sequence[player_id] = position + add_video_to_course(coursenum, player_mode.lower(), display_name=player_id) + + +@step('I open the section with videos$') +def visit_video_section(_step): + visit_scenario_item('SECTION') + + +@step('I select the "([^"]*)" speed on video "([^"]*)"$') +def change_video_speed(_step, speed, player_id): + _navigate_to_an_item_in_a_sequence(sequence[player_id]) + _change_video_speed(speed) + + +@step('I open video "([^"]*)"$') +def open_video(_step, player_id): + _navigate_to_an_item_in_a_sequence(sequence[player_id]) + + +@step('video "([^"]*)" should start playing at speed "([^"]*)"$') +def check_video_speed(_step, player_id, speed): + speed_css = '.speeds p.active' + assert world.css_has_text(speed_css, '{0}x'.format(speed)) + +def add_video_to_course(course, player_mode, display_name='Video'): category = 'video' kwargs = { 'parent_location': section_location(course), 'category': category, - 'display_name': 'Video' + 'display_name': display_name } if player_mode == 'html5': @@ -112,3 +142,12 @@ def error_message_has_correct_text(_step): assert world.css_has_text(selector, text) +def _navigate_to_an_item_in_a_sequence(number): + sequence_css = 'a[data-element="{0}"]'.format(number) + world.css_click(sequence_css) + + +def _change_video_speed(speed): + world.browser.execute_script("$('.speeds').addClass('open')") + speed_css = 'li[data-speed="{0}"] a'.format(speed) + world.css_click(speed_css) diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index a642855d10..21f9835c5c 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -19,6 +19,7 @@ from xmodule.exceptions import NotFoundError class TestVideo(BaseTestXmodule): """Integration tests: web client + mongo.""" + CATEGORY = "video" DATA = SOURCE_XML METADATA = {} @@ -57,6 +58,7 @@ class TestVideoYouTube(TestVideo): } expected_context = { + 'ajax_url': self.item_descriptor.xmodule_runtime.ajax_url + '/save_user_state', 'data_dir': getattr(self, 'data_dir', None), 'caption_asset_path': '/static/subs/', 'show_captions': 'true', @@ -64,6 +66,7 @@ class TestVideoYouTube(TestVideo): 'end': 3610.0, 'id': self.item_module.location.html_id(), 'sources': sources, + 'speed': 1.0, 'start': 3603.0, 'sub': u'a_sub_file.srt.sjson', 'track': None, @@ -75,7 +78,7 @@ class TestVideoYouTube(TestVideo): self.assertEqual( context, - self.item_module.xmodule_runtime.render_template('video.html', expected_context) + self.item_module.xmodule_runtime.render_template('video.html', expected_context), ) @@ -93,9 +96,10 @@ class TestVideoNonYouTube(TestVideo): """ MODEL_DATA = { - 'data': DATA + 'data': DATA, } METADATA = {} + def test_video_constructor(self): """Make sure that if the 'youtube' attribute is omitted in XML, then the template generates an empty string for the YouTube streams. @@ -107,8 +111,8 @@ class TestVideoNonYouTube(TestVideo): } context = self.item_module.render('student_view').content - expected_context = { + 'ajax_url': self.item_descriptor.xmodule_runtime.ajax_url + '/save_user_state', 'data_dir': getattr(self, 'data_dir', None), 'caption_asset_path': '/static/subs/', 'show_captions': 'true', @@ -116,6 +120,7 @@ class TestVideoNonYouTube(TestVideo): 'end': 3610.0, 'id': self.item_module.location.html_id(), 'sources': sources, + 'speed': 1.0, 'start': 3603.0, 'sub': u'a_sub_file.srt.sjson', 'track': None, @@ -127,7 +132,7 @@ class TestVideoNonYouTube(TestVideo): self.assertEqual( context, - self.item_module.xmodule_runtime.render_template('video.html', expected_context) + self.item_module.xmodule_runtime.render_template('video.html', expected_context), ) @@ -137,6 +142,7 @@ class TestGetHtmlMethod(BaseTestXmodule): ''' CATEGORY = "video" DATA = SOURCE_XML + maxDiff = None METADATA = {} def setUp(self): @@ -195,7 +201,8 @@ class TestGetHtmlMethod(BaseTestXmodule): }, 'start': 3603.0, 'sub': u'a_sub_file.srt.sjson', - 'track': '', + 'speed': 1.0, + 'track': None, 'youtube_streams': '1.00:OEoXaMPEzfM', 'autoplay': settings.FEATURES.get('AUTOPLAY_VIDEOS', True), 'yt_test_timeout': 1500, @@ -212,16 +219,18 @@ class TestGetHtmlMethod(BaseTestXmodule): self.initialize_module(data=DATA) track_url = self.item_descriptor.xmodule_runtime.handler_url(self.item_module, 'download_transcript') + context = self.item_module.render('student_view').content + expected_context.update({ + 'ajax_url': self.item_descriptor.xmodule_runtime.ajax_url + '/save_user_state', 'track': track_url if data['expected_track_url'] == u'a_sub_file.srt.sjson' else data['expected_track_url'], 'sub': data['sub'], 'id': self.item_module.location.html_id(), }) - context = self.item_module.render('student_view').content self.assertEqual( context, - self.item_module.xmodule_runtime.render_template('video.html', expected_context) + self.item_module.xmodule_runtime.render_template('video.html', expected_context), ) def test_get_html_source(self): @@ -293,6 +302,7 @@ class TestGetHtmlMethod(BaseTestXmodule): 'end': 3610.0, 'id': None, 'sources': None, + 'speed': 1.0, 'start': 3603.0, 'sub': u'a_sub_file.srt.sjson', 'track': None, @@ -309,14 +319,14 @@ class TestGetHtmlMethod(BaseTestXmodule): sources=data['sources'] ) self.initialize_module(data=DATA) + context = self.item_module.render('student_view').content expected_context.update({ + 'ajax_url': self.item_descriptor.xmodule_runtime.ajax_url + '/save_user_state', 'sources': data['result'], 'id': self.item_module.location.html_id(), }) - context = self.item_module.render('student_view').content - self.assertEqual( context, self.item_module.xmodule_runtime.render_template('video.html', expected_context) diff --git a/lms/djangoapps/courseware/tests/test_video_xml.py b/lms/djangoapps/courseware/tests/test_video_xml.py index 0e99928716..81573f6c10 100644 --- a/lms/djangoapps/courseware/tests/test_video_xml.py +++ b/lms/djangoapps/courseware/tests/test_video_xml.py @@ -15,11 +15,7 @@ common/lib/xmodule/xmodule/modulestore/tests/factories.py to create the course, section, subsection, unit, etc. """ -import unittest - -from django.conf import settings - -from xmodule.video_module import VideoDescriptor, _create_youtube_string +from xmodule.video_module import VideoDescriptor from xmodule.modulestore import Location from xmodule.tests import get_test_system, LogicTest, get_test_descriptor_system from xblock.field_data import DictFieldData @@ -63,40 +59,6 @@ class VideoFactory(object): return descriptor -class VideoModuleUnitTest(unittest.TestCase): - """Unit tests for Video Xmodule.""" - def test_video_get_html(self): - """Make sure that all parameters extracted correclty from xml""" - module = VideoFactory.create() - sources = { - 'main': 'example.mp4', - 'mp4': 'example.mp4', - 'webm': 'example.webm', - } - - expected_context = { - 'caption_asset_path': '/static/subs/', - 'sub': 'a_sub_file.srt.sjson', - 'data_dir': getattr(self, 'data_dir', None), - 'display_name': 'A Name', - 'end': 3610.0, - 'start': 3603.0, - 'id': module.location.html_id(), - 'show_captions': 'true', - 'sources': sources, - 'youtube_streams': _create_youtube_string(module), - 'track': None, - 'autoplay': settings.FEATURES.get('AUTOPLAY_VIDEOS', False), - 'yt_test_timeout': 1500, - 'yt_test_url': 'https://gdata.youtube.com/feeds/api/videos/' - } - - self.assertEqual( - module.render('student_view').content, - module.runtime.render_template('video.html', expected_context) - ) - - class VideoModuleLogicTest(LogicTest): """Tests for logic of Video Xmodule.""" diff --git a/lms/templates/video.html b/lms/templates/video.html index 4656e1f3b6..58c635c902 100644 --- a/lms/templates/video.html +++ b/lms/templates/video.html @@ -17,8 +17,10 @@ ${'data-webm-source="{}"'.format(sources.get('webm')) if sources.get('webm') else ''} ${'data-ogg-source="{}"'.format(sources.get('ogv')) if sources.get('ogv') else ''} + data-save-state-url="${ajax_url}" data-caption-data-dir="${data_dir}" data-show-captions="${show_captions}" + data-speed="${speed}" data-start="${start}" data-end="${end}" data-caption-asset-path="${caption_asset_path}" @@ -108,5 +110,3 @@ % endif
- -