From 2401fa2e655ae8981f5cd4fc9cbf1fc876f1fd5f Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Thu, 8 Aug 2013 15:28:49 -0400 Subject: [PATCH 01/30] Update the XBlock version to fix save bugs --- requirements/edx/github.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/github.txt b/requirements/edx/github.txt index a75efe8e9b..9eea9e1eff 100644 --- a/requirements/edx/github.txt +++ b/requirements/edx/github.txt @@ -8,6 +8,6 @@ -e git+https://github.com/eventbrite/zendesk.git@d53fe0e81b623f084e91776bcf6369f8b7b63879#egg=zendesk # Our libraries: --e git+https://github.com/edx/XBlock.git@f8c10cb0d16122ba66f7e94a32ec3774c1dee13e#egg=XBlock +-e git+https://github.com/edx/XBlock.git@446668fddc75b78512eef4e9425cbc9a3327606f#egg=XBlock -e git+https://github.com/edx/codejail.git@0a1b468#egg=codejail -e git+https://github.com/edx/diff-cover.git@v0.2.0#egg=diff_cover From fcaf3e6329f464e4a5f82168b65ee4e3b289eead Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 5 Aug 2013 23:08:35 -0400 Subject: [PATCH 02/30] give some debug message regarding why export might fail --- cms/djangoapps/contentstore/views/assets.py | 31 +++++++++++++++++---- cms/templates/export.html | 19 +++++++++++++ common/lib/xmodule/xmodule/exceptions.py | 10 +++++++ common/lib/xmodule/xmodule/raw_module.py | 5 ++-- 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 1c22114d76..252f36e392 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -3,6 +3,7 @@ import json import os import tarfile import shutil +import cgi from tempfile import mkdtemp from path import path @@ -27,7 +28,7 @@ from xmodule.modulestore import Location from xmodule.contentstore.content import StaticContent from xmodule.util.date_utils import get_default_time_display from xmodule.modulestore import InvalidLocationError -from xmodule.exceptions import NotFoundError +from xmodule.exceptions import NotFoundError, SerializationError from .access import get_location_and_verify_access from util.json_request import JsonResponse @@ -336,16 +337,34 @@ def generate_export_course(request, org, course, name): the course """ location = get_location_and_verify_access(request, org, course, name) - + course_module = modulestore().get_item(location) loc = Location(location) export_file = NamedTemporaryFile(prefix=name + '.', suffix=".tar.gz") root_dir = path(mkdtemp()) - # export out to a tempdir - logging.debug('root = {0}'.format(root_dir)) - - export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name, modulestore()) + try: + export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name, modulestore()) + except SerializationError, e: + failed_item = modulestore().get_item(e.location) + parent_locs = modulestore().get_parent_locations(failed_item.location, None) + if len(parent_locs) > 0: + parent = modulestore().get_item(parent_locs[0]) + parent_info = "Parent Display Name: {0}
Parent Identifier: {1}".format(parent.display_name, parent.location.name) + else: + parent_info = '' + return render_to_response('export.html', { + 'context_course': course_module, + 'successful_import_redirect_url': '', + 'err_msg': "A courseware module has failed to convert to XML. Details:
Module Type: {0}
Display Name: {1}
Identifier: {2}
{3}". + format(failed_item.location.category, failed_item.display_name, failed_item.location.name, parent_info) + }) + except Exception, e: + return render_to_response('export.html', { + 'context_course': course_module, + 'successful_import_redirect_url': '', + 'err_msg': str(e) + }) logging.debug('tar file being generated at {0}'.format(export_file.name)) tar_file = tarfile.open(name=export_file.name, mode='w:gz') diff --git a/cms/templates/export.html b/cms/templates/export.html index 593cf3dd6e..0cc67a7b5a 100644 --- a/cms/templates/export.html +++ b/cms/templates/export.html @@ -6,6 +6,24 @@ <%block name="title">${_("Course Export")} <%block name="bodyclass">is-signedin course tools export +<%block name="jsextra"> + % if err_msg: + + %endif + + <%block name="content">
@@ -18,6 +36,7 @@
+

${_("About Exporting Courses")}

