Preloading of audio and bandwidth confirmation #3727

tjiang11 · 2017-08-09T03:54:56Z

Users are asked to confirm bandwidth usage when trying to download audio
Audio is loaded and cached after confirming bandwidth usage

seanlip

Thanks, @tjiang11! This looks pretty solid technically, but I have some questions about the intended UX.

seanlip · 2017-08-09T07:20:22Z

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

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

+    States.prototype.getAllAudioTranslations = function() {


Remember to write tests, please :)

seanlip · 2017-08-09T08:21:15Z

data/explorations/audio_test/audio_test.yaml

@@ -17,11 +17,11 @@ states:
      audio_translations:
        en:
          filename: 'test_audio_2_en.mp3'
-          file_size_bytes: 2
+          file_size_bytes: 100


Consider using the actual values for the files (59 in this case; ditto below). (I noticed this when trying to do a sanity check of the 1.13 MB.)

Yeah sorry I kept thinking in kB when we were using bytes so these values were way too low, using actual values now

seanlip · 2017-08-09T08:22:30Z

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

+  'ExplorationPlayerStateService', 'UrlInterpolationService',
+  function($modal, explorationContextService, AssetsBackendApiService,
+      ExplorationPlayerStateService, UrlInterpolationService) {
+    var NUM_BYTES_IN_MB = 1000;


Actually, there are (1 << 20) bytes in an MB...

seanlip · 2017-08-09T08:31:32Z

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

-            // and subsequently for confirmation to use bandwidth 
-            // to download audio files.
+            if (!AudioPreloaderService.hasPreloaded()) {
+              if (getCurrentAudioTranslation()) {


In what circumstance does this condition evaluate to true? I get this popup modal as soon as I try to play any one file...

Also, I have a general question about this. The options available to me as a learner seem to be preload-or-nothing, and the former option downloads all the audio in the entire exploration. Do I not get the option to just download the specific audio files I need on an as-needed basis? (I looked at the doc, but I'm not sure that this part was written out in detail...or maybe I misunderstood it.)

It should evaluate to true if there is an audio translation available in the current language.

Yeah, I assumed learners would either be willing to use the necessary bandwidth for all audio or decline to use any bandwidth. But I suppose there is a middleground where some learners may need help with only one specific piece of content and also be restrictive in their use of bandwidth.

Yeah sorry, it was never in the design doc. How about this: clicking "continue" will only download the current file. But a checkbox will also be present saying "Download all audio for this exploration (XX MB)". And preloading will occur only if the learner checks that. Does that sound okay?

Ah, interesting. On reading I had interpreted "getCurrentAudioTranslation()" as "get the audio translation that's currently playing". I wonder if we could change the name to make the intended meaning clearer...

And yep that sounds good! In the non-preload case, what about subsequent downloads, though? Will there be a modal each time?

If they preloaded, then there shouldn't be any more modals. But they'll still see modals each time if they decide to only download that specific file.

Because I suppose it'd be better to be extra cautious if they're being restrictive on their bandwidth--?

Changed the function name to getAudioTranslationInCurrentLanguage()

OK, all this sgtm. Thanks!

seanlip · 2017-08-09T08:32:58Z

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

+    var _hasPreloaded = false;
+    var _excludedFilename = null;
+
+    var _preload = function() {


Perhaps _preloadAllAudioFiles

seanlip · 2017-08-09T08:33:36Z

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

+      ExplorationPlayerStateService, UrlInterpolationService) {
+    var NUM_BYTES_IN_MB = 1000;
+    var _hasPreloaded = false;
+    var _excludedFilename = null;


Maybe add one-line comments above these. Without context, I don't know what e.g. _excludedFilename refers to or represents.

seanlip · 2017-08-09T08:34:10Z

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

+              $scope, $modalInstance,
+              ExplorationPlayerStateService, AudioPreloaderService) {
+            $scope.totalFileSizeOfAllAudioTranslationsBytes =
+              _calculateTotalFileSize(


Maybe define this as a method on the exploration object? (Please add tests too.)

seanlip · 2017-08-09T08:35:20Z

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

+      });
+    };
+
+    var _calculateTotalFileSize = function(audioTranslations) {


Add unit to the function name, e.g. _calculateTotalFileSizeMB

seanlip · 2017-08-09T08:36:10Z

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

+          function(
+              $scope, $modalInstance,
+              ExplorationPlayerStateService, AudioPreloaderService) {
+            $scope.totalFileSizeOfAllAudioTranslationsBytes =


The units in this name seem misleading.

Changed to MB

seanlip · 2017-08-09T08:36:40Z

assets/i18n/en.json

    "I18N_ONE_SUBSCRIBER_TEXT": "You have 1 subscriber.",
    "I18N_PLAYER_AUDIO_LANGUAGE": "Language",
    "I18N_PLAYER_AUDIO_MIGHT_NOT_MATCH_TEXT": "Audio might not fully match text",
    "I18N_PLAYER_AUDIO_NOT_AVAILABLE_IN": "Not available in <[languageDescription]>",
    "I18N_PLAYER_AUDIO_TRANSLATION_SETTINGS": "Audio Translation Settings",
    "I18N_PLAYER_BACK": "Back",
    "I18N_PLAYER_BACK_TO_COLLECTION": "Back to collection",
+    "I18N_PLAYER_BANDWIDTH_USAGE_WARNING_MODAL_BODY": "This exploration contains <b><[filesize]>MB</b> of audio. Continue downloading?",


Consider calling the variable fileSizeMB

codecov-io · 2017-08-09T22:56:23Z

Codecov Report

Merging #3727 into develop will decrease coverage by 0.02%.
The diff coverage is 34.02%.

@@             Coverage Diff             @@
##           develop    #3727      +/-   ##
===========================================
- Coverage    45.06%   45.04%   -0.03%     
===========================================
  Files          264      265       +1     
  Lines        20427    20510      +83     
  Branches      3174     3184      +10     
===========================================
+ Hits          9205     9238      +33     
- Misses       11222    11272      +50

Impacted Files	Coverage Δ
...ploration_player/AudioTranslationManagerService.js	`2.5% <0%> (-0.07%)`	⬇️
...ev/head/pages/exploration_player/PlayerServices.js	`1.69% <0%> (-0.02%)`	⬇️
...pages/exploration_player/AudioControlsDirective.js	`2.04% <0%> (-0.46%)`	⬇️
...ead/domain/exploration/ExplorationObjectFactory.js	`60.27% <100%> (+5.58%)`	⬆️
...omain/exploration/AudioTranslationObjectFactory.js	`100% <100%> (ø)`	⬆️
...lates/dev/head/services/AssetsBackendApiService.js	`60% <100%> (+11.06%)`	⬆️
...dev/head/domain/exploration/StatesObjectFactory.js	`68.75% <100%> (+2.99%)`	⬆️
.../pages/exploration_player/AudioPreloaderService.js	`2.63% <2.63%> (ø)`
...es/exploration_editor/EditorNavigationDirective.js	`2.32% <0%> (-0.06%)`	⬇️

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 8410ed4...4c3771c. Read the comment docs.

seanlip · 2017-08-09T23:08:33Z

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

+    // Whether all audio files have been preloaded in the exploration.
+    var _hasPreloaded = false;
+
+    // This is a file to exclude while preloading all audio translations 


Just a thought: if you're supporting the "load each file individually" case, should you be maintaining a list of exclusions here instead? Maybe exposing a markFileAsAlreadyLoaded() function would be a way to do this, but if there's some way we could do something like getAlreadyLoadedFiles() from the audio caching service then that might be even better since we don't have to keep track of this in multiple places.

I feel like it shouldn't make a difference, because AssetsBackendApiService already maintains its own cache, and AudioPreloaderService just fills up the cache with any audio that hasn't been loaded yet. If a file is already in the cache, it won't be fetched for again via the check in AssetsBackendApiService.

Oh, OK. So do we even need _excludedFilename then?

Well, the reason why I had to put that was because otherwise there are two simultaneous requests for the same file, so AssetsBackendApiService can't use the cache check. Thinking about it more though, if the bandwidth is low enough, it might be the case that a learner might go on to another audio while it's still in the process of preloading. So maybe AssetsBackendApiService could maintain a list of files currently being preloaded?

tjiang11 · 2017-08-10T01:21:31Z

(don't review this yet)

tjiang11 · 2017-08-10T03:56:09Z

Ready for another pass @seanlip

tjiang11 · 2017-08-10T03:57:34Z

Going to be adding some more comments shortly, since I realized it's rather windy..

seanlip · 2017-08-10T04:06:19Z

assets/i18n/qqq.json

 	"I18N_ONE_SUBSCRIBER_TEXT": "Text displayed under the subscribers tab in creator dashboard. If the creator has one subscriber, this text is displayed which informs him/her about the same.",
 	"I18N_PLAYER_AUDIO_LANGUAGE": "Text displayed in the audio translation settings modal, asking the learner to pick what language they want to listen to audio translations in.",
 	"I18N_PLAYER_AUDIO_MIGHT_NOT_MATCH_TEXT": "Text displayed under the audio controls to the learner when the audio translation for the current language is flagged as needing an update by the creator.",
 	"I18N_PLAYER_AUDIO_NOT_AVAILABLE_IN": "Text displayed in a tooltip over the speaker icon to play audio when there is no audio available in the selected language.",
 	"I18N_PLAYER_AUDIO_TRANSLATION_SETTINGS": "Title displayed at the top of the audio translation settings modal in the learner view.",
 	"I18N_PLAYER_BACK": "Text read to users with screenreaders when they navigate through an exploration. - This labels the leftward-pointing arrow that is used to go backward by one card in the exploration.\n{{Identical|Back}}",
 	"I18N_PLAYER_BACK_TO_COLLECTION": "Text shown to users after they complete an exploration in a collection. - This labels the link that is used to return back to the home page of the collection the user is currently exploring",
+	"I18N_PLAYER_BANDWIDTH_USAGE_WARNING_MODAL_BODY": "Text displayed in the body of the modal shown when the user clicks to play audio for the first time asking if they would like to use bandwidth to download audio.",
+	"I18N_PLAYER_BANDWIDTH_USAGE_WARNING_MODAL_DOWNLOAD_ALL_AUDIO": "Text displayed by the checkbox of the modal shown when the user clicks to play audio for the first time asking if they would like to use bandwidth to download audio. Checking the assoicated checkbox signifies that the user intends to predownload all audio translation in the exploration of the selected language.",


predownload all audio translations ... (pluralize "translations")

seanlip · 2017-08-10T04:07:04Z

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

+        var allAudioTranslations =
+          this.states.getAllAudioTranslations(languageCode);
+        allAudioTranslations.map(function(audioTranslation) {
+          totalFileSizeBytes += audioTranslation.fileSizeBytes;


Optional: Might as well reuse audioTranslation.getFileSizeMB()? At the very least you don't then duplicate NUM_BYTES_IN_MB across the codebase.

seanlip · 2017-08-10T04:10:56Z

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

@@ -86,7 +106,7 @@ oppia.directive('audioControls', [
                // immediately start playing the newly requested audio.
                loadAndPlayAudioTranslation();
              }
-            }
+            }        


Please remove trailing whitespace

seanlip · 2017-08-10T04:13:32Z

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

+              if (!AudioPreloaderService.hasPreloadedLanguage(
+                    getCurrentAudioLanguageCode()) &&
+                    !isCached(audioTranslation)) {
+                AudioPreloaderService.excludeFile(


Mmm ... I don't really see why the audio controls directive cares about all these details. Shouldn't the preloader (or even the backend api service) handle it internally? I assume the issue is that the current translation is in the middle of being loaded, but presumably the backend api service could maintain some sort of list detailing which requests are currently in flight?

Ah okay, yeah that's what I suggested in the earlier lost comment--will add that

seanlip · 2017-08-10T04:14:06Z

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

+  function($modal, explorationContextService, AssetsBackendApiService,
+      ExplorationPlayerStateService, UrlInterpolationService,
+      AudioTranslationManagerService) {
+    // Mapping from language code to boolean indicating if it has 


This looks like an array rather than a mapping to me...

…to prevent duplicate requests

seanlip

LGTM. Thanks @tjiang11!

* upstream/develop: (28 commits) Preloading of audio and bandwidth confirmation (oppia#3727) Fix oppia#3693: Deprecate ClassifierDataModel and update ClassifierTrainingJobModel (oppia#3734) Fix part of oppia#3453: Removed jinja template in editor_navigation_directive (oppia#3732) Eliminates stray tick mark (oppia#3731) Fix oppia#3453: Removed jinja template in collection editor navigation bar directive (oppia#3723) Fix oppia#3726: prevent graph SVG from overlapping modal footer buttons. (oppia#3728) address sean review comments Add a one-off job to list which explorations use gadgets; remove old fallbacks job. (oppia#3717) fix linting remove jinja template in create_activity_modal 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) ...

* upstream/develop: fix variable name Preloading of audio and bandwidth confirmation (oppia#3727) Fix oppia#3693: Deprecate ClassifierDataModel and update ClassifierTrainingJobModel (oppia#3734) Fix part of oppia#3453: Removed jinja template in editor_navigation_directive (oppia#3732) use userIsLoggedIn instead of hasFullyRegistered variable address Sean review comments remove new line removed jinja template logics in search_results_directive Eliminates stray tick mark (oppia#3731) Fix oppia#3453: Removed jinja template in collection editor navigation bar directive (oppia#3723) Fix oppia#3726: prevent graph SVG from overlapping modal footer buttons. (oppia#3728) address sean review comments fix linting remove jinja template in create_activity_modal

tjiang11 added 6 commits August 8, 2017 21:28

Add preloading

a6c6808

bandwidth confirmation

6f44ba9

internationalized strings

a4eb235

lint

34b2946

lint

d9e144e

remove todo

22b8f0c

tjiang11 requested a review from seanlip August 9, 2017 03:55

seanlip requested changes Aug 9, 2017

View reviewed changes

tjiang11 added 2 commits August 9, 2017 18:52

suggested changes

880a2e8

lint

375aae4

seanlip reviewed Aug 9, 2017

View reviewed changes

Option for on-demand loading

0f469f9

tjiang11 added 2 commits August 10, 2017 00:15

Some comments and bug fix - error on switching to unavailable language

9178773

renaming

858a78a

seanlip reviewed Aug 10, 2017

View reviewed changes

tjiang11 added 4 commits August 10, 2017 17:44

suggested changes

d05292b

Keep track of audio files being requested in AssetsBackendApiService …

fb3c559

…to prevent duplicate requests

lint

744416c

renaming

4c3771c

seanlip approved these changes Aug 10, 2017

View reviewed changes

seanlip added the PR: LGTM label Aug 10, 2017

seanlip merged commit b2ccf54 into oppia:develop Aug 11, 2017

Preloading of audio and bandwidth confirmation #3727

Preloading of audio and bandwidth confirmation #3727

Conversation

tjiang11 commented Aug 9, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 9, 2017 • edited Loading

Codecov Report

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 Aug 10, 2017

tjiang11 commented Aug 10, 2017

tjiang11 commented Aug 10, 2017 • edited Loading

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

codecov-io commented Aug 9, 2017 •

edited

Loading

tjiang11 commented Aug 10, 2017 •

edited

Loading