Stopping video playback for hidden steps #115

jbzdak · 2016-04-04T20:42:24Z

This is a follow up on: #114 and #112.

Course set up

What you need is a course where on a step you have a video, and then you can to the next step (you should still hear the video on that next step --- which is the error).

Create a course with three sections, each containing a single subsection and a single unit.
First unit contains following steps:

a. A step with Video Block and a question
b. A step with HTML Block with a youtube video and a question. Example embedding code: <iframe width="420" height="315" src="https://www.youtube.com/embed/QH2-TGUlwu4" frameborder="0" allowfullscreen></iframe>
c. A step with a question (no videos!)
d. A review step with a HTML with a youtube video, and other review elements.
Second unit contains

a. A step with OOyala video and a question (OOyala set-up is described in the OC-1441 issue)
b. A step with a question
c. A review step
Third unit contains (note that having the same Ooyala video on a two steps in single unit might not work.

a. A step with a question
a. A review step with a HTML block with ooyala video embedded.

Verification instructions

Please note that OOyala video needs to be manually started by pasting a snippet to a javascript console --- snippet is attached above the video, and looks like that: OO.ready(function() { OO.Player.create('xxx', 'yyy_zzz'); });. After you start the video it should work as it was started "normally" so tests are still valid.
Go to the block with video, start the video and wait until it loads, fill in the quiz and go to next step. Note that you still hear video music in the background.
Check the same with the OOyala video subsection.
Note that on re-entering the ooyala videos on review step the video doesn't display itself, this is due to the fact that html of review elements is replaced when displaying this step (this is current behaviour in master), which doesn't play well with ooyala .

Testing instructions

For each of the units:

Checkout this branch.
Go to video and block, start the video and wait until it loads, fill in the quiz and go to next step. Note that the video is no longer playing in the background.
Fill in rest of the quiz, watch the review step, and go back to the video step. Verify that video is still working properly.
Check if:

a. Reloading a page with a video works
b. Video works on first visit
c. Video works when re-visiting
d. Video XBlock rememebers it's position
Note that Ooyala videos in review steps are still broken.

jbzdak · 2016-04-05T09:11:05Z

problem_builder/public/js/mentoring_with_steps.js

    }

-    var activeStep = $('.mentoring', element).data('active-step');
+    var activeStepIndex = $('.mentoring', element).data('active-step');


Most of the changes in this file are leftover from last PR, I left it in on the basis that this makes the code cleaner. But they can be safely removed.

jbzdak · 2016-04-05T09:50:29Z

problem_builder/public/js/lms_util.js