diff --git a/common/lib/xmodule/xmodule/exceptions.py b/common/lib/xmodule/xmodule/exceptions.py index d0a8e76557..48c083cbf1 100644 --- a/common/lib/xmodule/xmodule/exceptions.py +++ b/common/lib/xmodule/xmodule/exceptions.py @@ -13,6 +13,7 @@ class ProcessingError(Exception): ''' pass + class InvalidVersionError(Exception): """ Tried to save an item with a location that a store cannot support (e.g., draft version @@ -21,3 +22,12 @@ class InvalidVersionError(Exception): def __init__(self, location): super(InvalidVersionError, self).__init__() self.location = location + + +class SerializationError(Exception): + """ + Thrown when a module cannot be exported to XML + """ + def __init__(self, location, msg): + super(SerializationError, self).__init__(msg) + self.location = location diff --git a/common/lib/xmodule/xmodule/raw_module.py b/common/lib/xmodule/xmodule/raw_module.py index b972d7c8eb..4c6ddb5b51 100644 --- a/common/lib/xmodule/xmodule/raw_module.py +++ b/common/lib/xmodule/xmodule/raw_module.py @@ -4,6 +4,7 @@ from xmodule.xml_module import XmlDescriptor import logging import sys from xblock.core import String, Scope +from exceptions import SerializationError log = logging.getLogger(__name__) @@ -27,11 +28,11 @@ class RawDescriptor(XmlDescriptor, XMLEditingDescriptor): # re-raise lines = self.data.split('\n') line, offset = err.position - msg = ("Unable to create xml for problem {loc}. " + msg = ("Unable to create xml for module {loc}. " "Context: '{context}'".format( context=lines[line - 1][offset - 40:offset + 40], loc=self.location)) - raise Exception, msg, sys.exc_info()[2] + raise SerializationError(self.location, msg) class EmptyDataRawDescriptor(XmlDescriptor, XMLEditingDescriptor): From 69c34a65b14727375d6f75c1d7ef190dc69d695c Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 7 Aug 2013 22:09:08 -0400 Subject: [PATCH 03/30] switch the notification to be a prompt and allow for the user to go to the edit unit page which contains the module in error. Otherwise, present the raw exception message and allow user to go to the course outline page. --- cms/djangoapps/contentstore/views/assets.py | 34 ++++++++++--- cms/templates/export.html | 56 +++++++++++++++++---- 2 files changed, 73 insertions(+), 17 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 252f36e392..c2da87d0aa 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -346,24 +346,42 @@ def generate_export_course(request, org, course, name): try: export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name, modulestore()) except SerializationError, e: - failed_item = modulestore().get_item(e.location) - parent_locs = modulestore().get_parent_locations(failed_item.location, None) + failed_item = modulestore().get_instance(course_module.location.course_id, e.location) + parent_locs = modulestore().get_parent_locations(failed_item.location, course_module.location.course_id) + unit = None if len(parent_locs) > 0: parent = modulestore().get_item(parent_locs[0]) - parent_info = "Parent Display Name: {0}
Parent Identifier: {1}".format(parent.display_name, parent.location.name) - else: - parent_info = '' + if parent.location.category == 'vertical': + unit = parent + return render_to_response('export.html', { 'context_course': course_module, 'successful_import_redirect_url': '', - 'err_msg': "A courseware module has failed to convert to XML. Details:
Module Type: {0}
Display Name: {1}
Identifier: {2}
{3}". - format(failed_item.location.category, failed_item.display_name, failed_item.location.name, parent_info) + 'in_err': True, + 'raw_err_msg': str(e), + 'failed_module': failed_item, + 'unit': unit, + 'edit_unit_url': reverse('edit_unit', kwargs={ + 'location': parent.location + }), + 'course_home_url': reverse('course_index', kwargs={ + 'org': org, + 'course': course, + 'name': name + }) }) except Exception, e: return render_to_response('export.html', { 'context_course': course_module, 'successful_import_redirect_url': '', - 'err_msg': str(e) + 'in_err': True, + 'unit': None, + 'raw_err_msg': str(e), + 'course_home_url': reverse('course_index', kwargs={ + 'org': org, + 'course': course, + 'name': name + }) }) logging.debug('tar file being generated at {0}'.format(export_file.name)) diff --git a/cms/templates/export.html b/cms/templates/export.html index 0cc67a7b5a..3b5af8bf87 100644 --- a/cms/templates/export.html +++ b/cms/templates/export.html @@ -7,17 +7,55 @@ <%block name="bodyclass">is-signedin course tools export <%block name="jsextra"> - % if err_msg: + % if in_err: From 40545441264a26deb007110ab99ba55034a7cdf0 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 7 Aug 2013 22:26:00 -0400 Subject: [PATCH 04/30] handle exceptions inside the outer exception handling --- cms/djangoapps/contentstore/views/assets.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index c2da87d0aa..bbaea74ce5 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -346,13 +346,20 @@ def generate_export_course(request, org, course, name): try: export_to_xml(modulestore('direct'), contentstore(), loc, root_dir, name, modulestore()) except SerializationError, e: - failed_item = modulestore().get_instance(course_module.location.course_id, e.location) - parent_locs = modulestore().get_parent_locations(failed_item.location, course_module.location.course_id) unit = None - if len(parent_locs) > 0: - parent = modulestore().get_item(parent_locs[0]) - if parent.location.category == 'vertical': - unit = parent + failed_item = None + parent = None + try: + failed_item = modulestore().get_instance(course_module.location.course_id, e.location) + parent_locs = modulestore().get_parent_locations(failed_item.location, course_module.location.course_id) + + if len(parent_locs) > 0: + parent = modulestore().get_item(parent_locs[0]) + if parent.location.category == 'vertical': + unit = parent + except: + # if we have a nested exception, then we'll show the more generic error message + pass return render_to_response('export.html', { 'context_course': course_module, @@ -363,7 +370,7 @@ def generate_export_course(request, org, course, name): 'unit': unit, 'edit_unit_url': reverse('edit_unit', kwargs={ 'location': parent.location - }), + }) if parent else '', 'course_home_url': reverse('course_index', kwargs={ 'org': org, 'course': course, From 487ae964e4e011027adf9c1d79bbbdb2f95e2ded Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 8 Aug 2013 15:58:23 -0400 Subject: [PATCH 05/30] implement PR feedback --- cms/templates/export.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cms/templates/export.html b/cms/templates/export.html index 3b5af8bf87..dd494a532b 100644 --- a/cms/templates/export.html +++ b/cms/templates/export.html @@ -12,7 +12,7 @@ $(document).ready(function() { %if unit: - dialog = new CMS.Views.Prompt({ + var dialog = new CMS.Views.Prompt({ title: gettext('There has been an error with your export.'), message: gettext("There has been a failure to export to XML at least one component. It is recommended that you go to the edit page and repair the error before attempting another export. Please check that all components on the page are valid and do not display any error messages."), intent: "error", @@ -25,7 +25,7 @@ } }, secondary: { - text: gettext('cancel'), + text: gettext('Cancel'), click: function(view) { view.hide(); } @@ -33,9 +33,9 @@ } }); % else: - var msg = gettext("

There has been a failure to export your course to XML. Unfortunately, we do not have specific enough information to assist you in identifying the failed component. It is recommended that you inspect your courseware to identify any components in error and try again.

The raw error message is:

"); + var msg = "

" + gettext("There has been a failure to export your course to XML. Unfortunately, we do not have specific enough information to assist you in identifying the failed component. It is recommended that you inspect your courseware to identify any components in error and try again.") + "

" + gettext("The raw error message is:") + "

"; msg = msg + "${raw_err_msg}"; - dialog = new CMS.Views.Prompt({ + var dialog = new CMS.Views.Prompt({ title: gettext('There has been an error with your export.'), message: msg, intent: "error", @@ -48,7 +48,7 @@ } }, secondary: { - text: gettext('cancel'), + text: gettext('Cancel'), click: function(view) { view.hide(); } From 32d92d97e63b16ebb42a330ffe204270a52c89f1 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 8 Aug 2013 20:36:39 -0400 Subject: [PATCH 06/30] get_item -> get_instance --- cms/djangoapps/contentstore/views/assets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index bbaea74ce5..2334c61b4c 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -337,7 +337,7 @@ def generate_export_course(request, org, course, name): the course """ location = get_location_and_verify_access(request, org, course, name) - course_module = modulestore().get_item(location) + course_module = modulestore().get_instance(location.course_id, location) loc = Location(location) export_file = NamedTemporaryFile(prefix=name + '.', suffix=".tar.gz") From 55bfe46e79c944e8a25c8e9c2c33b7173513ae55 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 7 Aug 2013 12:13:27 -0400 Subject: [PATCH 07/30] need to filter out non-own metadata. Also we need to clone draft content --- .../xmodule/modulestore/store_utilities.py | 69 ++++++++++--------- 1 file changed, 38 insertions(+), 31 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index 3c29627d84..e941cf5224 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -1,10 +1,45 @@ from xmodule.contentstore.content import StaticContent from xmodule.modulestore import Location from xmodule.modulestore.mongo import MongoModuleStore +from xmodule.modulestore.inheritance import own_metadata import logging +def _clone_modules(modulestore, modules, dest_location): + for module in modules: + original_loc = Location(module.location) + + if original_loc.category != 'course': + module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org, + course=dest_location.course) + else: + # on the course module we also have to update the module name + module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org, + course=dest_location.course, name=dest_location.name) + + print "Cloning module {0} to {1}....".format(original_loc, module.location) + + modulestore.update_item(module.location, module._model_data._kvs._data) + + # repoint children + if module.has_children: + new_children = [] + for child_loc_url in module.children: + child_loc = Location(child_loc_url) + child_loc = child_loc._replace( + tag=dest_location.tag, + org=dest_location.org, + course=dest_location.course + ) + new_children.append(child_loc.url()) + + modulestore.update_children(module.location, new_children) + + # save metadata + modulestore.update_metadata(module.location, own_metadata(module)) + + def clone_course(modulestore, contentstore, source_location, dest_location, delete_original=False): # first check to see if the modulestore is Mongo backed if not isinstance(modulestore, MongoModuleStore): @@ -37,38 +72,10 @@ def clone_course(modulestore, contentstore, source_location, dest_location, dele # Get all modules under this namespace which is (tag, org, course) tuple modules = modulestore.get_items([source_location.tag, source_location.org, source_location.course, None, None, None]) + _clone_modules(modulestore, modules, dest_location) - for module in modules: - original_loc = Location(module.location) - - if original_loc.category != 'course': - module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org, - course=dest_location.course) - else: - # on the course module we also have to update the module name - module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org, - course=dest_location.course, name=dest_location.name) - - print "Cloning module {0} to {1}....".format(original_loc, module.location) - - modulestore.update_item(module.location, module._model_data._kvs._data) - - # repoint children - if module.has_children: - new_children = [] - for child_loc_url in module.children: - child_loc = Location(child_loc_url) - child_loc = child_loc._replace( - tag=dest_location.tag, - org=dest_location.org, - course=dest_location.course - ) - new_children.append(child_loc.url()) - - modulestore.update_children(module.location, new_children) - - # save metadata - modulestore.update_metadata(module.location, module._model_data._kvs._metadata) + modules = modulestore.get_items([source_location.tag, source_location.org, source_location.course, None, None, 'draft']) + _clone_modules(modulestore, modules, dest_location) # now iterate through all of the assets and clone them # first the thumbnails From 0e09ee61d788ab222010552d1ca18ad2831fe38d Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 7 Aug 2013 14:23:54 -0400 Subject: [PATCH 08/30] update some unit tests on clone_course --- .../contentstore/tests/test_contentstore.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 838af2cafa..2ace5db89d 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -633,15 +633,21 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # now loop through all the units in the course and verify that the clone can render them, which # means the objects are at least present - items = module_store.get_items(Location(['i4x', 'edX', 'toy', 'poll_question', None])) + items = module_store.get_items(Location(['i4x', 'edX', 'toy', None, None])) self.assertGreater(len(items), 0) - clone_items = module_store.get_items(Location(['i4x', 'MITx', '999', 'poll_question', None])) + clone_items = module_store.get_items(Location(['i4x', 'MITx', '999', None, None])) self.assertGreater(len(clone_items), 0) + self.assertEqual(len(items), len(clone_items)) + for descriptor in items: new_loc = descriptor.location.replace(org='MITx', course='999') print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) - resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) - self.assertEqual(resp.status_code, 200) + lookup_item = module_store.get_item(new_loc) + + # we want to assert equality between the objects, but we know the locations + # differ, so just make them equal for testing purposes + descriptor.location = new_loc + self.assertEqual(descriptor, lookup_item) def test_illegal_draft_crud_ops(self): draft_store = modulestore('draft') From 7b4388ba8ce3f0addaecd514fb79c53fb886a77d Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Wed, 7 Aug 2013 23:43:09 -0400 Subject: [PATCH 09/30] add more testing. Assert that metadata is properly cloned and that draft content is cloned as well --- .../contentstore/tests/test_contentstore.py | 59 ++++++++++++++++--- 1 file changed, 52 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 2ace5db89d..bd35201d68 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -617,8 +617,26 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): } module_store = modulestore('direct') + draft_store = modulestore('draft') import_from_xml(module_store, 'common/test/data/', ['toy']) + source_course_id = 'edX/toy/2012_Fall' + dest_course_id = 'MITx/999/2013_Spring' + source_location = CourseDescriptor.id_to_location(source_course_id) + dest_location = CourseDescriptor.id_to_location(dest_course_id) + + # get a vertical (and components in it) to put into 'draft' + # this is to assert that draft content is also cloned over + vertical = module_store.get_item(Location(['i4x', 'edX', 'toy', + 'vertical', 'vertical_test', None]), depth=1) + + draft_store.convert_to_draft(vertical.location) + for child in vertical.get_children(): + draft_store.convert_to_draft(child.location) + + items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])) + self.assertGreater(len(items), 0) + resp = self.client.post(reverse('create_new_course'), course_data) self.assertEqual(resp.status_code, 200) data = parse_json(resp) @@ -626,28 +644,55 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): content_store = contentstore() - source_location = CourseDescriptor.id_to_location('edX/toy/2012_Fall') - dest_location = CourseDescriptor.id_to_location('MITx/999/2013_Spring') - clone_course(module_store, content_store, source_location, dest_location) + # first assert that all draft content got cloned as well + items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])) + self.assertGreater(len(items), 0) + clone_items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])) + self.assertGreater(len(clone_items), 0) + self.assertEqual(len(items), len(clone_items)) + # now loop through all the units in the course and verify that the clone can render them, which # means the objects are at least present items = module_store.get_items(Location(['i4x', 'edX', 'toy', None, None])) self.assertGreater(len(items), 0) clone_items = module_store.get_items(Location(['i4x', 'MITx', '999', None, None])) self.assertGreater(len(clone_items), 0) - self.assertEqual(len(items), len(clone_items)) + #self.assertEqual(len(items), len(clone_items)) for descriptor in items: - new_loc = descriptor.location.replace(org='MITx', course='999') + source_item = module_store.get_instance(source_course_id, descriptor.location) + if descriptor.location.category == 'course': + new_loc = descriptor.location.replace(org='MITx', course='999', name='2013_Spring') + else: + new_loc = descriptor.location.replace(org='MITx', course='999') print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) lookup_item = module_store.get_item(new_loc) # we want to assert equality between the objects, but we know the locations # differ, so just make them equal for testing purposes - descriptor.location = new_loc - self.assertEqual(descriptor, lookup_item) + source_item.location = new_loc + if hasattr(source_item, 'data') and hasattr(lookup_item, 'data'): + self.assertEqual(source_item.data, lookup_item.data) + + # also make sure that metadata was cloned over and filtered with own_metadata, i.e. inherited + # values were not explicitly set + self.assertEqual(own_metadata(source_item), own_metadata(lookup_item)) + + # check that the children are as expected + self.assertEqual(source_item.has_children, lookup_item.has_children) + if source_item.has_children: + expected_children = [] + for child_loc_url in source_item.children: + child_loc = Location(child_loc_url) + child_loc = child_loc._replace( + tag=dest_location.tag, + org=dest_location.org, + course=dest_location.course + ) + expected_children.append(child_loc.url()) + self.assertEqual(expected_children, lookup_item.children) def test_illegal_draft_crud_ops(self): draft_store = modulestore('draft') From 481eac7fc8abd86254d758680d63ff1fa073c196 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Thu, 8 Aug 2013 20:51:18 -0400 Subject: [PATCH 10/30] address PR feedback --- .../contentstore/tests/test_contentstore.py | 15 +++++++-------- .../xmodule/modulestore/store_utilities.py | 11 ++++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index bd35201d68..23135964a9 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -627,8 +627,8 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # get a vertical (and components in it) to put into 'draft' # this is to assert that draft content is also cloned over - vertical = module_store.get_item(Location(['i4x', 'edX', 'toy', - 'vertical', 'vertical_test', None]), depth=1) + vertical = module_store.get_instance(source_course_id, Location([ + source_location.tag, source_location.org, source_location.course, 'vertical', 'vertical_test', None]), depth=1) draft_store.convert_to_draft(vertical.location) for child in vertical.get_children(): @@ -649,24 +649,23 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # first assert that all draft content got cloned as well items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])) self.assertGreater(len(items), 0) - clone_items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None, 'draft'])) + clone_items = module_store.get_items(Location([dest_location.tag, dest_location.org, dest_location.course, None, None, 'draft'])) self.assertGreater(len(clone_items), 0) self.assertEqual(len(items), len(clone_items)) # now loop through all the units in the course and verify that the clone can render them, which # means the objects are at least present - items = module_store.get_items(Location(['i4x', 'edX', 'toy', None, None])) + items = module_store.get_items(Location([source_location.tag, source_location.org, source_location.course, None, None])) self.assertGreater(len(items), 0) - clone_items = module_store.get_items(Location(['i4x', 'MITx', '999', None, None])) + clone_items = module_store.get_items(Location([dest_location.tag, dest_location.org, dest_location.course, None, None])) self.assertGreater(len(clone_items), 0) - #self.assertEqual(len(items), len(clone_items)) for descriptor in items: source_item = module_store.get_instance(source_course_id, descriptor.location) if descriptor.location.category == 'course': - new_loc = descriptor.location.replace(org='MITx', course='999', name='2013_Spring') + new_loc = descriptor.location.replace(org=dest_location.org, course=dest_location.course, name='2013_Spring') else: - new_loc = descriptor.location.replace(org='MITx', course='999') + new_loc = descriptor.location.replace(org=dest_location.org, course=dest_location.course) print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) lookup_item = module_store.get_item(new_loc) diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index e941cf5224..cfe0a0a6c5 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -11,16 +11,17 @@ def _clone_modules(modulestore, modules, dest_location): original_loc = Location(module.location) if original_loc.category != 'course': - module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org, - course=dest_location.course) + module.location = module.location._replace( + tag=dest_location.tag, org=dest_location.org, course=dest_location.course) else: # on the course module we also have to update the module name - module.location = module.location._replace(tag=dest_location.tag, org=dest_location.org, - course=dest_location.course, name=dest_location.name) + module.location = module.location._replace( + tag=dest_location.tag, org=dest_location.org, course=dest_location.course, name=dest_location.name) print "Cloning module {0} to {1}....".format(original_loc, module.location) - modulestore.update_item(module.location, module._model_data._kvs._data) + # NOTE: usage of the the internal module.xblock_kvs._data does not include any 'default' values for the fields + modulestore.update_item(module.location, module.xblock_kvs._data) # repoint children if module.has_children: From bc503c88c6e089a9242a24f8c09a2628ec99f8f2 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 6 Aug 2013 14:23:48 +0300 Subject: [PATCH 11/30] Making it possible to tab to captions. When a VideoAlpha is present on the page, if the user tries to tab through out the entire page, eventually he will land on the VideoAlpha controls. The tab index order has been set up so that all control receive focus eventually in the order from left to right. When a control receives focus, it is hilighted. The last tab focuses the captions dialog. When the captions dialog has focus, the Up and Down arrows can scroll it up and down. --- .../xmodule/css/videoalpha/display.scss | 23 ++-- lms/templates/videoalpha.html | 126 +++++++++--------- 2 files changed, 73 insertions(+), 76 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/videoalpha/display.scss b/common/lib/xmodule/xmodule/css/videoalpha/display.scss index 3b40809fa3..15ed6fb3b0 100644 --- a/common/lib/xmodule/xmodule/css/videoalpha/display.scss +++ b/common/lib/xmodule/xmodule/css/videoalpha/display.scss @@ -133,7 +133,6 @@ div.videoalpha { line-height: 46px; padding: 0 lh(.75); text-indent: -9999px; - @include transition(background-color 0.75s linear 0s, opacity 0.75s linear 0s); width: 14px; background: url('../images/vcr.png') 15px 15px no-repeat; outline: 0; @@ -150,7 +149,7 @@ div.videoalpha { &.play { background-position: 17px -114px; - &:hover { + &:hover, &:focus { background-color: #444; } } @@ -158,7 +157,7 @@ div.videoalpha { &.pause { background-position: 16px -50px; - &:hover { + &:hover, &:focus { background-color: #444; } } @@ -382,7 +381,7 @@ div.videoalpha { @include transition(none); width: 30px; - &:hover { + &:hover, &:active, &:focus { background-color: #444; color: #fff; text-decoration: none; @@ -403,7 +402,7 @@ div.videoalpha { @include transition(none); width: 30px; - &:hover { + &:hover, &:focus { background-color: #444; color: #fff; text-decoration: none; @@ -432,7 +431,7 @@ div.videoalpha { -webkit-font-smoothing: antialiased; width: 30px; - &:hover { + &:hover, &:focus { background-color: #444; color: #fff; text-decoration: none; @@ -442,9 +441,9 @@ div.videoalpha { opacity: 0.7; } - background-color: #444; - color: #fff; - text-decoration: none; + background-color: inherit; + color: #797979; + text-decoration: inherit; } } } @@ -513,12 +512,6 @@ div.videoalpha { z-index: 1; } - article.video-wrapper section.video-controls div.secondary-controls a.hide-subtitles { - background-color: inherit; - color: #797979; - text-decoration: inherit; - } - article.video-wrapper div.video-player-pre, article.video-wrapper div.video-player-post { height: 0px; } diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html index 682c299e10..e0e0a6c0d9 100644 --- a/lms/templates/videoalpha.html +++ b/lms/templates/videoalpha.html @@ -1,82 +1,86 @@ <%! from django.utils.translation import ugettext as _ %> % if display_name is not UNDEFINED and display_name is not None: -

${display_name}

+

${display_name}

% endif
-
- + +
-
- -
+
+
% if sources.get('main'): -
-

${_('Download video here.') % sources.get('main')}

-
+
+

${_('Download video here.') % sources.get('main')}

+
% endif % if track: -
-

${_('Download subtitles here.') % track}

-
+
+

${_('Download subtitles here.') % track}

+
% endif From b24ee155683f0c9d3cee68d505ac31ff9d5b4327 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 6 Aug 2013 15:11:04 +0300 Subject: [PATCH 12/30] Fixed styling for volume button. For some reason the "muted" icon was not showing. Styles have been corrected.Now on focus background color is also simplified for div .volume control. --- common/lib/xmodule/xmodule/css/videoalpha/display.scss | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/videoalpha/display.scss b/common/lib/xmodule/xmodule/css/videoalpha/display.scss index 15ed6fb3b0..0b94e1bd71 100644 --- a/common/lib/xmodule/xmodule/css/videoalpha/display.scss +++ b/common/lib/xmodule/xmodule/css/videoalpha/display.scss @@ -299,12 +299,15 @@ div.videoalpha { &.muted { &>a { - background: url('../images/mute.png') 10px center no-repeat; + background-image: url('../images/mute.png'); } } > a { - background: url('../images/volume.png') 10px center no-repeat; + background-image: url('../images/volume.png'); + background-position: 10px center; + background-repeat: no-repeat; + border-right: 1px solid #000; box-shadow: 1px 0 0 #555, inset 1px 0 0 #555; @include clearfix(); From d46542cba8a8184d74823989be2dd7bed2161453 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 6 Aug 2013 16:25:29 +0300 Subject: [PATCH 13/30] Removing JavaScript qTip tooltips. Because sometimes the tooltips generated by qTip get in the way, and the controls becomes not responsive, it was decided to use standard title attributes for tips. Also along the way, an error was discoevered in Jasmine tests. It was fixed, but that test is failing so it was marked specifically to be skipped when all VideoAlpha tests run. --- .../js/spec/videoalpha/video_caption_spec.js | 5 +- .../js/spec/videoalpha/video_player_spec.js | 14 ------ .../videoalpha/video_progress_slider_spec.js | 49 ------------------- .../js/src/videoalpha/01_initialize.js | 9 +--- .../js/src/videoalpha/04_video_control.js | 5 +- .../videoalpha/05_video_quality_control.js | 4 -- .../videoalpha/06_video_progress_slider.js | 28 ----------- 7 files changed, 4 insertions(+), 110 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js index 2c138a734c..1e6e5b8e11 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js @@ -368,13 +368,12 @@ expect(realHeight).toBeCloseTo(shouldBeHeight, 2); }); - it('when CC button is disabled ', function() { + xit('when CC button is disabled ', function() { var realHeight = parseInt($('.subtitles').css('maxHeight'), 10), videoWrapperHeight = $('.video-wrapper').height(), controlsHeight = videoControl.el.height(), progressSliderHeight = videoControl.sliderEl.height(), - shouldBeHeight = videoWrapperHeight - controlsHeight \ - - 0.5 * controlsHeight; + shouldBeHeight = videoWrapperHeight - controlsHeight - 0.5 * controlsHeight; state.captionsHidden = true; videoCaption.setSubtitlesHeight(); diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js index 467ecc75a2..c807936ecd 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js @@ -34,12 +34,6 @@ }); describe('constructor', function() { - beforeEach(function() { - $.fn.qtip.andCallFake(function() { - $(this).data('qtip', true); - }); - }); - describe('always', function() { beforeEach(function() { initialize(); @@ -170,10 +164,6 @@ initialize(); }); - it('does not add the tooltip to fullscreen button', function() { - expect($('.add-fullscreen')).not.toHaveData('qtip'); - }); - it('create video volume control', function() { expect(videoVolumeControl).toBeDefined(); expect(videoVolumeControl.el).toHaveClass('volume'); @@ -187,10 +177,6 @@ initialize(); }); - it('add the tooltip to fullscreen button', function() { - expect($('.add-fullscreen')).toHaveData('qtip'); - }); - it('controls are in paused state', function() { expect(videoControl.isPlaying).toBe(false); }); diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js index 93b839dedf..bb0e5fee7d 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js @@ -39,21 +39,6 @@ it('build the seek handle', function() { expect(videoProgressSlider.handle).toBe('.slider .ui-slider-handle'); - expect($.fn.qtip).toHaveBeenCalledWith({ - content: "0:00", - position: { - my: 'bottom center', - at: 'top center', - container: videoProgressSlider.handle - }, - hide: { - delay: 700 - }, - style: { - classes: 'ui-tooltip-slider', - widget: true - } - }); }); }); @@ -114,21 +99,6 @@ // // it('build the seek handle', function() { // expect(videoProgressSlider.handle).toBe('.ui-slider-handle'); - // expect($.fn.qtip).toHaveBeenCalledWith({ - // content: "0:00", - // position: { - // my: 'bottom center', - // at: 'top center', - // container: videoProgressSlider.handle - // }, - // hide: { - // delay: 700 - // }, - // style: { - // classes: 'ui-tooltip-slider', - // widget: true - // } - // }); // }); // }); }); @@ -181,10 +151,6 @@ expect(videoProgressSlider.frozen).toBeTruthy(); }); - it('update the tooltip', function() { - expect($.fn.qtip).toHaveBeenCalled(); - }); - it('trigger seek event', function() { expect(videoPlayer.onSlideSeek).toHaveBeenCalled(); expect(videoPlayer.currentTime).toEqual(20); @@ -199,9 +165,6 @@ value: 20 }); }); - it('update the tooltip', function() { - expect($.fn.qtip).toHaveBeenCalled(); - }); }); describe('onStop', function() { @@ -228,18 +191,6 @@ expect(videoProgressSlider.frozen).toBeFalsy(); }); }); - - describe('updateTooltip', function() { - beforeEach(function() { - initialize(); - spyOn($.fn, 'slider').andCallThrough(); - videoProgressSlider.updateTooltip(90); - }); - - it('set the tooltip value', function() { - expect($.fn.qtip).toHaveBeenCalledWith('option', 'content.text', '1:30'); - }); - }); }); }).call(this); diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js b/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js index d5d7149ceb..88d866cd32 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/01_initialize.js @@ -93,14 +93,7 @@ function (VideoPlayer) { fadeOutTimeout: 1400, - availableQualities: ['hd720', 'hd1080', 'highres'], - - qTipConfig: { - position: { - my: 'top right', - at: 'top center' - } - } + availableQualities: ['hd720', 'hd1080', 'highres'] }; if (!(_parseYouTubeIDs(state))) { diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js index c7a575046f..1ae0ccf00b 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js @@ -53,9 +53,6 @@ function () { if (!onTouchBasedDevice()) { state.videoControl.pause(); - - state.videoControl.playPauseEl.qtip(state.config.qTipConfig); - state.videoControl.fullScreenEl.qtip(state.config.qTipConfig); } else { state.videoControl.play(); } @@ -77,7 +74,7 @@ function () { $(document).on('keyup', state.videoControl.exitFullScreen); if (state.videoType === 'html5') { - state.el.on('mousemove', state.videoControl.showControls) + state.el.on('mousemove', state.videoControl.showControls); } } diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/05_video_quality_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/05_video_quality_control.js index 952d760610..24363cafed 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/05_video_quality_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/05_video_quality_control.js @@ -43,10 +43,6 @@ function () { state.videoQualityControl.el.show(); state.videoQualityControl.quality = null; - - if (!onTouchBasedDevice()) { - state.videoQualityControl.el.qtip(state.config.qTipConfig); - } } // function _bindHandlers(state) diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/videoalpha/06_video_progress_slider.js index 4409446c2a..b36fac6cd0 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/06_video_progress_slider.js @@ -32,9 +32,7 @@ function () { // get the 'state' object as a context. function _makeFunctionsPublic(state) { state.videoProgressSlider.onSlide = _.bind(onSlide, state); - state.videoProgressSlider.onChange = _.bind(onChange, state); state.videoProgressSlider.onStop = _.bind(onStop, state); - state.videoProgressSlider.updateTooltip = _.bind(updateTooltip, state); state.videoProgressSlider.updatePlayTime = _.bind(updatePlayTime, state); //Added for tests -- JM state.videoProgressSlider.buildSlider = _.bind(buildSlider, state); @@ -56,22 +54,6 @@ function () { function _buildHandle(state) { state.videoProgressSlider.handle = state.videoProgressSlider.el.find('.ui-slider-handle'); - - state.videoProgressSlider.handle.qtip({ - content: '' + Time.format(state.videoProgressSlider.slider.slider('value')), - position: { - my: 'bottom center', - at: 'top center', - container: state.videoProgressSlider.handle - }, - hide: { - delay: 700 - }, - style: { - classes: 'ui-tooltip-slider', - widget: true - } - }); } // *************************************************************** @@ -83,7 +65,6 @@ function () { function buildSlider(state) { state.videoProgressSlider.slider = state.videoProgressSlider.el.slider({ range: 'min', - change: state.videoProgressSlider.onChange, slide: state.videoProgressSlider.onSlide, stop: state.videoProgressSlider.onStop }); @@ -91,15 +72,10 @@ function () { function onSlide(event, ui) { this.videoProgressSlider.frozen = true; - this.videoProgressSlider.updateTooltip(ui.value); this.trigger('videoPlayer.onSlideSeek', {'type': 'onSlideSeek', 'time': ui.value}); } - function onChange(event, ui) { - this.videoProgressSlider.updateTooltip(ui.value); - } - function onStop(event, ui) { var _this = this; @@ -112,10 +88,6 @@ function () { }, 200); } - function updateTooltip(value) { - this.videoProgressSlider.handle.qtip('option', 'content.text', '' + Time.format(value)); - } - //Changed for tests -- JM: Check if it is the cause of Chrome Bug Valera noticed function updatePlayTime(params) { if ((this.videoProgressSlider.slider) && (!this.videoProgressSlider.frozen)) { From 594ed6e0c3fd5546399fa501b5079d57c0f6422f Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Tue, 6 Aug 2013 19:17:46 +0300 Subject: [PATCH 14/30] Fixed failing test for VideoAlpha caption heiht. It turns out that there was wrong invocation of toBeCloseTo() function. Most likely a typo. Updated the readme. --- common/lib/xmodule/xmodule/js/spec/videoalpha/readme.md | 3 ++- .../xmodule/js/spec/videoalpha/video_caption_spec.js | 7 +++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/readme.md b/common/lib/xmodule/xmodule/js/spec/videoalpha/readme.md index 570827b910..1462c14acf 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/readme.md +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/readme.md @@ -1,7 +1,8 @@ Jasmine JavaScript tests status ------------------------------- -As of 22.07.2013, all the tests in this directory pass. To disable each of them, change the top level "describe(" to "xdescribe(". +As of 22.07.2013, all the tests in this directory pass. To enable a test file, change +the top level "xdescribe(" to "describe(". PS: When you are running the tests in chrome locally, make sure that chrome is started with the option "--allow-file-access-from-files". diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js index 1e6e5b8e11..4c46b577db 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js @@ -368,16 +368,15 @@ expect(realHeight).toBeCloseTo(shouldBeHeight, 2); }); - xit('when CC button is disabled ', function() { + it('when CC button is disabled ', function() { var realHeight = parseInt($('.subtitles').css('maxHeight'), 10), videoWrapperHeight = $('.video-wrapper').height(), - controlsHeight = videoControl.el.height(), progressSliderHeight = videoControl.sliderEl.height(), - shouldBeHeight = videoWrapperHeight - controlsHeight - 0.5 * controlsHeight; + shouldBeHeight = videoWrapperHeight - 0.5 * progressSliderHeight; state.captionsHidden = true; videoCaption.setSubtitlesHeight(); - expect(realHeight).toBeCloseTo($('.video-wrapper').height(shouldBeHeight, 2)); + expect(realHeight).toBeCloseTo(shouldBeHeight, 2); }); }); From fb0efd1b16c62c3a39fa7d9bb8d777edb3c1953b Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 7 Aug 2013 11:06:46 +0300 Subject: [PATCH 15/30] Made it so that keyboard also prolongs auto hiding of controls. --- common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js | 2 ++ .../lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js | 1 + .../lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js | 1 + 3 files changed, 4 insertions(+) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js index 415ac440ad..c3ed8fe907 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js @@ -104,6 +104,8 @@ it('parse the videos if subtitles do not exist', function () { var sub = ''; + // !! + $('#example').find('.videoalpha').data('sub', ''); state = new window.VideoAlpha('#example'); diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js index 1ae0ccf00b..d1a662712f 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/04_video_control.js @@ -75,6 +75,7 @@ function () { if (state.videoType === 'html5') { state.el.on('mousemove', state.videoControl.showControls); + state.el.on('keydown', state.videoControl.showControls); } } diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js index e95f99109e..4210d927a2 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js @@ -109,6 +109,7 @@ function () { if (this.videoType === 'html5') { this.el.on('mousemove', this.videoCaption.autoShowCaptions); + this.el.on('keydown', this.videoCaption.autoShowCaptions); // Moving slider on subtitles is not a mouse move, // but captions and controls should be showed. From 6f431df9108bf087a574f3f2a8e56920b0c75d2f Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 7 Aug 2013 14:33:44 +0300 Subject: [PATCH 16/30] Fixed bug dealing with empty subtitles Url string. Now when subs parameter is empty or the YouTube IDs are not specified, captions will not try to fetch non-existent file. --- .../lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js index 4210d927a2..b8bc432a1b 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/09_video_caption.js @@ -123,6 +123,10 @@ function () { this.videoCaption.hideCaptions(this.hide_captions); + if (!this.youtubeId('1.0')) { + return; + } + $.ajaxWithPrefix({ url: _this.videoCaption.captionURL(), notifyOnError: false, From 740a343b76a4e1470fcd1b4f316acf843b103fca Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 7 Aug 2013 14:40:56 +0300 Subject: [PATCH 17/30] Styling improvements. --- common/lib/xmodule/xmodule/css/videoalpha/display.scss | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/videoalpha/display.scss b/common/lib/xmodule/xmodule/css/videoalpha/display.scss index 0b94e1bd71..85425c1b01 100644 --- a/common/lib/xmodule/xmodule/css/videoalpha/display.scss +++ b/common/lib/xmodule/xmodule/css/videoalpha/display.scss @@ -421,7 +421,7 @@ div.videoalpha { a.hide-subtitles { background: url('../images/cc.png') center no-repeat; - display: block; + /* display: block; */ float: left; font-weight: 800; line-height: 46px; //height of play pause buttons @@ -444,9 +444,7 @@ div.videoalpha { opacity: 0.7; } - background-color: inherit; color: #797979; - text-decoration: inherit; } } } From f08394ac2c6e52a58d891755b46356e5ba9b7d16 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 7 Aug 2013 15:28:30 +0300 Subject: [PATCH 18/30] Volume and speeds dialogs are fully extended. When the controls receive focus not via mouse click or mouse hover, their appropriate slider and selection menu are fully extended and shown to the user. --- .../xmodule/xmodule/css/videoalpha/display.scss | 1 - .../js/src/videoalpha/07_video_volume_control.js | 13 +++++++++++++ .../js/src/videoalpha/08_video_speed_control.js | 16 ++++++++++++---- lms/templates/videoalpha.html | 10 +++++----- 4 files changed, 30 insertions(+), 10 deletions(-) diff --git a/common/lib/xmodule/xmodule/css/videoalpha/display.scss b/common/lib/xmodule/xmodule/css/videoalpha/display.scss index 85425c1b01..93c387315a 100644 --- a/common/lib/xmodule/xmodule/css/videoalpha/display.scss +++ b/common/lib/xmodule/xmodule/css/videoalpha/display.scss @@ -421,7 +421,6 @@ div.videoalpha { a.hide-subtitles { background: url('../images/cc.png') center no-repeat; - /* display: block; */ float: left; font-weight: 800; line-height: 46px; //height of play pause buttons diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js index b778a2cbd0..ac2857b027 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js @@ -61,6 +61,11 @@ function () { slide: state.videoVolumeControl.onChange }); + // Make sure that we can't focus the actual volume slider while Tabing. + state.videoVolumeControl.volumeSliderEl.find('a').each(function (index, value) { + $(value).attr('tabindex', '-1'); + }); + state.videoVolumeControl.el.toggleClass('muted', state.videoVolumeControl.currentVolume === 0); } @@ -74,9 +79,17 @@ function () { $(this).addClass('open'); }); + state.videoVolumeControl.buttonEl.on('focus', function() { + $(this).parent().addClass('open'); + }); + state.videoVolumeControl.el.on('mouseleave', function() { $(this).removeClass('open'); }); + + state.videoVolumeControl.buttonEl.on('blur', function() { + $(this).parent().removeClass('open'); + }); } // *************************************************************** diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js index a13aa03f4f..5e0685a550 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js @@ -46,9 +46,9 @@ function () { $.each(state.videoSpeedControl.speeds, function(index, speed) { //var link = $('' + speed + 'x'); - var link = '' + speed + 'x'; + var link = '' + speed + 'x'; - state.videoSpeedControl.videoSpeedsEl.prepend($('
  • ' + link + '
    • ')); + state.videoSpeedControl.videoSpeedsEl.prepend($('
  • ' + link + '
    • ')); }); state.videoSpeedControl.setSpeed(state.speed); @@ -77,6 +77,14 @@ function () { event.preventDefault(); $(this).removeClass('open'); }); + + state.videoSpeedControl.el.children('a') + .on('focus', function () { + $(this).parent().addClass('open'); + }) + .on('blur', function () { + $(this).parent().removeClass('open'); + }); } } @@ -120,9 +128,9 @@ function () { var link, listItem; //link = $('' + speed + 'x'); - link = '' + speed + 'x'; + link = '' + speed + 'x'; - listItem = $('
  • ' + link + '
    • '); + listItem = $('
  • ' + link + '
    • '); if (speed === params.currentSpeed) { listItem.addClass('active'); diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html index e0e0a6c0d9..fd11f2248f 100644 --- a/lms/templates/videoalpha.html +++ b/lms/templates/videoalpha.html @@ -49,15 +49,15 @@
    -

    ${_('Speed')}

    -

    +

    ${_('Speed')}

    +

    -
      +
        -
        -
        +
        +
        ${_('Fill browser')} From 326d958727c1a3491b72f1d6d3411f5ca9fb7018 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Wed, 7 Aug 2013 17:11:55 +0300 Subject: [PATCH 19/30] Replaced Video with VideoAlpha. Now all Video tags and VideoAlpha tags will be handled by VideoAlpha. --- common/lib/xmodule/setup.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 6b106dd94d..98e04f9936 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -39,7 +39,8 @@ setup( "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", "timelimit = xmodule.timelimit_module:TimeLimitDescriptor", "vertical = xmodule.vertical_module:VerticalDescriptor", - "video = xmodule.video_module:VideoDescriptor", + # "video = xmodule.video_module:VideoDescriptor", + "video = xmodule.videoalpha_module:VideoAlphaDescriptor", "videoalpha = xmodule.videoalpha_module:VideoAlphaDescriptor", "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", "videosequence = xmodule.seq_module:SequenceDescriptor", From 1f238b953bffeb9a3204f00125fef33a0c2f3a33 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 09:43:05 +0300 Subject: [PATCH 20/30] Fixed test for height of player when CC is disabled. It turned out that we were reading the heights of the contols and other elements before we initialized the heights. --- .../js/spec/videoalpha/video_caption_spec.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js index 4c46b577db..d1ec74bb13 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoCaptionAlpha', function() { + describe('VideoCaptionAlpha', function() { var state, videoPlayer, videoCaption, videoSpeedControl, oldOTBD; function initialize() { @@ -369,14 +369,21 @@ }); it('when CC button is disabled ', function() { - var realHeight = parseInt($('.subtitles').css('maxHeight'), 10), - videoWrapperHeight = $('.video-wrapper').height(), - progressSliderHeight = videoControl.sliderEl.height(), - shouldBeHeight = videoWrapperHeight - 0.5 * progressSliderHeight; + var realHeight, videoWrapperHeight, progressSliderHeight, + controlHeight, shouldBeHeight; state.captionsHidden = true; videoCaption.setSubtitlesHeight(); - expect(realHeight).toBeCloseTo(shouldBeHeight, 2); + + realHeight = parseInt($('.subtitles').css('maxHeight'), 10); + videoWrapperHeight = $('.video-wrapper').height(); + progressSliderHeight = videoControl.sliderEl.height(); + controlHeight = videoControl.el.height(); + shouldBeHeight = videoWrapperHeight - + 0.5 * progressSliderHeight - + controlHeight; + + expect(realHeight).toBe(shouldBeHeight); }); }); From 4f207299dbc479b018cb03a6f0627750b07d39a3 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 10:11:23 +0300 Subject: [PATCH 21/30] Changed back handling of Video by original Video code. --- common/lib/xmodule/setup.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/common/lib/xmodule/setup.py b/common/lib/xmodule/setup.py index 98e04f9936..6b106dd94d 100644 --- a/common/lib/xmodule/setup.py +++ b/common/lib/xmodule/setup.py @@ -39,8 +39,7 @@ setup( "slides = xmodule.backcompat_module:TranslateCustomTagDescriptor", "timelimit = xmodule.timelimit_module:TimeLimitDescriptor", "vertical = xmodule.vertical_module:VerticalDescriptor", - # "video = xmodule.video_module:VideoDescriptor", - "video = xmodule.videoalpha_module:VideoAlphaDescriptor", + "video = xmodule.video_module:VideoDescriptor", "videoalpha = xmodule.videoalpha_module:VideoAlphaDescriptor", "videodev = xmodule.backcompat_module:TranslateCustomTagDescriptor", "videosequence = xmodule.seq_module:SequenceDescriptor", From f9f9123ce4b7a9430134c394f6d8a21e190b47d3 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 10:30:13 +0300 Subject: [PATCH 22/30] Removed unnecessary commented out code from Jasmine tests. --- .../js/spec/videoalpha/video_caption_spec.js | 22 ------- .../js/spec/videoalpha/video_player_spec.js | 57 ++----------------- .../videoalpha/video_progress_slider_spec.js | 23 -------- .../videoalpha/video_speed_control_spec.js | 14 +---- 4 files changed, 6 insertions(+), 110 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js index d1ec74bb13..bd45c6d701 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js @@ -130,7 +130,6 @@ describe('mouse movement', function() { beforeEach(function() { - //initialize(); window.setTimeout.andReturn(100); spyOn(window, 'clearTimeout'); }); @@ -221,10 +220,6 @@ }); describe('search', function() { - beforeEach(function() { - //initialize(); - }); - it('return a correct caption index', function() { expect(videoCaption.search(0)).toEqual(0); expect(videoCaption.search(3120)).toEqual(1); @@ -277,7 +272,6 @@ describe('pause', function() { beforeEach(function() { - //initialize(); videoCaption.playing = true; videoCaption.pause(); }); @@ -288,10 +282,6 @@ }); describe('updatePlayTime', function() { - /*beforeEach(function() { - initialize(); - });*/ - describe('when the video speed is 1.0x', function() { beforeEach(function() { videoSpeedControl.currentSpeed = '1.0'; @@ -439,17 +429,6 @@ }); it('scroll to current caption', function() { - // Check for calledWith(parameters) for some reason fails... - // - // var offset = -0.5 * ($('.video-wrapper').height() - $('.subtitles .current:first').height()); - // - // expect($.fn.scrollTo).toHaveBeenCalledWith( - // $('.subtitles .current:first', videoCaption.el), - // { - // offset: offset - // } - // ); - expect($.fn.scrollTo).toHaveBeenCalled(); }); }); @@ -459,7 +438,6 @@ describe('seekPlayer', function() { describe('when the video speed is 1.0x', function() { beforeEach(function() { - //initialize(); videoSpeedControl.currentSpeed = '1.0'; $('.subtitles li[data-start="14910"]').trigger('click'); }); diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js index c807936ecd..18e442b227 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js @@ -74,38 +74,6 @@ // All the toHandleWith() expect tests are not necessary for this version of Video Alpha. // jQuery event system is not used to trigger and invoke methods. This is an artifact from // previous version of Video Alpha. - // - // xit('bind to video control play event', function() { - // expect($(videoControl)).toHandleWith('play', player.play); - // }); - // - // xit('bind to video control pause event', function() { - // expect($(videoControl)).toHandleWith('pause', player.pause); - // }); - // - // xit('bind to video caption seek event', function() { - // expect($(videoCaption)).toHandleWith('caption_seek', player.onSeek); - // }); - // - // xit('bind to video speed control speedChange event', function() { - // expect($(videoSpeedControl)).toHandleWith('speedChange', player.onSpeedChange); - // }); - // - // xit('bind to video progress slider seek event', function() { - // expect($(videoProgressSlider)).toHandleWith('slide_seek', player.onSeek); - // }); - // - // xit('bind to video volume control volumeChange event', function() { - // expect($(videoVolumeControl)).toHandleWith('volumeChange', player.onVolumeChange); - // }); - // - // xit('bind to key press', function() { - // expect($(document.documentElement)).toHandleWith('keyup', player.bindExitFullScreen); - // }); - // - // xit('bind to fullscreen switching button', function() { - // expect($('.add-fullscreen')).toHandleWith('click', player.toggleFullScreen); - // }); }); it('create Youtube player', function() { @@ -143,20 +111,6 @@ // We can't test the invocation of HTML5Video because it is not available // globally. It is defined within the scope of Require JS. - // - // xit('create HTML5 player', function() { - // spyOn(state.HTML5Video, 'Player').andCallThrough(); - // initialize(); - // - // expect(window.HTML5Video.Player).toHaveBeenCalledWith(this.video.el, { - // playerVars: playerVars, - // videoSources: this.video.html5Sources, - // events: { - // onReady: player.onReady, - // onStateChange: player.onStateChange - // } - // }); - // }); describe('when not on a touch based device', function() { beforeEach(function() { @@ -419,11 +373,9 @@ expect(state.setSpeed).toHaveBeenCalledWith('0.75', false); }); - // Not relevant any more. + // Not relevant any more: // - // it('tell video caption that the speed has changed', function() { - // expect(this.player.caption.currentSpeed).toEqual('0.75'); - // }); + // expect( "tell video caption that the speed has changed" ) ... }); describe('when the video is playing', function() { @@ -534,8 +486,9 @@ expect(true).toBe(false); } - // The below test has been replaced by above trickery. - // expect($('.vidtime')).toHaveHtml('1:00 / 1:01'); + // The below test has been replaced by above trickery: + // + // expect($('.vidtime')).toHaveHtml('1:00 / 1:01'); }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js index bb0e5fee7d..f0e177d5d7 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js @@ -54,7 +54,6 @@ // We can't expect $.fn.slider not to have been called, // because sliders are used in other parts of VideoAlpha. - // expect($.fn.slider).not.toHaveBeenCalled(); }); }); }); @@ -79,28 +78,6 @@ }); // Currently, the slider is not rebuilt if it does not exist. - // - // describe('when the slider was not already built', function() { - // beforeEach(function() { - // spyOn($.fn, 'slider').andCallThrough(); - // videoProgressSlider.slider = null; - // videoPlayer.play(); - // }); - // - // it('build the slider', function() { - // expect(videoProgressSlider.slider).toBe('.slider'); - // expect($.fn.slider).toHaveBeenCalledWith({ - // range: 'min', - // change: videoProgressSlider.onChange, - // slide: videoProgressSlider.onSlide, - // stop: videoProgressSlider.onStop - // }); - // }); - // - // it('build the seek handle', function() { - // expect(videoProgressSlider.handle).toBe('.ui-slider-handle'); - // }); - // }); }); describe('updatePlayTime', function() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js index c56375f6f7..c40a9c7295 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js @@ -90,19 +90,7 @@ // detect (and do not do anything) if there is a request for a speed that // is already set. // - // describe('when new speed is the same', function() { - // beforeEach(function() { - // initialize(); - // videoSpeedControl.setSpeed(1.0); - // spyOn(videoPlayer, 'onSpeedChange').andCallThrough(); - // - // $('li[data-speed="1.0"] a').click(); - // }); - // - // it('does not trigger speedChange event', function() { - // expect(videoPlayer.onSpeedChange).not.toHaveBeenCalled(); - // }); - // }); + // describe("when new speed is the same") ... describe('when new speed is not the same', function() { beforeEach(function() { From 417bf6dd85cf08eef05629beca97fc3bb84d981f Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 10:32:35 +0300 Subject: [PATCH 23/30] Removed some more comments. --- common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js index c3ed8fe907..415ac440ad 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js @@ -104,8 +104,6 @@ it('parse the videos if subtitles do not exist', function () { var sub = ''; - // !! - $('#example').find('.videoalpha').data('sub', ''); state = new window.VideoAlpha('#example'); From 8a3ef339858d4589315be021518d10c0789bb587 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 11:57:57 +0300 Subject: [PATCH 24/30] Fixing tests related to fetching YouTube metadata. Previously we were using dummy YouTube IDs such as "slowerSpeedYoutubeId", and "normalSpeedYoutubeId". This was causing errors when the code tried to fetch metadata from YouTube with that ID. We can't stub these fetch requests because the data that is fetched is necessary. For example it contains the length of the video. --- .../xmodule/xmodule/js/fixtures/video.html | 4 ++-- .../xmodule/js/fixtures/videoalpha.html | 2 +- .../xmodule/js/fixtures/videoalpha_all.html | 2 +- .../xmodule/js/fixtures/videoalpha_html5.html | 2 +- .../js/fixtures/videoalpha_no_captions.html | 2 +- .../lib/xmodule/xmodule/js/spec/helper.coffee | 19 ++++++++------- .../video/display/video_caption_spec.coffee | 2 +- .../video/display/video_player_spec.coffee | 8 +++---- .../xmodule/js/spec/video/display_spec.coffee | 24 +++++++++---------- .../js/spec/videoalpha/general_spec.js | 20 ++++++++-------- .../js/spec/videoalpha/html5_video_spec.js | 2 +- .../js/spec/videoalpha/video_control_spec.js | 2 +- .../js/spec/videoalpha/video_player_spec.js | 6 ++--- .../videoalpha/video_progress_slider_spec.js | 2 +- .../videoalpha/video_quality_control_spec.js | 2 +- .../videoalpha/video_speed_control_spec.js | 2 +- .../videoalpha/video_volume_control_spec.js | 2 +- 17 files changed, 53 insertions(+), 50 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/fixtures/video.html b/common/lib/xmodule/xmodule/js/fixtures/video.html index 1b27255b1e..3273dd3aa7 100644 --- a/common/lib/xmodule/xmodule/js/fixtures/video.html +++ b/common/lib/xmodule/xmodule/js/fixtures/video.html @@ -2,8 +2,8 @@
        + navigator.userAgent.match /iPhone|iPod|iPad/i + jasmine.stubbedCaption = end: [3120, 6270, 8490, 21620, 24920, 25750, 27900, 34380, 35550, 40250] start: [1180, 3120, 6270, 14910, 21620, 24920, 25750, 27900, 34380, 35550] @@ -36,7 +39,7 @@ jasmine.stubbedCaption = # # We will replace it with a function that does: # -# 1.) Return a hard coded captions object if the file name contains 'test_name_of_the_subtitles'. +# 1.) Return a hard coded captions object if the file name contains 'Z5KLxerq05Y'. # 2.) Behaves the same a as the origianl in all other cases. window.jQuery.ajaxWithPrefix = (url, settings) -> @@ -46,7 +49,7 @@ window.jQuery.ajaxWithPrefix = (url, settings) -> success = settings.success data = settings.data - if url.match(/test_name_of_the_subtitles/g) isnt null or url.match(/slowerSpeedYoutubeId/g) isnt null or url.match(/normalSpeedYoutubeId/g) isnt null + if url.match(/Z5KLxerq05Y/g) isnt null or url.match(/7tqY6eQzVhE/g) isnt null or url.match(/cogebirgzzM/g) isnt null if window.jQuery.isFunction(success) is true success jasmine.stubbedCaption else if window.jQuery.isFunction(data) is true @@ -60,11 +63,11 @@ window.WAIT_TIMEOUT = 1000 jasmine.getFixtures().fixturesPath = 'xmodule/js/fixtures' jasmine.stubbedMetadata = - slowerSpeedYoutubeId: - id: 'slowerSpeedYoutubeId' + '7tqY6eQzVhE': + id: '7tqY6eQzVhE' duration: 300 - normalSpeedYoutubeId: - id: 'normalSpeedYoutubeId' + 'cogebirgzzM': + id: 'cogebirgzzM' duration: 200 bogus: duration: 100 @@ -117,7 +120,7 @@ jasmine.stubVideoPlayer = (context, enableParts, createPlayer=true) -> loadFixtures 'video.html' jasmine.stubRequests() YT.Player = undefined - videosDefinition = '0.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId' + videosDefinition = '0.75:7tqY6eQzVhE,1.0:cogebirgzzM' context.video = new Video '#example', videosDefinition jasmine.stubYoutubePlayer() if createPlayer @@ -135,7 +138,7 @@ jasmine.stubVideoPlayerAlpha = (context, enableParts, html5=false) -> YT.Player = undefined window.OldVideoPlayerAlpha = undefined jasmine.stubYoutubePlayer() - return new VideoAlpha '#example', '.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId' + return new VideoAlpha '#example', '.75:7tqY6eQzVhE,1.0:cogebirgzzM' # Stub jQuery.cookie diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee index 7f4f6073cb..2c339b3ca2 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_caption_spec.coffee @@ -19,7 +19,7 @@ describe 'VideoCaption', -> @caption = @player.caption it 'set the youtube id', -> - expect(@caption.youtubeId).toEqual 'normalSpeedYoutubeId' + expect(@caption.youtubeId).toEqual 'cogebirgzzM' it 'create the caption element', -> expect($('.video')).toContain 'ol.subtitles' diff --git a/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee index dab8c0815a..9cec0e6e96 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display/video_player_spec.coffee @@ -35,7 +35,7 @@ describe 'VideoPlayer', -> expect(window.VideoCaption.prototype.initialize).toHaveBeenCalled() expect(@player.caption).toBeDefined() expect(@player.caption.el).toBe @player.el - expect(@player.caption.youtubeId).toEqual 'normalSpeedYoutubeId' + expect(@player.caption.youtubeId).toEqual 'cogebirgzzM' expect(@player.caption.currentSpeed).toEqual '1.0' expect(@player.caption.captionAssetPath).toEqual '/static/subs/' @@ -60,7 +60,7 @@ describe 'VideoPlayer', -> showinfo: 0 enablejsapi: 1 modestbranding: 1 - videoId: 'normalSpeedYoutubeId' + videoId: 'cogebirgzzM' events: onReady: @player.onReady onStateChange: @player.onStateChange @@ -290,7 +290,7 @@ describe 'VideoPlayer', -> @player.onSpeedChange {}, '0.75' it 'load the video', -> - expect(@player.player.loadVideoById).toHaveBeenCalledWith 'slowerSpeedYoutubeId', '80.000' + expect(@player.player.loadVideoById).toHaveBeenCalledWith '7tqY6eQzVhE', '80.000' it 'trigger updatePlayTime event', -> expect(@player.updatePlayTime).toHaveBeenCalledWith '80.000' @@ -301,7 +301,7 @@ describe 'VideoPlayer', -> @player.onSpeedChange {}, '0.75' it 'cue the video', -> - expect(@player.player.cueVideoById).toHaveBeenCalledWith 'slowerSpeedYoutubeId', '80.000' + expect(@player.player.cueVideoById).toHaveBeenCalledWith '7tqY6eQzVhE', '80.000' it 'trigger updatePlayTime event', -> expect(@player.updatePlayTime).toHaveBeenCalledWith '80.000' diff --git a/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee b/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee index 0ba0cc85f8..35a56a83ae 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee +++ b/common/lib/xmodule/xmodule/js/spec/video/display_spec.coffee @@ -5,14 +5,14 @@ describe 'Video', -> loadFixtures 'video.html' jasmine.stubRequests() - @slowerSpeedYoutubeId = 'slowerSpeedYoutubeId' - @normalSpeedYoutubeId = 'normalSpeedYoutubeId' + @['7tqY6eQzVhE'] = '7tqY6eQzVhE' + @['cogebirgzzM'] = 'cogebirgzzM' metadata = - slowerSpeedYoutubeId: - id: @slowerSpeedYoutubeId + '7tqY6eQzVhE': + id: @['7tqY6eQzVhE'] duration: 300 - normalSpeedYoutubeId: - id: @normalSpeedYoutubeId + 'cogebirgzzM': + id: @['cogebirgzzM'] duration: 200 afterEach -> @@ -38,8 +38,8 @@ describe 'Video', -> it 'parse the videos', -> expect(@video.videos).toEqual - '0.75': @slowerSpeedYoutubeId - '1.0': @normalSpeedYoutubeId + '0.75': @['7tqY6eQzVhE'] + '1.0': @['cogebirgzzM'] it 'fetch the video metadata', -> expect(@video.fetchMetadata).toHaveBeenCalled @@ -102,12 +102,12 @@ describe 'Video', -> describe 'with speed', -> it 'return the video id for given speed', -> - expect(@video.youtubeId('0.75')).toEqual @slowerSpeedYoutubeId - expect(@video.youtubeId('1.0')).toEqual @normalSpeedYoutubeId + expect(@video.youtubeId('0.75')).toEqual @['7tqY6eQzVhE'] + expect(@video.youtubeId('1.0')).toEqual @['cogebirgzzM'] describe 'without speed', -> it 'return the video id for current speed', -> - expect(@video.youtubeId()).toEqual @normalSpeedYoutubeId + expect(@video.youtubeId()).toEqual @cogebirgzzM describe 'setSpeed', -> beforeEach -> @@ -148,6 +148,6 @@ describe 'Video', -> it 'call the logger with valid parameters', -> expect(Logger.log).toHaveBeenCalledWith 'someEvent', id: 'id' - code: @normalSpeedYoutubeId + code: @cogebirgzzM currentTime: 25 speed: '1.0' diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js index 415ac440ad..39a1e64c65 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js @@ -1,14 +1,14 @@ (function () { - xdescribe('VideoAlpha', function () { + describe('VideoAlpha', function () { var oldOTBD; beforeEach(function () { jasmine.stubRequests(); oldOTBD = window.onTouchBasedDevice; window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice').andReturn(false); - this.videosDefinition = '0.75:slowerSpeedYoutubeId,1.0:normalSpeedYoutubeId'; - this.slowerSpeedYoutubeId = 'slowerSpeedYoutubeId'; - this.normalSpeedYoutubeId = 'normalSpeedYoutubeId'; + this.videosDefinition = '0.75:7tqY6eQzVhE,1.0:cogebirgzzM'; + this['7tqY6eQzVhE'] = '7tqY6eQzVhE'; + this['cogebirgzzM'] = 'cogebirgzzM'; }); afterEach(function () { @@ -45,8 +45,8 @@ it('parse the videos', function () { expect(this.state.videos).toEqual({ - '0.75': this.slowerSpeedYoutubeId, - '1.0': this.normalSpeedYoutubeId + '0.75': this['7tqY6eQzVhE'], + '1.0': this['cogebirgzzM'] }); }); @@ -91,7 +91,7 @@ }); it('parse the videos if subtitles exist', function () { - var sub = 'test_name_of_the_subtitles'; + var sub = 'Z5KLxerq05Y'; expect(state.videos).toEqual({ '0.75': sub, @@ -165,14 +165,14 @@ describe('with speed', function () { it('return the video id for given speed', function () { - expect(state.youtubeId('0.75')).toEqual(this.slowerSpeedYoutubeId); - expect(state.youtubeId('1.0')).toEqual(this.normalSpeedYoutubeId); + expect(state.youtubeId('0.75')).toEqual(this['7tqY6eQzVhE']); + expect(state.youtubeId('1.0')).toEqual(this['cogebirgzzM']); }); }); describe('without speed', function () { it('return the video id for current speed', function () { - expect(state.youtubeId()).toEqual(this.normalSpeedYoutubeId); + expect(state.youtubeId()).toEqual(this.cogebirgzzM); }); }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js index 9a6c44052c..b8b03ee6bf 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js @@ -1,5 +1,5 @@ (function () { - xdescribe('VideoAlpha HTML5Video', function () { + describe('VideoAlpha HTML5Video', function () { var state, player, oldOTBD, playbackRates = [0.75, 1.0, 1.25, 1.5]; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js index b98fd1e413..dfa7a75368 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoControlAlpha', function() { + describe('VideoControlAlpha', function() { var state, videoControl, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js index 18e442b227..7566ca0964 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoPlayerAlpha', function() { + describe('VideoPlayerAlpha', function() { var state, videoPlayer, player, videoControl, videoCaption, videoProgressSlider, videoSpeedControl, videoVolumeControl, oldOTBD; function initialize(fixture) { @@ -54,7 +54,7 @@ it('create video caption', function() { expect(videoCaption).toBeDefined(); - expect(state.youtubeId()).toEqual('test_name_of_the_subtitles'); + expect(state.youtubeId()).toEqual('Z5KLxerq05Y'); expect(state.speed).toEqual('1.0'); expect(state.config.caption_asset_path).toEqual('/static/subs/'); }); @@ -98,7 +98,7 @@ modestbranding: 1, html5: 1 }, - videoId: 'normalSpeedYoutubeId', + videoId: 'cogebirgzzM', events: { onReady: videoPlayer.onReady, onStateChange: videoPlayer.onStateChange, diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js index f0e177d5d7..f19d4245c5 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoProgressSliderAlpha', function() { + describe('VideoProgressSliderAlpha', function() { var state, videoPlayer, videoProgressSlider, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js index 7126dc5921..1605551e63 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoQualityControlAlpha', function() { + describe('VideoQualityControlAlpha', function() { var state, videoControl, videoQualityControl, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js index c40a9c7295..fe2a537781 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoSpeedControlAlpha', function() { + describe('VideoSpeedControlAlpha', function() { var state, videoPlayer, videoControl, videoSpeedControl; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js index f7f6c6c583..dfed7c351d 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js @@ -1,5 +1,5 @@ (function() { - xdescribe('VideoVolumeControlAlpha', function() { + describe('VideoVolumeControlAlpha', function() { var state, videoControl, videoVolumeControl, oldOTBD; function initialize() { From 4c7250a000c2785ebb6b5ea80b4001a630a84b14 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 12:17:15 +0300 Subject: [PATCH 25/30] Improved strings in template. Only relevant part of string will go through the Python internationalization function. --- lms/templates/videoalpha.html | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html index fd11f2248f..a7650c0360 100644 --- a/lms/templates/videoalpha.html +++ b/lms/templates/videoalpha.html @@ -63,7 +63,7 @@ ${_('Fill browser')} ${_('HD')} - Captions + ${_('Captions')}
        @@ -75,12 +75,12 @@ % if sources.get('main'):
        -

        ${_('Download video here.') % sources.get('main')}

        +

        ${(_('Download video') + ' ' + _('here') + '.') % sources.get('main')}

        % endif % if track:
        -

        ${_('Download subtitles here.') % track}

        +

        ${(_('Download subtitles') + ' ' + _('here') + '.') % track}

        % endif From 8bbe1c39f6d526127ffeebb976daacab0b840d2c Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 12:47:19 +0300 Subject: [PATCH 26/30] Turned off all VideoAlpha Jasmine tests. --- .../xmodule/xmodule/js/spec/videoalpha/general_spec.js | 2 +- .../xmodule/js/spec/videoalpha/html5_video_spec.js | 2 +- .../xmodule/js/spec/videoalpha/video_caption_spec.js | 2 +- .../xmodule/js/spec/videoalpha/video_control_spec.js | 2 +- .../xmodule/js/spec/videoalpha/video_player_spec.js | 2 +- .../js/spec/videoalpha/video_progress_slider_spec.js | 2 +- .../js/spec/videoalpha/video_quality_control_spec.js | 2 +- .../js/spec/videoalpha/video_speed_control_spec.js | 2 +- .../js/spec/videoalpha/video_volume_control_spec.js | 2 +- .../xmodule/js/src/videoalpha/08_video_speed_control.js | 8 ++++---- 10 files changed, 13 insertions(+), 13 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js index 39a1e64c65..bde4e6b43f 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/general_spec.js @@ -1,5 +1,5 @@ (function () { - describe('VideoAlpha', function () { + xdescribe('VideoAlpha', function () { var oldOTBD; beforeEach(function () { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js index b8b03ee6bf..9a6c44052c 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/html5_video_spec.js @@ -1,5 +1,5 @@ (function () { - describe('VideoAlpha HTML5Video', function () { + xdescribe('VideoAlpha HTML5Video', function () { var state, player, oldOTBD, playbackRates = [0.75, 1.0, 1.25, 1.5]; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js index bd45c6d701..07c6c196c9 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_caption_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoCaptionAlpha', function() { + xdescribe('VideoCaptionAlpha', function() { var state, videoPlayer, videoCaption, videoSpeedControl, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js index dfa7a75368..b98fd1e413 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_control_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoControlAlpha', function() { + xdescribe('VideoControlAlpha', function() { var state, videoControl, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js index 7566ca0964..5e4e40cdec 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_player_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoPlayerAlpha', function() { + xdescribe('VideoPlayerAlpha', function() { var state, videoPlayer, player, videoControl, videoCaption, videoProgressSlider, videoSpeedControl, videoVolumeControl, oldOTBD; function initialize(fixture) { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js index f19d4245c5..f0e177d5d7 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_progress_slider_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoProgressSliderAlpha', function() { + xdescribe('VideoProgressSliderAlpha', function() { var state, videoPlayer, videoProgressSlider, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js index 1605551e63..7126dc5921 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_quality_control_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoQualityControlAlpha', function() { + xdescribe('VideoQualityControlAlpha', function() { var state, videoControl, videoQualityControl, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js index fe2a537781..c40a9c7295 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_speed_control_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoSpeedControlAlpha', function() { + xdescribe('VideoSpeedControlAlpha', function() { var state, videoPlayer, videoControl, videoSpeedControl; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js index dfed7c351d..f7f6c6c583 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js @@ -1,5 +1,5 @@ (function() { - describe('VideoVolumeControlAlpha', function() { + xdescribe('VideoVolumeControlAlpha', function() { var state, videoControl, videoVolumeControl, oldOTBD; function initialize() { diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js index 5e0685a550..c54c65fc59 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js @@ -46,9 +46,9 @@ function () { $.each(state.videoSpeedControl.speeds, function(index, speed) { //var link = $('' + speed + 'x'); - var link = '' + speed + 'x'; + var link = '' + speed + 'x'; - state.videoSpeedControl.videoSpeedsEl.prepend($('
      1. ' + link + '
        2. ')); + state.videoSpeedControl.videoSpeedsEl.prepend($('
      2. ' + link + '
        3. ')); }); state.videoSpeedControl.setSpeed(state.speed); @@ -128,9 +128,9 @@ function () { var link, listItem; //link = $('' + speed + 'x'); - link = '' + speed + 'x'; + link = '' + speed + 'x'; - listItem = $('
      3. ' + link + '
        4. '); + listItem = $('
      4. ' + link + '
        5. '); if (speed === params.currentSpeed) { listItem.addClass('active'); From 5380b5cfc35d05d80d99c9d11bec17a37e250c4b Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 13:36:13 +0300 Subject: [PATCH 27/30] Making it possible to tab through individual speeds. In speed control, when it is focused, a drop down becomes available which contains all of the available speeds that the player can switch to. When using the Tab button to tab through all of the controls, it should be possible to select individual speed with the keyboard. --- .../videoalpha/video_volume_control_spec.js | 1 - .../src/videoalpha/08_video_speed_control.js | 71 +++++++++++++------ 2 files changed, 49 insertions(+), 23 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js index f7f6c6c583..872b1fe18e 100644 --- a/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/videoalpha/video_volume_control_spec.js @@ -39,7 +39,6 @@ range: "min", min: 0, max: 100, - /* value: 100, */ value: videoVolumeControl.currentVolume, change: videoVolumeControl.onChange, slide: videoVolumeControl.onChange diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js index c54c65fc59..b0f55d49a1 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/08_video_speed_control.js @@ -21,34 +21,41 @@ function () { // function _makeFunctionsPublic(state) // - // Functions which will be accessible via 'state' object. When called, these functions will - // get the 'state' object as a context. + // Functions which will be accessible via 'state' object. When called, + // these functions will get the 'state' object as a context. function _makeFunctionsPublic(state) { - state.videoSpeedControl.changeVideoSpeed = _.bind(changeVideoSpeed, state); + state.videoSpeedControl.changeVideoSpeed = _.bind( + changeVideoSpeed, state + ); state.videoSpeedControl.setSpeed = _.bind(setSpeed, state); state.videoSpeedControl.reRender = _.bind(reRender, state); } // function _renderElements(state) // - // Create any necessary DOM elements, attach them, and set their initial configuration. Also - // make the created DOM elements available via the 'state' object. Much easier to work this - // way - you don't have to do repeated jQuery element selects. + // Create any necessary DOM elements, attach them, and set their + // initial configuration. Also make the created DOM elements available + // via the 'state' object. Much easier to work this way - you don't + // have to do repeated jQuery element selects. function _renderElements(state) { state.videoSpeedControl.speeds = state.speeds; state.videoSpeedControl.el = state.el.find('div.speeds'); - state.videoSpeedControl.videoSpeedsEl = state.videoSpeedControl.el.find('.video_speeds'); + state.videoSpeedControl.videoSpeedsEl = state.videoSpeedControl.el + .find('.video_speeds'); - state.videoControl.secondaryControlsEl.prepend(state.videoSpeedControl.el); + state.videoControl.secondaryControlsEl.prepend( + state.videoSpeedControl.el + ); $.each(state.videoSpeedControl.speeds, function(index, speed) { + var link = '' + speed + 'x'; - //var link = $('' + speed + 'x'); - var link = '' + speed + 'x'; - - state.videoSpeedControl.videoSpeedsEl.prepend($('
      5. ' + link + '
        6. ')); + state.videoSpeedControl.videoSpeedsEl + .prepend( + $('
      6. ' + link + '
        7. ') + ); }); state.videoSpeedControl.setSpeed(state.speed); @@ -56,9 +63,11 @@ function () { // function _bindHandlers(state) // - // Bind any necessary function callbacks to DOM events (click, mousemove, etc.). + // Bind any necessary function callbacks to DOM events (click, + // mousemove, etc.). function _bindHandlers(state) { - state.videoSpeedControl.videoSpeedsEl.find('a').on('click', state.videoSpeedControl.changeVideoSpeed); + state.videoSpeedControl.videoSpeedsEl.find('a') + .on('click', state.videoSpeedControl.changeVideoSpeed); if (onTouchBasedDevice()) { state.videoSpeedControl.el.on('click', function(event) { @@ -83,20 +92,30 @@ function () { $(this).parent().addClass('open'); }) .on('blur', function () { - $(this).parent().removeClass('open'); + state.videoSpeedControl.videoSpeedsEl + .find('a.speed_link:first') + .focus(); + }); + + state.videoSpeedControl.videoSpeedsEl.find('a.speed_link:last') + .on('blur', function () { + state.videoSpeedControl.el.removeClass('open'); }); } } // *************************************************************** // Public functions start here. - // These are available via the 'state' object. Their context ('this' keyword) is the 'state' object. - // The magic private function that makes them available and sets up their context is makeFunctionsPublic(). + // These are available via the 'state' object. Their context ('this' + // keyword) is the 'state' object. The magic private function that makes + // them available and sets up their context is makeFunctionsPublic(). // *************************************************************** function setSpeed(speed) { this.videoSpeedControl.videoSpeedsEl.find('li').removeClass('active'); - this.videoSpeedControl.videoSpeedsEl.find("li[data-speed='" + speed + "']").addClass('active'); + this.videoSpeedControl.videoSpeedsEl + .find("li[data-speed='" + speed + "']") + .addClass('active'); this.videoSpeedControl.el.find('p.active').html('' + speed + 'x'); } @@ -110,10 +129,15 @@ function () { this.videoSpeedControl.setSpeed( // To meet the API expected format. - parseFloat(this.videoSpeedControl.currentSpeed).toFixed(2).replace(/\.00$/, '.0') + parseFloat(this.videoSpeedControl.currentSpeed) + .toFixed(2) + .replace(/\.00$/, '.0') ); - this.trigger('videoPlayer.onSpeedChange', this.videoSpeedControl.currentSpeed); + this.trigger( + 'videoPlayer.onSpeedChange', + this.videoSpeedControl.currentSpeed + ); } } @@ -127,7 +151,6 @@ function () { $.each(this.videoSpeedControl.speeds, function(index, speed) { var link, listItem; - //link = $('' + speed + 'x'); link = '' + speed + 'x'; listItem = $('
      7. ' + link + '
        8. '); @@ -139,7 +162,11 @@ function () { _this.videoSpeedControl.videoSpeedsEl.prepend(listItem); }); - this.videoSpeedControl.videoSpeedsEl.find('a').on('click', this.videoSpeedControl.changeVideoSpeed); + this.videoSpeedControl.videoSpeedsEl.find('a') + .on('click', this.videoSpeedControl.changeVideoSpeed); + + // TODO: After the control was re-rendered, we should attach 'focus' + // and 'blur' events once more. } }); From f301231051b8aeb2ba87b962b25375b75c0a3a05 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Thu, 8 Aug 2013 14:23:10 +0300 Subject: [PATCH 28/30] Enabled tabbing to volume slider. --- .../js/src/videoalpha/07_video_volume_control.js | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js index ac2857b027..0f1cd1182d 100644 --- a/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/videoalpha/07_video_volume_control.js @@ -61,10 +61,8 @@ function () { slide: state.videoVolumeControl.onChange }); - // Make sure that we can't focus the actual volume slider while Tabing. - state.videoVolumeControl.volumeSliderEl.find('a').each(function (index, value) { - $(value).attr('tabindex', '-1'); - }); + // Make sure that we can focus the actual volume slider while Tabing. + state.videoVolumeControl.volumeSliderEl.find('a').attr('tabindex', '0'); state.videoVolumeControl.el.toggleClass('muted', state.videoVolumeControl.currentVolume === 0); } @@ -88,7 +86,11 @@ function () { }); state.videoVolumeControl.buttonEl.on('blur', function() { - $(this).parent().removeClass('open'); + state.videoVolumeControl.volumeSliderEl.find('a').focus(); + }); + + state.videoVolumeControl.volumeSliderEl.find('a').on('blur', function () { + state.videoVolumeControl.el.removeClass('open'); }); } From 1efea116f6a61ff0c02fa2a4a905f129f44c2e82 Mon Sep 17 00:00:00 2001 From: Valera Rozuvan Date: Fri, 9 Aug 2013 10:26:18 +0300 Subject: [PATCH 29/30] Removed unnecessary leading and trailing spaces from heading in template. --- lms/templates/videoalpha.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/videoalpha.html b/lms/templates/videoalpha.html index a7650c0360..d0eb7290a7 100644 --- a/lms/templates/videoalpha.html +++ b/lms/templates/videoalpha.html @@ -1,7 +1,7 @@ <%! from django.utils.translation import ugettext as _ %> % if display_name is not UNDEFINED and display_name is not None: -

        ${display_name}

        +

        ${display_name}

        % endif
        Date: Fri, 9 Aug 2013 10:01:23 -0400 Subject: [PATCH 30/30] Studio: revises export prompt control copy --- cms/templates/export.html | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cms/templates/export.html b/cms/templates/export.html index dd494a532b..3356bea42b 100644 --- a/cms/templates/export.html +++ b/cms/templates/export.html @@ -13,19 +13,19 @@ %if unit: var dialog = new CMS.Views.Prompt({ - title: gettext('There has been an error with your export.'), + title: gettext('There has been an error while exporting.'), message: gettext("There has been a failure to export to XML at least one component. It is recommended that you go to the edit page and repair the error before attempting another export. Please check that all components on the page are valid and do not display any error messages."), intent: "error", actions: { primary: { - text: gettext('Yes, allow me to fix the failed component'), + text: gettext('Correct failed component'), click: function(view) { view.hide(); document.location = "${edit_unit_url}" } }, secondary: { - text: gettext('Cancel'), + text: gettext('Return to Export'), click: function(view) { view.hide(); } @@ -54,7 +54,7 @@ } } } - }); + }); %endif dialog.show(); })