Fix #123 & #116: Functional AudioPlayerController #133
Conversation
|
I tried creating a test based on UserAppHistoryControllerTest, but I can't seem to run either. I get a compiler error "Dagger/MissingBinding" @org.oppia.util.logging.EnableConsoleLog java.lang.Boolean cannot be provided without an @Provides-annotated method". I know the LoggerModule has a provides so I'm not sure what's wrong. Also if you could suggest what kind of things I should be testing for, that would be great. |
| import org.robolectric.annotation.Config | ||
| import javax.inject.Singleton | ||
|
|
||
| /** Tests for [UserAppHistoryController]. */ |
There was a problem hiding this comment.
Update doc string to correspond to the correct component being tested here.
|
|
||
| @Test | ||
| fun test() { | ||
| //TODO |
There was a problem hiding this comment.
I suggest starting with the easiest happy path case (usually not errors) to get a test to work. After that, you can copy & paste that to test other cases to be considered.
For this, I suggest starting with: testAudioPlayer_successfulInitialize_reportsSuccessfulInit() sets up media player so that the init will succeed, initializes the controller, and checks that the report LiveData is correct.
See http://robolectric.org/javadoc/4.0/org/robolectric/shadows/ShadowMediaPlayer.html for how to fake the media player. I think you'll need to do use ShadowMediaPlayer.setCreateListener to get access to the ShadowMediaPlayer corresponding to the instance of MediaPlayer you're testing. See http://robolectric.org/extending/ for a background on Shadows.
|
|
||
| class AudioPlayerController @Inject constructor(val context: Context) { | ||
|
|
||
| class PlayStatus(val type: String, val value: Int) |
There was a problem hiding this comment.
Might an enum be a better fit for the play status? Strings can be difficult to use since they aren't strongly typed, meaning if they change in one places other places will break at runtime rather than compile-time.
What purposes is the value? Could 'PlayStatus' just become an enum to represent the different states?
There was a problem hiding this comment.
Or, maybe a data object for an enum on status and other properties that should be passed to the UI could be a good fit (e.g. for duration, position, etc.)
There was a problem hiding this comment.
Discussed in meeting: since LiveData observers can miss updates (e.g. during configuration changes), we need to model this based on absolute state. I think that means we need to include position & duration each time.
| import java.util.concurrent.TimeUnit | ||
| import javax.inject.Inject | ||
|
|
||
| class AudioPlayerController @Inject constructor(val context: Context) { |
There was a problem hiding this comment.
Please add documentation for this controller. It doesn't need to be super detailed, but consider what sort of information you would like to know in order to understand how to use this controller correctly if you came across this code and knew nothing about playing audio in the Oppia app. How to initialize, play, deinitialize, and caveats would be good things to include.
| private var prepared = false | ||
| private val playState = MutableLiveData<PlayStatus>() | ||
|
|
||
| fun initializeMediaPlayer (stringUri: String) { |
There was a problem hiding this comment.
Nit: no space between function name and parameter '(' per https://google.github.io/styleguide/javaguide.html#s4.6.2-horizontal-whitespace.
| private val playState = MutableLiveData<PlayStatus>() | ||
|
|
||
| fun initializeMediaPlayer (stringUri: String) { | ||
| if (mediaPlayer == null) mediaPlayer = MediaPlayer() |
There was a problem hiding this comment.
I suggest we instead lazy initialize media player. Kotlin solves this exact pattern for us:
private val mediaPlayer: MediaPlayer by lazy { MediaPlayer() }| private var prepared = false | ||
| private val playState = MutableLiveData<PlayStatus>() | ||
|
|
||
| fun initializeMediaPlayer (stringUri: String) { |
There was a problem hiding this comment.
If we have an initialize, we probably also need a deinitialize. Can you add a method for that?
|
|
||
| private fun startUpdatingSeekBar() { | ||
| if (executor == null) executor = Executors.newSingleThreadScheduledExecutor() | ||
| executor?.scheduleAtFixedRate({ updateSeekBar() }, 0, 1000, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
Who removes this schedule? Does MediaPlayer not provide a way for us to monitor the progress?
If not, we should use the background Kotlin dispatcher used by the DataProviders object. Unfortunately there isn't as nice of an API for scheduling at a fixed rate in Kotlin, but with delay() you can get close (and get some additional benefits I'll mention later):
fun scheduleNextSeekBarUpdate() {
nextUpdateJob = backgroundDispatcherScope.launch {
delay(TimeUnit.SECONDS.toMillis(1)) // TODO: move delay to constant
updateSeekBar()
scheduleNextSeekBarUpdate()
}
// NB: nextUpdateJob will need to be locked since it can be changed on multiple threads
}Then, you can cancel the nextUpdateJob both during completion (like you similarly do with the executor service in stopUpdatingSeekBar below) and during deinitialization (which presumably is called by the UI when the user navigates away from the activity).
There was a problem hiding this comment.
From discussion & following up: I think we actually want a custom MutableLiveData class to schedule/stop in onActive/onInactive to ensure that we're only updating at times that observers actually care about it.
|
|
||
| private fun startUpdatingSeekBar() { | ||
| if (executor == null) executor = Executors.newSingleThreadScheduledExecutor() | ||
| executor?.scheduleAtFixedRate({ updateSeekBar() }, 0, 1000, TimeUnit.MILLISECONDS) |
There was a problem hiding this comment.
From discussion & following up: I think we actually want a custom MutableLiveData class to schedule/stop in onActive/onInactive to ensure that we're only updating at times that observers actually care about it.
|
|
||
| class AudioPlayerController @Inject constructor(val context: Context) { | ||
|
|
||
| class PlayStatus(val type: String, val value: Int) |
There was a problem hiding this comment.
Discussed in meeting: since LiveData observers can miss updates (e.g. during configuration changes), we need to model this based on absolute state. I think that means we need to include position & duration each time.
|
Regarding your error, I accidentally forgot some of the test-only bindings in the test you referenced, but they're there now: https://github.com/oppia/oppia-android/blob/develop/domain/src/test/java/org/oppia/domain/UserAppHistoryControllerTest.kt#L209. You can copy these into your test and it should work. |
|
Started a new pull request #149 to make getting changes in from develop easier |
Explanation
I didn't separate #123 & #116 because I already finished basic functionality for the AudioPlayerController.
Checklist