Fix #3790: Add automatic text-to-speech audio to explorations #3818

tjiang11 · 2017-08-28T04:24:34Z

No description provided.

seanlip

Thanks, @tjiang11! I'm still kind of amazed this is even possible, it's very cool :)

PTAL at comments; happy to talk about anything that needs discussion.

seanlip · 2017-08-28T09:29:04Z

assets/constants.js

@@ -271,11 +271,20 @@ var constants = {
  "SUPPORTED_AUDIO_LANGUAGES": [{
    "id": "en",
    "text": "English",
-    "related_languages": ["en"]
+    "related_languages": ["en"],
+    "speech_synthesis_code": "en-GB"


I'm not sure the modelling here is right. I don't think we should have dicts with different keys inside a list that should be homogeneous.

Could we instead have two separate top-level constants: SUPPORTED_AUDIO_LANGUAGES and AUTOGENERATED_AUDIO_LANGUAGES, perhaps? The two things are handled rather differently anyway in e.g. the editor view, so I think it might make sense to separate them. Though you may need to unify both when generating the dropdown choices for the audio preferences.

seanlip · 2017-08-28T09:31:24Z

core/templates/dev/head/domain/utilities/BrowserCheckerService.js

+    // For details on the reliabiltiy of this check, see
+    // https://stackoverflow.com/questions/4565112/
+    // javascript-how-to-find-out-if-the-user-browser-is-chrome
+    if (isIOSChrome ||


Maybe use an additional paren since I don't think people tend to remember whether || or && takes precedence.

Incidentally, audio doesn't work on Chromium on Ubuntu, at least on my machine. Maybe related to https://askubuntu.com/questions/761975/chromium-is-not-generating-voice

seanlip · 2017-08-28T09:31:40Z

core/templates/dev/head/domain/utilities/BrowserCheckerService.js

+  var isIOSChrome = winNav.userAgent.match('CriOS');
+
+  var _isChrome = function() {
+    // For details on the reliabiltiy of this check, see


"reliability" is misspelled

seanlip · 2017-08-28T09:32:40Z

core/templates/dev/head/domain/utilities/LanguageUtilService.js

@@ -36,6 +36,16 @@ oppia.factory('LanguageUtilService', [function() {
      audioLanguage.related_languages;
  });

+  var generatedAudioLanguageCodesToSpeechSynthesisLanguageCode = {};


Again should we be storing human-generated audio + autogenerated audio separately (and only unifying where needed)? Feels like it'd be less implicit; you're currently relying on adding custom strings and stuff to the IDs which feels a bit less maintainable.

seanlip · 2017-08-28T09:34:21Z

core/templates/dev/head/pages/exploration_player/AudioControlsDirective.js

@@ -53,39 +56,61 @@ oppia.directive('audioControls', [
              AudioTranslationManagerService.getCurrentAudioLanguageCode()];
          };

-          $scope.AudioPlayerService = AudioPlayerService;
+          $scope.showSpeakerPlayingIcon = function() {


Why not replace these two methods with $scope.isAudioPlaying()?

seanlip · 2017-08-28T09:44:29Z

core/templates/dev/head/services/SpeechSynthesisChunkerService.js

+      }
+      else {
+        var chunkLength = (settings && settings.chunkLength) || 160;
+        var pattRegex = new RegExp('^[\\s\\S]{' +


Some intuitive explanation may be helpful here to explain what this regex is meant to represent.

seanlip · 2017-08-28T09:46:35Z

core/templates/dev/head/services/SpeechSynthesisChunkerService.js

+        newUtt = new SpeechSynthesisUtterance(chunk);
+        var x;
+        for (x in utt) {
+          if (x !== 'text') {


No idea what x is, or what the significance of 'text' is. Maybe use better name / comment?

seanlip · 2017-08-28T09:48:21Z

core/templates/dev/head/services/SpeechSynthesisChunkerService.js

+      html = html.replace(new RegExp('</li>', 'g'), '.').trim();
+      // Strip away HTML tags.
+      var tmp = document.createElement('div');
+      tmp.innerHTML = html;


In general we eschew using innerHTML; there are security concerns. Can you find another way to do this? There's an rteHelperService in app.js that uses .html(), perhaps that might be better. Also maybe it's worth adding a convertToReadableText() thing in there (see previous comment) --- though I'm not 100% sure if that's the best place for it or if having a new service is better.

used html()

seanlip · 2017-08-28T09:51:15Z

data/explorations/modeling_graphs/Graph Modeling.yaml

@@ -449,7 +449,7 @@ states:
      html: 'When mathematicians talk about graph theory, they usually aren''t referring
        to curve sketching!<div><br></div><div>A graph is a mathematical object that
        consists of "vertices" and "edges". Does this sound complicated? Actually,
-        it isn''t: in simple terms, graphs just dots joined by lines! The dots are
+        it isn''t: in simple terms, graphs are just dots joined by lines! The dots are


Nice catch :)

seanlip · 2017-08-28T09:53:40Z

core/templates/dev/head/services/SpeechSynthesisChunkerService.js

+    };
+
+    var _insertSpaceAfterIndex = function(targetString, index) {
+      return targetString.substr(0, index + 1) + ' ' +


So much copying and creation of new strings ... perhaps just build up the new string character by character in the for loop instead?

codecov-io · 2017-09-16T15:51:52Z

Codecov Report

Merging #3818 into develop will decrease coverage by 0.13%.
The diff coverage is 35.71%.

@@             Coverage Diff             @@
##           develop    #3818      +/-   ##
===========================================
- Coverage       46%   45.86%   -0.14%     
===========================================
  Files          283      288       +5     
  Lines        20942    21131     +189     
  Branches      3287     3319      +32     
===========================================
+ Hits          9634     9692      +58     
- Misses       11308    11439     +131

Impacted Files	Coverage Δ
.../pages/exploration_player/AudioPreloaderService.js	`2.5% <0%> (-0.14%)`	⬇️
...ev/head/pages/exploration_player/PlayerServices.js	`1.11% <0%> (-0.01%)`	⬇️
...pages/exploration_player/AudioControlsDirective.js	`1.35% <0%> (-0.54%)`	⬇️
...ead/pages/exploration_player/TutorCardDirective.js	`2.38% <0%> (-0.03%)`	⬇️
...ilities/AutogeneratedAudioLanguageObjectFactory.js	`100% <100%> (ø)`
...ead/domain/utilities/AudioLanguageObjectFactory.js	`100% <100%> (ø)`
...dev/head/services/SpeechSynthesisChunkerService.js	`21.12% <21.12%> (ø)`
...v/head/services/AutogeneratedAudioPlayerService.js	`26.66% <26.66%> (ø)`
...dev/head/domain/utilities/BrowserCheckerService.js	`47.36% <47.36%> (ø)`
...ploration_player/AudioTranslationManagerService.js	`45.58% <7.69%> (-8.96%)`	⬇️
... and 6 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 e04304f...dc2ed3f. Read the comment docs.

seanlip

This is very cool! Just a few minor comments.

Please also resolve merge conflicts and ensure that all existing comments are replied to.

seanlip · 2017-09-17T08:41:29Z

core/templates/dev/head/app.js

@@ -703,6 +703,7 @@ oppia.factory('rteHelperService', [
        var that = this;

        _RICH_TEXT_COMPONENTS.forEach(function(componentDefn) {
+          console.log(componentDefn);


please drop

seanlip · 2017-09-17T08:43:43Z

core/templates/dev/head/pages/exploration_player/AudioControlsDirective.js

-            return Boolean(getAudioTranslationInCurrentLanguage());
+            return Boolean(getAudioTranslationInCurrentLanguage()) ||
+              AudioTranslationManagerService
+                .isAutogeneratedLanguageCodeSelected();


Hm, might want a helper function locally here for checking isAutogeneratedLanguageCodeSelected(). The length of this whole thing seems to affect the readability of not only this line, but below too.

seanlip · 2017-09-17T08:45:49Z

core/templates/dev/head/pages/exploration_player/AudioTranslationManagerService.js

+      // that language if it is available.
+      // 2. If the exploration language has a related audio language, then set
+      // it to that.
+      // 3. If only the autogenerated audio language available, then set it


available --> is available

seanlip · 2017-09-17T08:46:14Z

core/templates/dev/head/pages/exploration_player/AudioTranslationManagerService.js

+            _explorationLanguageCode)) {
+        _currentAudioLanguageCode =
+          LanguageUtilService.getAutogeneratedAudioLanguage(
+            _explorationLanguageCode).id


missing semicolon

seanlip · 2017-09-17T08:47:49Z

core/templates/dev/head/pages/exploration_player/AudioTranslationManagerService.js

+      },
+      getSpeechSynthesisLanguageCode: function() {
+        return LanguageUtilService.getAutogeneratedAudioLanguage(
+          _explorationLanguageCode).speech_synthesis_code;


Should make domain object and use speechSynthesisCode

seanlip · 2017-09-17T08:52:56Z

core/templates/dev/head/pages/exploration_player/AudioTranslationManagerService.js

    var _currentAudioLanguageCode = null;
    var _allAudioLanguageCodesInExploration = null;
    var _explorationLanguageCode = null;
+    var _autogeneratedLanguageCodeIsSelected = false;


Would it be better to deduce this on the fly from the current language code, instead of maintaining a state variable that devs need to remember to update? I'm not sure. But the current "manual update" approach seems a little fragile to me.

yeah true, done

seanlip · 2017-09-17T08:56:21Z

core/templates/dev/head/services/SpeechSynthesisChunkerService.js

+              // Replace dashes with 'minus'.
+              .replace(/-/g, 'minus')
+              // Ensure that 'x^2' is pronounced 'x squared' rather than
+              // 'x karat 2'.


karat --> caret

oops, was recently interviewing with a company called Karat that used a ^ in their logo and this slipped out haha, fixed

seanlip · 2017-09-17T08:57:23Z

core/templates/dev/head/services/SpeechSynthesisChunkerService.js

+      elt.find('oppia-noninteractive-' + RTE_COMPONENT_NAMES.Math)
+        .replaceWith(function() {
+          if (this.attributes['raw_latex-with-value'] !== undefined) {
+            return this.attributes['raw_latex-with-value'].textContent


Consider pulling this part out into a function and adding karma tests that feed in a raw-latex-with-value string and that check the result after all the replacements? I can see this breaking as new improvements are added.

…text; add tests for LaTeX conversion to speakable text

tjiang11 · 2017-09-17T19:33:13Z

Sorry the LaTeX formatting at the moment is pretty sloppy (haven't done much regex before..) and will likely need a good deal of rework later. I just whipped some stuff up to handle basic things for now, but it will need a lot of polishing to handle function composition cleanly.

seanlip · 2017-09-17T20:07:14Z

core/templates/dev/head/pages/exploration_player/AudioControlsDirective.js

@@ -61,28 +61,32 @@ oppia.directive('audioControls', [
              AutogeneratedAudioPlayerService.isPlaying();
          };

+          $scope.isAudioLoading = false;


Consider audioIsLoading. Otherwise, looks like a function.

seanlip · 2017-09-17T20:08:05Z

LGTM. Thanks! Just one small fix, then let's merge.

tjiang11 added 3 commits August 27, 2017 23:06

Add automatics text to speech

75abb79

lint

819688b

Minor refactoring in AudioControlsDirective.

48adf76

tjiang11 requested a review from seanlip August 28, 2017 04:24

seanlip requested changes Aug 28, 2017

View reviewed changes

pranavsid98 assigned tjiang11 Aug 30, 2017

tjiang11 added 3 commits September 2, 2017 17:20

Suggested changes

c733e34

Making Links and LateX speakable

c358f5e

lint

a6bc664

seanlip reviewed Sep 17, 2017

View reviewed changes

tjiang11 added 4 commits September 17, 2017 14:51

Add language object factories; update LaTeX formatting for speakable …

2725311

…text; add tests for LaTeX conversion to speakable text

lint

797525b

lint

6d24a10

merge develop

5c60c3e

seanlip reviewed Sep 17, 2017

View reviewed changes

seanlip approved these changes Sep 17, 2017

View reviewed changes

Change variable name; add dependencies to collection editor

dc2ed3f

seanlip added the PR: LGTM label Sep 18, 2017

seanlip merged commit aecb981 into oppia:develop Sep 18, 2017

Fix #3790: Add automatic text-to-speech audio to explorations #3818

Fix #3790: Add automatic text-to-speech audio to explorations #3818

Conversation

tjiang11 commented Aug 28, 2017

seanlip left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 16, 2017 • edited Loading

Codecov Report

seanlip left a 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

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

tjiang11 commented Sep 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip commented Sep 17, 2017

codecov-io commented Sep 16, 2017 •

edited

Loading

tjiang11 commented Sep 17, 2017 •

edited

Loading