learningequality / ka-lite

KA Lite: lightweight web server for serving core Khan Academy content (videos and exercises) without needing internet connectivity
https://learningequality.org/ka-lite/
Other
456 stars 304 forks source link

Captions on by default. #5467

Closed mrpau-eugene closed 7 years ago

mrpau-eugene commented 7 years ago

Summary

Captions should be enabled by default.

TODO

If not all TODOs are marked, this PR is considered WIP (work in progress)

Issues addressed

List the issues solved or partly solved by the PR

5359

5464

benjaoming commented 7 years ago

@mrpau-eugene the test failure is because of a sad, randomly occurring timeout, I've restarted the test build. These kinds of things happen from time to time unfortunately.. it's typically either a failure in the BDD tests (where it's waiting for some object to show up in the Selenium browser instance) or in this case it was calling kalite status and something went wrong, probably because of high load on the Circle CI backend.

benjaoming commented 7 years ago

Also, there's this just happened.. which also occurs randomly because our server or something in between has hiccups..


======================================================================
FAIL: test_online (kalite.updates.tests.regression_tests.RegistrationRedirectTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/ubuntu/ka-lite/kalite/updates/tests/regression_tests.py", line 39, in test_online
    self.assertEqual(am_i_online(), google_is_online)
AssertionError: False != True

----------------------------------------------------------------------
mrpau-eugene commented 7 years ago

@benjaoming oh.. that's why.. I thought there were some problems on my part while doing a pull request. Anyways, thanks for clarifying! 👍

benjaoming commented 7 years ago

Wow, this PR has now failed 3 times in a row for 3 different reasons :)

  1. the kalite status timeout
  2. the connectivity false negative
  3. a BDD test

@mrpau-eugene one more thing, if you want to go all the way with a perfect PR, is to add a release note in docs/installguide/release_notes.rst. Small tip for docs-only changes is to add [ci skip] to your commit message, then Circle CI will skip the tests.

codecov[bot] commented 7 years ago

Codecov Report

Merging #5467 into 0.17.x will increase coverage by 2.67%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.17.x    #5467      +/-   ##
==========================================
+ Coverage    58.7%   61.38%   +2.67%     
==========================================
  Files         118      118              
  Lines        6572     6572              
==========================================
+ Hits         3858     4034     +176     
+ Misses       2714     2538     -176
Impacted Files Coverage Δ
kalite/version.py
kalite/settings/base.py
kalite/settings/__init__.py
kalite/__init__.py
...ormtools/tests/wizard/namedwizardtests/__init__.py 87.5% <0%> (ø)
kalite/project/settings/__init__.py 76.92% <0%> (ø)
.../django/db/backends/postgresql_psycopg2/version.py 100% <0%> (ø)
kalite/project/settings/base.py 88.07% <0%> (ø)
kalite/coachreports/api_views.py 93.07% <0%> (+3.84%) :arrow_up:
kalite/topic_tools/content_models.py 75.28% <0%> (+5.05%) :arrow_up:
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 153b1aa...517869f. Read the comment docs.

benjaoming commented 7 years ago

@mrpau-eugene @mrpau-eduard @mrpau-richard sorry to ping all of you at once, but I want to have your combined experience with building language packs for this:

Can we reliably tell the language of a video, such that:

If the video is in English and subtitles are available for the currently set language, then display subtitles in this language

Can we reliably use extra_fields.translated_youtube_lang for detecting if the video is in English?

mrpau-eugene commented 7 years ago

@mrpau-eugene one more thing, if you want to go all the way with a perfect PR, is to add a release note in docs/installguide/release_notes.rst. Small tip for docs-only changes is to add [ci skip] to your commit message, then Circle CI will skip the tests.

Okay, I will add it to the release_notes.rst

@mrpau-eugene @mrpau-eduard @mrpau-richard sorry to ping all of you at once, but I want to have your combined experience with building language packs for this:

Can we reliably tell the language of a video, such that:

If the video is in English and subtitles are available for the currently set language, then display subtitles in this language

Can we reliably use extra_fields.translated_youtube_lang for detecting if the video is in English?

@benjaoming I've just browsed the extra_fields.translated_youtube_lang for some language content packs, I think it is reliable for using it to determine whether a video is in English or not.

They have the following values:

English content pack: translated_youtube_lang: en

Spanish content pack: translated_youtube_lang: es-ES

Polski translated_youtube_lang: pl

benjaoming commented 7 years ago

@mrpau-eugene great, thanks for checking! To finalize the PR, then I think, the logic should be as I put in the previous comment. Data is available to state the conditions, so everything should be fine..

mrpau-eugene commented 7 years ago

@benjaoming Okay.. I was thinking that checking the current language was enough since the default language determines which videos will be loaded.

Are there any instances that, for example, the default language is set to Spanish and it gets an English dubbed video or vice versa?

benjaoming commented 7 years ago

Are there any instances that, for example, the default language is set to Spanish and it gets an English dubbed video or vice versa?

Yes, that's allowed... You can have a video with audio in English but with "Spanish" subtitles (replace "Spanish" with any given language).

The vice versa does not happen, i.e. a "Spanish" audio with English subtitles.. it's what I believe is referred to as "anglocentric" :)

benjaoming commented 7 years ago

The vice versa does not happen, i.e. a "Spanish" audio with English subtitles.. it's what I believe is referred to as "anglocentric" :)

Allow me to clarify this: Actually, there are English subtitles on a number of dubbed videos in the KA.org data set. However, this has been filtered out while building the content packs - on purpose because the dubbed audio didn't match the script of the English video. It's also the same thing for "Spanish" videos - we don't show the "Spanish" subtitles because the subtitles were written for the English script and doesn't match the dubbed "Spanish" audio.

Castillian Spanish, I know :) I put Spanish in quotes to try making it clear that it's not just about Spanish, but all the non-English languages.

mrpau-eugene commented 7 years ago

@benjaoming ahh.. i see. thanks for clarifying 👍

I'm done adding the logic you suggested..

radinamatic commented 7 years ago

@mrpau-eugene @benjaoming Awesome progress on this, can't wait to test! :+1:

benjaoming commented 7 years ago

@radinamatic would you be able to test this directly from the 0.17.x git branch? Just need a confirmation that subtitles are displaying as expected.

radinamatic commented 7 years ago

Ok, so as far as I can see, English captions are loaded by default, and videos in Spanish do not display the CC button! 👍

virtualbox_gubuntu_13_06_2017_16_20_21

@benjaoming @mrpau-eugene I'd like to test more languages, and see if I can pinpoint any video that has a subtitle, but no dubbed version, so in other locales plays the English video with the captions in the given locale (do you happen to know any?), so I'll continue tomorrow! 🙂