Peter fogg/remove video xml #132

peter-fogg · 2013-06-11T18:25:04Z

Removes XML from the video component. Maintains backwards compatibility on export/import from XML-based courses.

TODO: This breaks the 1.5x and .75x speeds. I'm still looking into why. TODO: VideoDescriptor inherits from RawDescriptor in order to use the from_xml and export_to_xml methods. This seems really ugly, though; I'd rather find a better way to do this.

cahrens · 2013-06-11T18:56:16Z

common/lib/xmodule/xmodule/video_module.py

    position = Integer(help="Current position in the video", scope=Scope.user_state, default=0)
+    show_captions = Boolean(help="Whether or not captions are shown", display_name="Show Captions", scope=Scope.settings, default=True)
+    youtube_id_1_0 = String(help="Youtube ID for normal speed video", display_name="Normal Speed", scope=Scope.settings, default="OEoXaMPEzfM")
+    youtube_id_0_75 = String(help="Youtube ID for .75x speed video", display_name=".75x", scope=Scope.settings, default="JMD_ifUUfsU")


We need better display names for these properties (and help text). I recommend crafting the display names in such a way that they alphabetically appear together.

Toddi is our documentation person, and she's out this week. I'd recommend having Marco help you in the interim, both with display names and help text.

cahrens · 2013-06-11T19:10:53Z