@@ -0,0 +1,109 @@
+(function () {


This can be moved to step.js but I figured that it would be better to have it on separate file, to keep every file simple enough.

@jbzdak Is there a reason not to move the contents of this file to the existing util.js file? If they do need to be kept in a separate file, I think it would make sense to call it step_util.js to indicate the relationship more clearly. (This would also take into account the fact that usage of this file isn't limited to the LMS runtime.)

@itsjeyd utils.js is used only in studio (or cms), and this one is used only in lms and while this would have negligible effect on performance, i decided that adding unneeded code would be bad, so I decided to split them.

I'll rename them step_utils.js.

jbzdak · 2016-04-05T11:37:18Z

@itsjeyd This is ready for review.

itsjeyd · 2016-04-07T18:18:23Z

problem_builder/public/js/lms_util.js

+    /**
+     *  Manager for HTML XBlocks. These blocks are hid by detaching and shown
+     *  by re-attaching them to the DOM.This is only generic way to generically
+     *  handle things like video players (they should stop playing when removed from DOM).


@jbzdak "hid" ⟶ "hidden"

"DOM.This is only generic way to generically handle" ⟶ "DOM. This is the only way to generically handle"

itsjeyd · 2016-04-07T18:32:20Z

@jbzdak Reviewed! Please ping me when you're done addressing the comments so I can have another quick look.

itsjeyd · 2016-04-07T20:32:57Z

problem_builder/public/js/step.js

    var submitXHR, resultsXHR,
        message = $(element).find('.sb-step-message');
+
+    var childManager = new ProblemBuilderUtilLMS.ChildManager(element, runtime);


@jbzdak Now that the utils file has been renamed, ProblemBuilderUtilLMS should be renamed as well I think. How about just StepUtil?

Done. I went for ProblemBuilderStepUtil this is a global name attached to window object, so I'd rather be verbose than have a clash.

itsjeyd · 2016-04-07T20:41:28Z

@jbzdak 👍 once the build is green and @bradenmacdonald confirms that we can go ahead without adding integration tests for now.

bradenmacdonald · 2016-04-08T00:36:32Z

@jbzdak As long as your code is well commented (looks like it is - thanks!), writing integration tests would require enormous effort (which seems to be the case), and all the other non-video-specific refactoring you've done is covered by tests, that's fine with me in this case.

The only thing I would suggest is that you first have a look to see if you can use XBlock.register_temp_plugin to temporarily register a mock HTML or Video XBlock which offers a JS pause method, then do an acceptance test with that, to make sure that the pause method is being called. But again if you look into that and it's going to be an enormous amount of work, we can go without. Then we can revisit testing when we implement first-class support for ooyala blocks.

jbzdak · 2016-04-08T13:26:18Z

@bradenmacdonald

Using XBlock.register_temp_plugin would work, if not for this. JQuery-xblock doesnt return Html XModules as children of the step (this runtime is used on Apros), so I added a workaround of querying dom for HTML modules. . . and this workaround would not work in XBlock sdk as the dom elements generated by sdk and lms are different.

In LMS I can select html xblock using div.xblock[data-module-type="html"] and in SDK I'd have to use div.xblock-v1[data-module-type="html"], of course I could add some more or less ugly hack, to make it work. But I'd suggest rather pushing for inclusion of Ooyala block as a first class citizen, and then removing the "detach html nodes from DOM" logic.

bradenmacdonald · 2016-04-11T16:11:42Z

@jbzdak Sounds good, let's push for that.

jbzdak · 2016-04-11T16:40:46Z

Merging.

jbzdak reviewed Apr 5, 2016
View reviewed changes

jbzdak force-pushed the jbzdak/oc-1441/detach-html-elements branch from af3c0e7 to 6d9fae4 Compare April 5, 2016 09:15

jbzdak reviewed Apr 5, 2016
View reviewed changes

jbzdak force-pushed the jbzdak/oc-1441/detach-html-elements branch from 6d9fae4 to 28bdcbd Compare April 5, 2016 09:52

Detach HTML elements from the DOM when hiding a step.

5a0c553

jbzdak force-pushed the jbzdak/oc-1441/detach-html-elements branch from 7a3f6f7 to c477ea0 Compare April 7, 2016 11:13

itsjeyd reviewed Apr 7, 2016
View reviewed changes

jbzdak force-pushed the jbzdak/oc-1441/detach-html-elements branch 3 times, most recently from a26168b to 2af1ddc Compare April 7, 2016 20:13

Removed special handling when hiding video blocks.

2af1ddc

itsjeyd reviewed Apr 7, 2016
View reviewed changes

jbzdak force-pushed the jbzdak/oc-1441/detach-html-elements branch from 8fa4281 to 4b7b231 Compare April 7, 2016 20:34

Renamed lms_utils -> step_util

4b7b231

jbzdak merged commit 313837a into master Apr 11, 2016

jbzdak deleted the jbzdak/oc-1441/detach-html-elements branch April 11, 2016 16:40

jbzdak mentioned this pull request Apr 11, 2016

Bump hash for problem builder edx/edx-solutions-edx-platform#676

Merged

itsjeyd mentioned this pull request Aug 9, 2016

Problem Builder - Review for release on edx.org #119

Merged

3 tasks

Stopping video playback for hidden steps #115

Stopping video playback for hidden steps #115

Uh oh!

Conversation

jbzdak commented Apr 4, 2016

Course set up

Verification instructions

Testing instructions

Uh oh!

jbzdak Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

jbzdak Apr 5, 2016

Choose a reason for hiding this comment

Uh oh!

itsjeyd Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

jbzdak Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

jbzdak commented Apr 5, 2016

Uh oh!

itsjeyd Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

itsjeyd commented Apr 7, 2016

Uh oh!

itsjeyd Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

jbzdak Apr 7, 2016

Choose a reason for hiding this comment

Uh oh!

itsjeyd commented Apr 7, 2016

Uh oh!

bradenmacdonald commented Apr 8, 2016

Uh oh!

jbzdak commented Apr 8, 2016

Uh oh!

bradenmacdonald commented Apr 11, 2016

Uh oh!

jbzdak commented Apr 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants