Learner view audio translations #3681

tjiang11 · 2017-07-27T06:12:11Z

This PR implements minimum viable functionality for audio in the learner view.

seanlip · 2017-07-27T06:14:05Z

core/templates/dev/head/pages/exploration_player/audio_translation_settings_modal.html

+    <p>
+      Pick Language
+    </p>
+<!--     <select2-drdopdown


seanlip

Nice! Took a first pass.

seanlip · 2017-07-27T06:21:02Z

core/templates/dev/head/domain/exploration/StatesObjectFactory.js

@@ -107,6 +107,18 @@ oppia.factory('StatesObjectFactory', [
      return finalStateNames;
    };

+    States.prototype.getAllAudioLanguageCodes = function() {
+      var allAudioLanguageCodes = new Set();


Set is relatively recent and isn't supported in some slightly older browsers. Might want to consider using an array instead and checking for existing membership with indexOf().

seanlip · 2017-07-27T06:21:19Z

core/templates/dev/head/domain/exploration/StatesObjectFactory.js

+      var allAudioLanguageCodes = new Set();
+      for (var stateName in this._states) {
+        var audioTranslationsForState =
+          this._states[stateName].content.getBindableAudioTranslations()


Missing semicolon.

seanlip · 2017-07-27T06:21:26Z

core/templates/dev/head/domain/exploration/StatesObjectFactory.js

@@ -107,6 +107,18 @@ oppia.factory('StatesObjectFactory', [
      return finalStateNames;
    };

+    States.prototype.getAllAudioLanguageCodes = function() {


seanlip · 2017-07-27T06:22:55Z

core/templates/dev/head/pages/exploration_editor/EditorServices.js

@@ -838,6 +838,13 @@ oppia.factory('explorationLanguageCodeService', [
        }
      }
    };
+    child.getLanguageDescription = function(languageCode) {


I don't think this has anything to do with this factory, actually. You might want to define a separate utility factory for handling global language-related operations that aren't exploration-specific.

moved into LanguageUtilService

seanlip · 2017-07-27T06:25:03Z

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

+
+/**
+ * @fileoverview Service to manage the current language being
+ * used for audio translations as well as related modals.


This is a bit unclear -- on first reading I thought the service also managed the language being used to display the text on related modals, and thought, hm, this isn't in the spec. I think it's fine to drop the "as well as related modals" bit.

seanlip · 2017-07-27T06:53:17Z

core/templates/dev/head/pages/exploration_player/audio_translation_settings_modal.html

+
+  <div class="modal-body">
+    <p>
+      Pick Language


seanlip · 2017-07-27T06:53:55Z

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

@@ -30,6 +33,18 @@ oppia.factory('AudioPlayerService', [
            var blobUrl = URL.createObjectURL(audioBlob);
            _currentTrack = ngAudio.load(blobUrl);
            _currentTrackFilename = filename; 
+
+            // Doesn't seem to be any way of detecing when native


detecting (typo)

seanlip · 2017-07-27T06:54:42Z

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

+            // to grab native audio.
+            // TODO(tjiang11): Find a better way to handle this.
+            $timeout(function() {
+              _currentTrack.audio.onended = function() {


Can you elaborate a bit more (in comments) on the ideas behind the workaround here? I'm not sure I fully understand it.

seanlip · 2017-07-27T06:55:26Z

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

+        if (!_currentTrack) {
+          return false;
+        }
+        return !_currentTrack.paused;


Alternative: return (_currentTrack && !_currentTrack.paused);

seanlip · 2017-07-27T06:55:47Z

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

+        }
+        return !_currentTrack.paused;
+      },
+      trackLoaded: function() {


start with verb. (This one is particularly misleading because "track" is a verb too :P)

codecov-io · 2017-07-27T09:19:41Z

Codecov Report

Merging #3681 into develop will decrease coverage by 0.01%.
The diff coverage is 18.46%.

@@             Coverage Diff             @@
##           develop    #3681      +/-   ##
===========================================
- Coverage    45.03%   45.01%   -0.02%     
===========================================
  Files          261      264       +3     
  Lines        20219    20343     +124     
  Branches      3157     3168      +11     
===========================================
+ Hits          9106     9158      +52     
- Misses       11113    11185      +72

Impacted Files	Coverage Δ
...lates/dev/head/services/AssetsBackendApiService.js	`58.97% <ø> (-1.03%)`	⬇️
...ev/head/pages/exploration_player/PlayerServices.js	`1.71% <0%> (-0.09%)`	⬇️
...xploration_player/ExplorationPlayerStateService.js	`5.26% <0%> (-0.62%)`	⬇️
.../templates/dev/head/services/AudioPlayerService.js	`1.69% <0%> (-0.81%)`	⬇️
...ead/pages/exploration_player/TutorCardDirective.js	`2.81% <0%> (-0.17%)`	⬇️
...s/dev/head/domain/utilities/LanguageUtilService.js	`100% <100%> (ø)`
...dev/head/domain/exploration/StatesObjectFactory.js	`65.75% <100%> (+4.21%)`	⬆️
...ploration_player/AudioTranslationManagerService.js	`2.77% <2.77%> (ø)`
...pages/exploration_player/AudioControlsDirective.js	`3.33% <3.33%> (ø)`
...d/domain/exploration/SubtitledHtmlObjectFactory.js	`90.62% <50%> (-2.71%)`	⬇️
... 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 bc31014...e026b84. Read the comment docs.

tjiang11 · 2017-07-29T06:14:38Z

stop audio on switching cards

seanlip

Very nice, thanks @tjiang11! Mostly trivial comments.

seanlip · 2017-07-29T07:00:05Z

core/templates/dev/head/domain/exploration/ExplorationObjectFactory.js

@@ -122,6 +122,24 @@ oppia.factory('ExplorationObjectFactory', [
      return this.getState(stateName).content.getHtml();
    };

+    Exploration.prototype.getAudioTranslations = function(stateName) {


seanlip · 2017-07-29T07:00:34Z

core/templates/dev/head/domain/exploration/StatesObjectFactory.js

+        var audioTranslationsForState =
+          this._states[stateName].content.getBindableAudioTranslations();
+        for (var languageCode in audioTranslationsForState) {
+          if (allAudioLanguageCodes.indexOf(languageCode) == -1) {


seanlip · 2017-07-29T07:02:57Z

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

+    return {
+      restrict: 'E',
+      scope: {
+        audioTranslations: '='


If this is intended to be readonly (i.e. not modified from within the directive), then use getAudioTranslations: '&audioTranslations' and call $scope.getAudioTranslations() instead.

seanlip · 2017-07-29T07:03:31Z

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

+        '$scope', 'AudioTranslationManagerService', 'AudioPlayerService',
+        function(
+            $scope, AudioTranslationManagerService, AudioPlayerService) {
+          var filenameOfLastStartedAudio;


Initialize to null?

seanlip · 2017-07-29T07:05:33Z

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

+              }
+            } else {
+              AudioPlayerService.pause();
+              if (!isRequestForSameAudioAsLastTime()) {


Still a bit confused by this. Presumably the user has just clicked a button that looks visually like "pause" -- how come we then play?

This should only activate when a user has clicked a "play" button on an audio translation that is different from the current audio being played. This way, the audio player pauses the existing audio and immediately starts playing the newly requested audio translation.

I'm a bit confused about this scenario: "when a user has clicked a 'play' button on an audio translation that is different from the current audio being played". In what circumstances would it happen? Are there ever multiple play buttons available at once?

Ah, my bad: as @tjiang11 clarified in chat, in the future there may be > 1 play button on a page. This handles that correctly, so I don't think further changes are needed here.

seanlip · 2017-07-29T07:20:15Z

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

          };

          $scope.arePreviousResponsesShown = false;

          $scope.waitingForOppiaFeedback = false;

+          $scope.currentAudioLanguageCode = null;


This isn't used or modified?

seanlip · 2017-07-29T07:20:47Z

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

@@ -52,14 +52,16 @@ oppia.directive('tutorCard', [
        '$scope', '$timeout', 'oppiaPlayerService', 'HintManagerService',
        'playerPositionService', 'playerTranscriptService',
        'ExplorationPlayerStateService', 'windowDimensionsService',
-        'urlService', 'TWO_CARD_THRESHOLD_PX', 'CONTENT_FOCUS_LABEL_PREFIX',
+        'urlService', 'AudioPlayerService', 'AudioTranslationManagerService',


The added deps aren't used?

removed, except for AudioPlayerService which I use now for stopping audio on changing cards

seanlip · 2017-07-29T07:21:24Z

core/templates/dev/head/pages/exploration_player/audio_controls_directive.html

+  }
+
+  .audio-control-button:hover {
+    cursor: pointer


Missing semicolon.

seanlip · 2017-07-29T07:22:10Z

core/templates/dev/head/pages/exploration_player/tutor_card_directive.html

           focus-on="<[getContentFocusLabel($index)]>">
+        <span angular-html-bind="activeCard.contentHtml">


Not div? Content may span multiple paragraphs, for instance. Might mean the next bit has to be a div too.

seanlip · 2017-07-29T07:23:47Z

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

@@ -72,6 +72,10 @@ oppia.factory('AssetsBackendApiService', [
    };

    var _getAudioDownloadUrl = function(explorationId, filename) {
+      if (GLOBALS.DEV_MODE) {
+        return '/assets/test/' + filename;


Can we have /assets/test/ be part of constants.js perhaps? Or rather -- have an assets dir prefix that's one thing in dev and another thing (the GCS url prefix) in prod? Better to do the if/else check in just one place if possible.

Not really sure how to go about doing the latter implementation while still avoiding an if/else for the url interpolation?

OK, I thought about this. It's not straightforward, unfortunately. Here's the reasoning:

constants.js needs to be a static dict (since the backend interprets it as JSON), so it can't have any logic in it. That's a shame, because that's the file for constants shared between frontend and backend. But we can't have an if/else condition there.

The other place we store constants is in feconf, and we can do "if DEV_MODE ... else ..." there. We can then ship it to the frontend as a GLOBALS property -- see how ALLOWED_GADGETS is sent in editor.py, for example. The logic here should then be:

return UrlInterpolationService.interpolateUrl(GLOBALS.AUDIO_URL_PREFIX, {...});

This would be better than doing a one-off thing here, because that way there'll be a single audio url prefix used everywhere. This becomes important when we work on the editor page because audio files need to be uploaded to that same location as well, so this prefix needs to be somehow global.

seanlip

Oh noes -- there's a merge conflict!

Otherwise this looks amazing :) I made a few nit comments and maybe we should change the image files to use material-icons or something but other than that, LGTM!

Feel free to merge once you've fixed the remaining stuff and Travis checks pass. Thanks a lot!

seanlip · 2017-08-02T03:26:14Z

core/templates/dev/head/pages/exploration_player/tutor_card_directive.html

@@ -238,6 +244,10 @@
    top: 14px;
  }

+  .audio-controls {


.oppia-player-audio-controls or similar

changed to conversation-skin-audio-controls

seanlip · 2017-08-02T03:27:00Z

feconf.py

+# files.
+
+if DEV_MODE:
+    AUDIO_URL_PREFIX = '/assets/test/<exploration_id>/audio/<filename>'


Hm, looks more like AUDIO_URL_TEMPLATE to me?

seanlip · 2017-08-02T03:28:23Z

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

+        function(
+            $scope, AudioTranslationManagerService, AudioPlayerService) {
+          var filenameOfLastStartedAudio = null;
+          var directiveId = Math.random().toString(36).substr(2, 10);


Please add a comment here to explain what this is used for -- it's not obvious from the naming.

seanlip · 2017-08-02T03:29:58Z

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

+            UrlInterpolationService.getStaticImageUrl(
+              '/icons/settings.svg'));
+
+          var PLAY_AUDIO_BUTTON_IMAGE_URL = (


Naming: use

IMAGE_URL_AUDIO_SETTINGS
IMAGE_URL_PLAY_AUDIO
IMAGE_URL_PAUSE_AUDIO
IMAGE_URL_REWIND_AUDIO

(basically, start with the type. That way it's easier to see that everything is in a "group of related things".)

seanlip · 2017-08-02T03:35:15Z

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

+        '$scope', 'AudioTranslationManagerService', 'AudioPlayerService',
+        function(
+            $scope, AudioTranslationManagerService, AudioPlayerService) {
+          var filenameOfLastStartedAudio = null;


Do you actually still need this?

seanlip · 2017-08-03T04:31:49Z

Just FYI, there are merge conflicts (which block the Travis tests from running).

seanlip · 2017-08-03T05:28:31Z

Hi @tjiang11 -- e2e tests are failing rather badly. Any idea why?

Feel free to pick up and use any patterns in my PR if you need them, btw. I'm happy to handle the merges.

* upstream/develop: (39 commits) Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717) Fix oppia#2998 implement collection skills update commands (oppia#3710) Improvements and bug fixes to changes introduced by PR oppia#3671 (oppia#3684) Fix oppia#3688: refactoring ExplorationStatusHandler to remove unpublish and publicize functionality as it is not called from frontend. (oppia#3715) fix oppia#3698: Shift exception handling for NotLoggedInException so that it does not generate log errors. (oppia#3714) Revert "Fix oppia#3707 Change text of collection learner view "start here"" (oppia#3713) Fix oppia#3701 Fixed Continue button name issue (oppia#3711) Fix oppia#3467: Introduce re-training and add_to_training_queue methods (oppia#3650) Fix oppia#3707 Change text of collection learner view "start here" (oppia#3700) Audio fixes: make audio download URL safe; don't show flagging modal if all translations are flagged; use templateUrl for modals. (oppia#3709) Learner audio updates (oppia#3705) Move hint button to left corner for supplemental cards (oppia#3702) Make the representation of the learner's 'Continue' answer more conversational. (oppia#3675) Add editor for audio translations (oppia#3692) Fix oppia#3686 fixed text overflow for contributors list (oppia#3697) Fix oppia#3551: Manually set cookie value during language change (oppia#3622) Learner view audio translations (oppia#3681) The sixth milestone of the learner dashboard. (oppia#3680) Routine update of translations. (oppia#3685) SiteWide ACL Refactor: Milestone 3.1 (oppia#3682) ...

Audio learner view

6fcab54

tjiang11 requested a review from seanlip July 27, 2017 06:12

seanlip reviewed Jul 27, 2017

View reviewed changes

rm select2

18f4751

seanlip requested changes Jul 27, 2017

View reviewed changes

tjiang11 added 3 commits July 29, 2017 01:54

suggested changes

8e77542

lint

b3e0d6b

lint

6dfcbfa

seanlip reviewed Jul 29, 2017

View reviewed changes

anthkris assigned tjiang11 Aug 1, 2017

tjiang11 added 4 commits August 1, 2017 22:39

suggested changes

a97a25d

lint

57038d9

test fix

f54195c

move isContentAudioTranslationsAvailable to oppiaPlayerService

d84cae0

seanlip approved these changes Aug 2, 2017

View reviewed changes

suggested changes

2624fee

seanlip mentioned this pull request Aug 2, 2017

Add editor for audio translations #3692

Merged

3 tasks

tjiang11 added 4 commits August 3, 2017 00:22

font awesome

9b48ae3

typo

c45aaf0

typo

4025367

merge develop

976f2dd

comma

c01bcde

seanlip added the PR: LGTM label Aug 3, 2017

tjiang11 added 2 commits August 3, 2017 11:16

fix tests

8bd9d40

temporarily remove resources_test

e026b84

seanlip merged commit 90f1f2c into oppia:develop Aug 3, 2017

tjiang11 mentioned this pull request Aug 8, 2017

Allow audio components in different languages to be added to an exploration. #3408

Closed

seanlip mentioned this pull request Aug 20, 2017

Fix issue with inter-paragraph spacing in content field being unintentionally reduced. #3775

Merged

		focus-on="<[getContentFocusLabel($index)]>">
		<span angular-html-bind="activeCard.contentHtml">

Learner view audio translations #3681

Learner view audio translations #3681

Conversation

tjiang11 commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 27, 2017 • edited Loading

Codecov Report

tjiang11 commented Jul 29, 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

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

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

seanlip commented Aug 3, 2017

seanlip commented Aug 3, 2017

codecov-io commented Jul 27, 2017 •

edited

Loading