/home/christina/mitx_all/edx-platform/common/lib/xmodule/xmodule/templates/video/default.yaml needs to be updated. You will want to remove the XML data stub there (though I'm not sure you can entirely delete it, as described in https://edx-wiki.atlassian.net/browse/STUD-146. You may need to make it an empty String or something). Don would be a good person to talk to about this, as the work he's doing may allow us to actually remove the data from the template entirely.

You should also remove data_dir in that template, as it's no longer a metadata field.

To update templates, run django-admin.py update_templates --traceback --settings=cms.envs.dev --pythonpath=.

cahrens · 2013-06-11T19:14:35Z

common/lib/xmodule/xmodule/js/src/video/display.coffee

-      @videos[speed] = video[1]
+    @videos =
+      '0.75': @el.data('youtube-id-0-75')
+      '1.0': @el.data('normal-speed-video-id')


What should the behavior be if not all of these speeds are specified? When I tried it, I still got all the speed options in the LMS, but "An Error Occurred" when I select the unspecified ones. Is that the current behavior?

I could change this to only show a speed if @el.data('youtube-id-x') != ''; that is, if the speed is actually specified by the course author.

That would be oh so nice.

cahrens · 2013-06-11T19:30:16Z

common/lib/xmodule/xmodule/video_module.py

    module_class = VideoModule
    stores_state = True
    template_dir_name = "video"
+
+    metadata_attributes = RawDescriptor.metadata_attributes + ('youtube_id_1_0',
+                                                               'youtube_id_0_75',


What is this for?

chrisndodge · 2013-06-12T04:54:33Z

common/lib/xmodule/xmodule/js/src/video/display.coffee

+    if @el.data('youtube-id-1-25')
+      @videos['1.25'] = @el.data('youtube-id-1-25')
+    if @el.data('youtube-id-1-5')
+      @videos['1.50'] = @el.data('youtube-id-1-5')



Seems like this PR hard codes that only up to 4 speeds can exist - which is the case in practice, but this change may make it harder to change that. Is this OK from a Product Management standpoint?

@markchang You are OK with just the 4 speeds, right? With videoalpha, the interpolation will happen automatically, correct?

@cahrens in videoalpha you will need xblock fields for storing any number of speeds. It is possible to add fields to xblocks dynamically?

I thought that with videoalpha the video speed would be interpolated for you (so you wouldn't need to specify multiple speeds). That is not correct?

You could use a xblock field of type List. However, that would not allow you to specify which speed corresponds to each entry.

There is an xblock field of type Dict. That would allow you to specify the key (speed) and the video.

We currently don't have metadata editors for either of these.

@cahrens @peter-fogg In HTML5 player youtube you need only one video link and one subtitles link.
In HTML5 player non-youtube you will need 3 fields for different sources of video for cross-browser compatibility.
All speeds for video and subtitles will be adjusted automatically in player itself.

My question was related to videoalpha supporting not-html5 youtube. In that case we use same speeds that video player does, it was misunderstanding. Don't mind about this.

valera-rozuvan · 2013-06-12T11:45:52Z

@peter-fogg @cahrens @chrisndodge Are you aware of Video Alpha ( https://edx-wiki.atlassian.net/wiki/display/BLD/Video+Alpha )? Should this also be worked into Video Alpha, since it is the future player of the system...

chrisndodge · 2013-06-12T12:14:00Z

@valera-rozuvan yea we discussed this yesterday in our standup, so we're aware that this is applicable to the Video Alpha. Peter's working on the old video player first.

…ate files; docstring cleaning.

cahrens · 2013-06-18T16:40:24Z

👍

Oh, but remember to follow up on that LMS/CMS videoalpha/normal video step naming problem.

peter-fogg · 2013-06-18T19:08:42Z

Just merged in master to get the import os dev environment fix.

vasylnakvasiuk · 2013-06-19T10:37:38Z

Please, use @rocha suggestion and cleanup commits history using $ git rebase master -i. Thanks.

auraz · 2013-06-19T10:50:38Z

common/test/data/two_toys/video/Video_Resources.xml

@@ -1 +1 @@
-<video youtube="1.0:1bK-WdDi6Qw" display_name="Video Resources"/>
+<video youtube_id_1_0="&quot;1bK-WdDi6Qw&quot;" display_name="Video Resources"/>


Why not leave old xml format? Just when reading inside descriptor, you can parse it to proper xblocks fields.

+1 Cause now you change interface and create possible problems in future (for new branches, tests, etc.)

Peter's code reads in the old format-- he has unit tests around this. Can you explain more you think it is necessary to keep the old format?

@cahrens I see not reason to implement new format in xml if we just can change parsing logics in descriptor from_xml, as have been already done by @peter-fogg . Please explain me why we need new format of xml?

valera-rozuvan · 2013-06-19T11:26:28Z

@peter-fogg Please rebase with current master.

cahrens · 2013-06-19T12:25:04Z

Just a note about interactive rebase-- we do not require an interactive rebase before merging a pull request. Some people do prefer it, but in the case where you have been doing merges with master along the way, interactive rebase is very difficult. I do not think we should require Peter to do it.

auraz · 2013-06-20T10:30:16Z

@cahrens When you mentioned interactive rebase, did you mean common rebase like "git rebase master"?
I'm sure you know, that the idea behind that, is after jenkins will verify tests, you can press "Merge pull request" button. Otherwise, you should run tests manually and do manual merge. This how i understand your message.

It this particular case, there are lot of tests in master that tests video module, and they are not included in current branch. They need to be included and passing before merge.

auraz · 2013-06-20T10:36:56Z

@peter-fogg why you merged master in but not rebasing on top of it? If you use rebase, you will have same changes you need, but you will not have commits in git history with "merged master to you branch" message, which will be hard to merge again in master. (this is just a suggestion)

cahrens · 2013-06-20T12:51:22Z

@auraz Alex, it is true that either a merge or rebase must be done so that the PR can be cleanly merged with master. What I meant was that it is not currently required to do a rebase where multiple commits are "squashed" together and the commit history cleaned up/altered. @vaxXxa had suggested that.

…eo module.

Peter fogg/remove video xml

I'll go ahead and merge this, since it fixes nearly all the issues we had with problem and html css, and this should go in before more changes. The table borders can be fixed in another round.

…-preferences-delete mattdrayer/api-user-preferences-delete: Added new Detail view, GET/DELET...

Change Carnegie's help tab to a mailto

identical to PR openedx#132 * live oauth support (openedx#115) * Updated message for Certificates * Update test_views.py (cherry picked from commit ab88963)

fix bug

…re/fix-activation-email-for-sites Use site_configuration helper to get url base for activation email

Peter Fogg added 5 commits June 6, 2013 10:29

Fixed .75 and 1.5 speeds playing at 1.0 speed.

ee64d8c

Changed description of source and track fields.

115ae42

Fix failing acceptance tests and allow videos to be imported from XML.

f9b825f

Enable exporting video to XML for backwards compatibility.

5b9e88e

ghost assigned cahrens Jun 11, 2013

peter-fogg mentioned this pull request Jun 11, 2013

Make video captions respect course author's setting. #128

Merged

cahrens reviewed Jun 11, 2013
View reviewed changes

Merge peter-fogg/fix-video-captions-setting.

dbb1275

cahrens reviewed Jun 11, 2013
View reviewed changes

Update acceptance tests for captions to use metadata settings editor.

3159c94

cahrens reviewed Jun 11, 2013
View reviewed changes

Peter Fogg added 4 commits June 11, 2013 15:38

Convert Floats to StringyFloats.

716da9c

Remove data attribute from templates/video/default.yaml.

872bf71

Prevent showing video speeds that aren't set by course author.

94ac7fb

Convert show_captions to StringyBoolean.

1ff72e0

chrisndodge reviewed Jun 12, 2013
View reviewed changes

Peter Fogg added 3 commits June 12, 2013 10:27

Update and fix acceptance to show captions when correct.

008c769

Fix failing test bug.

63e376d

Set Youtube ID defaults to the empty string.

52bf82a

Peter Fogg added 4 commits June 17, 2013 16:47

Update CHANGELOG.rst; move acceptance test methods into more appropri…

56015e4

…ate files; docstring cleaning.

Fix merge conflict.

5cb456b

Fix merge conflict in CHANGELOG.rst.

8ae22cf

Fix merge conflict in CHANGELOG.rst.

a403252

Merge branch 'master' into peter-fogg/remove-video-xml

7633968

auraz reviewed Jun 19, 2013
View reviewed changes

Peter Fogg added 5 commits June 20, 2013 09:58

Fix merge conflict in CHANGELOG.rst.

9c7be8b

Fix VideoAlpha acceptance test step definitions to not clash with Vid…

c77eb4f

…eo module.

Make sure we can handle empty youtube attributes.

ea56a0c

Merge in master (again).

1d8d507

Actually fix merge conflicts.

83d84e2

peter-fogg added a commit that referenced this pull request Jun 20, 2013

Merge pull request #132 from edx/peter-fogg/remove-video-xml

0ef4390

Peter fogg/remove video xml

peter-fogg merged commit 0ef4390 into master Jun 20, 2013

peter-fogg deleted the peter-fogg/remove-video-xml branch June 20, 2013 19:40

aboudreault pushed a commit to aboudreault/edx-platform that referenced this pull request Jul 20, 2014

Merge pull request openedx#132 from edx-solutions/mattdrayer/api-user…

5ada4d3

…-preferences-delete mattdrayer/api-user-preferences-delete: Added new Detail view, GET/DELET...

jbau pushed a commit that referenced this pull request Oct 10, 2014

Merge pull request #132 from Stanford-Online/giulio/carnegie-help-link

7d08a29

Change Carnegie's help tab to a mailto

xavierchan added a commit to xavierchan/edx-platform-1 that referenced this pull request Dec 24, 2018

Merge pull request openedx#132 from CrewS/fixui

fde1ad5

fix bug

rediris pushed a commit to gymnasium/edx-platform that referenced this pull request Feb 25, 2021

Merge pull request openedx#132 from appsembler/appsembler/ficus/featu…

c12764b

…re/fix-activation-email-for-sites Use site_configuration helper to get url base for activation email

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Peter fogg/remove video xml #132

Peter fogg/remove video xml #132

peter-fogg commented Jun 11, 2013

cahrens Jun 11, 2013

cahrens commented Jun 11, 2013

cahrens Jun 11, 2013

peter-fogg Jun 11, 2013

cahrens Jun 11, 2013

cahrens Jun 11, 2013

chrisndodge Jun 12, 2013

cahrens Jun 14, 2013

auraz Jun 19, 2013

cahrens Jun 19, 2013

auraz Jun 20, 2013

valera-rozuvan commented Jun 12, 2013

chrisndodge commented Jun 12, 2013

cahrens commented Jun 18, 2013

peter-fogg commented Jun 18, 2013

vasylnakvasiuk commented Jun 19, 2013

auraz Jun 19, 2013

vasylnakvasiuk Jun 19, 2013

cahrens Jun 19, 2013

auraz Jun 20, 2013

valera-rozuvan commented Jun 19, 2013

cahrens commented Jun 19, 2013

auraz commented Jun 20, 2013

auraz commented Jun 20, 2013

cahrens commented Jun 20, 2013

		@@ -1 +1 @@
		<video youtube="1.0:1bK-WdDi6Qw" display_name="Video Resources"/>
		<video youtube_id_1_0=""1bK-WdDi6Qw"" display_name="Video Resources"/>

Peter fogg/remove video xml #132

Peter fogg/remove video xml #132

Conversation

peter-fogg commented Jun 11, 2013

Choose a reason for hiding this comment

cahrens commented Jun 11, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valera-rozuvan commented Jun 12, 2013

chrisndodge commented Jun 12, 2013

cahrens commented Jun 18, 2013

peter-fogg commented Jun 18, 2013

vasylnakvasiuk commented Jun 19, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valera-rozuvan commented Jun 19, 2013

cahrens commented Jun 19, 2013

auraz commented Jun 20, 2013

auraz commented Jun 20, 2013

cahrens commented Jun 20, 2013