Audio fragment can guarantee a crash if loading audio fails #1341
Assignees
Labels
Priority: Essential
This work item must be completed for its milestone.
Z-ibt
Temporary label for Ben to keep track of issues he's triaged.
Milestone
Comments
BenHenning
added
Priority: Essential
|
This error is not arising with the latest develop branch. |
|
I just this locally by entering an exploration, playing audio, turning off wifi, navigating to the next state, observing that audio tries to play but doesn't, then navigating back to the story page & back to the exploration, then back to the story page & waiting about a minute. Not sure if all of these steps are needed, but I did observe a similar crash: |
|
I noticed one thing when investigating |
BenHenning
added a commit
that referenced
this issue
Aug 13, 2020
This does a bunch of refactoring & hacky workarounds to keep asset priming support fully isolated (just a few files now need to be deleted to clean things up). This also: 1. Fixes actual asset downloading (the GCS asset path templates have changed, so priming didn't actually work anymore). 2. Disables audio file caching since we can't play audio when offline, anyway (there's a dialog that prevents this in-player). 3. Fixes #1340 and #1341 by accounting for error cases when trying to play audio. It turns out that playing audio crashes if you didn't have internet access when going into an exploration (the no connectivity dialog only appears if you lose connectivity within a lesson). There's also some issues with existing view model code that wrongly assumed audio couldn't be in a failure state at that point. This has been fixed. 4. Moves the list of topics to cache to be in the same position as the flag enabling/disabling this functionality. The download experience isn't perfect, but it's meant to be a helper for user study coordinators so that they know when lessons can be brought offline. Note that the UI aspects of this change are really hacky. This is by design--I didn't want to overcomplicate the solution, and I wanted to keep the priming changes fully isolated to make future cleanup easier. Finally, no new tests were added. I clarified in StateFragmentTest that the edge cases fixed in this PR need to have corresponding tests, but I'm actually not sure offhand how to test that audio's playing. I think this will require additional work. I prefer to push this off to #388, but I will follow up with tests if anyone wants to push back on this.
|
The robustness work above is probably true, but my focus here is to make sure the user-facing crash isn't possible. This issue is happening because AudioFragmentPresenter tries to pause audio when you play without internet connectivity. It only fails if #1340 or similar happens (audio player is in a bad state). |
BenHenning
added a commit
that referenced
this issue
Aug 17, 2020
…line support (#1636) * Show a dialog when preloading assets for offline support. This does a bunch of refactoring & hacky workarounds to keep asset priming support fully isolated (just a few files now need to be deleted to clean things up). This also: 1. Fixes actual asset downloading (the GCS asset path templates have changed, so priming didn't actually work anymore). 2. Disables audio file caching since we can't play audio when offline, anyway (there's a dialog that prevents this in-player). 3. Fixes #1340 and #1341 by accounting for error cases when trying to play audio. It turns out that playing audio crashes if you didn't have internet access when going into an exploration (the no connectivity dialog only appears if you lose connectivity within a lesson). There's also some issues with existing view model code that wrongly assumed audio couldn't be in a failure state at that point. This has been fixed. 4. Moves the list of topics to cache to be in the same position as the flag enabling/disabling this functionality. The download experience isn't perfect, but it's meant to be a helper for user study coordinators so that they know when lessons can be brought offline. Note that the UI aspects of this change are really hacky. This is by design--I didn't want to overcomplicate the solution, and I wanted to keep the priming changes fully isolated to make future cleanup easier. Finally, no new tests were added. I clarified in StateFragmentTest that the edge cases fixed in this PR need to have corresponding tests, but I'm actually not sure offhand how to test that audio's playing. I think this will require additional work. I prefer to push this off to #388, but I will follow up with tests if anyone wants to push back on this. * Lint fixes. * Introduce new caching module to simplify tests, and fix app module tests. * Lint fixes.
BenHenning
moved this to Needs Triage
in [Team] Core Learner and Mastery flows & UI Frontend - Android
BenHenning
added
the
Z-ibt
seanlip
moved this from Needs Triage
to Done
in [Team] Core Learner and Mastery flows & UI Frontend - Android
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In cases when audio fails, leaving an exploration will trigger a crash. This can be reproed by getting the app into a bad state similar to #1340 (e.g. disabling internet connectivity and then navigating back from the exploration player). The following crash occurs:
The text was updated successfully, but these errors were encountered: