[APT-10727] Fix seek handling

kschults · kschults · commit 2523b12aedc5 · 2024-12-03T11:25:47.000-05:00
Previously, the progress position was not updating after a seek until the next playback poll. This caused problems with consumers attempting to track listening because it was not possible to accurately detect a seek. This now updates the position when the seek finishes, rather than waiting for the next poll for the position to catch up.

Also updates Reducer tests to ensure that we are chaining events properly.
diff --git a/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt b/Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt
@@ -195,9 +195,12 @@ internal object Reducer {
                             seekTarget = action.seekPositionTarget)
                     } else MediaControlState()
 
+                    // When we finish a seek, update our progress to the target
+                    val newProgress = if (action.isSeeking) { playbackInfo.progress.positionInDuration } else { action.seekPositionTarget }
                     oldState.copy(playbackInfo = playbackInfo.copy(
-                        controlState = controlState))
-                        .apply { debugState = newDebug }
+                        controlState = controlState,
+                        progress = playbackInfo.progress.copy(positionInDuration = newProgress)
+                    )).apply { debugState = newDebug }
                 }
 
                 is FastForwardAction -> {
diff --git a/Armadillo/src/main/java/com/scribd/armadillo/actions/Action.kt b/Armadillo/src/main/java/com/scribd/armadillo/actions/Action.kt
@@ -58,7 +58,7 @@ internal data class UpdateProgressAction(val isUpdated: Boolean, val currentChap
 // Media Control State updates - updates control info in ArmadilloState / exposes player intent to users
 
 /** For a beginning or end of a seek (discontinuity). when isSeeking is becoming false, the seek action is resolved. */
-internal data class SeekAction(val isSeeking: Boolean, val seekPositionTarget: Milliseconds?) : Action {
+internal data class SeekAction(val isSeeking: Boolean, val seekPositionTarget: Milliseconds) : Action {
     override val name = "Seeking: $isSeeking"
 }
 
diff --git a/Armadillo/src/main/java/com/scribd/armadillo/playback/PlayerEventListener.kt b/Armadillo/src/main/java/com/scribd/armadillo/playback/PlayerEventListener.kt
@@ -16,6 +16,7 @@ import com.scribd.armadillo.actions.SeekAction
 import com.scribd.armadillo.actions.UpdateProgressAction
 import com.scribd.armadillo.di.Injector
 import com.scribd.armadillo.models.PlaybackState
+import com.scribd.armadillo.time.milliseconds
 import javax.inject.Inject
 
 /**
@@ -46,9 +47,9 @@ internal class PlayerEventListener @Inject constructor(private val context: Cont
         stateModifier.dispatch(LoadingAction(isLoading))
     }
 
-    override fun onPositionDiscontinuity(reason: Int) {
+    override fun onPositionDiscontinuity(oldPosition: Player.PositionInfo, newPosition: Player.PositionInfo, reason: Int) {
         Log.v(TAG, "onPositionDiscontinuity --- reason: $reason")
-        stateModifier.dispatch(SeekAction(false, null))
+        stateModifier.dispatch(SeekAction(false, newPosition.contentPositionMs.milliseconds))
     }
 
     override fun onRepeatModeChanged(repeatMode: Int) = Unit
diff --git a/Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt b/Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt
@@ -30,6 +30,7 @@ import com.scribd.armadillo.models.DownloadState
 import com.scribd.armadillo.models.DrmState
 import com.scribd.armadillo.models.DrmType
 import com.scribd.armadillo.models.InternalState
+import com.scribd.armadillo.models.MediaControlState
 import com.scribd.armadillo.models.PlaybackProgress
 import com.scribd.armadillo.models.PlaybackState
 import com.scribd.armadillo.time.milliseconds
@@ -157,7 +158,7 @@ class ReducerTest {
         val identicalActions = listOf(MockModels.progressAction(), MockModels.progressAction())
         var appState2 = MockModels.appState()
         identicalActions.forEach {
-            appState2 = Reducer.reduce(appState2, it)
+            appState2 = Reducer.reduce(appState1, it)
         }
 
         assertThat(appState1).isEqualTo(appState2)
@@ -200,8 +201,8 @@ class ReducerTest {
 
     @Test
     fun reduce_skipNext_clearsOtherSeekControls() {
-        Reducer.reduce(MockModels.appState(), SkipPrevAction(100.milliseconds))
-        val secondSeek = Reducer.reduce(MockModels.appState(), SkipNextAction(4000.milliseconds))
+        val firstState = Reducer.reduce(MockModels.appState(), SkipPrevAction(100.milliseconds))
+        val secondSeek = Reducer.reduce(firstState, SkipNextAction(4000.milliseconds))
 
         assertThat(secondSeek.playbackInfo?.controlState?.isFastForwarding ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isRewinding ?: true).isFalse
@@ -212,8 +213,8 @@ class ReducerTest {
 
     @Test
     fun reduce_skipPrev_clearsOtherSeekControls() {
-        Reducer.reduce(MockModels.appState(), SkipNextAction(100.milliseconds))
-        val secondSeek = Reducer.reduce(MockModels.appState(), SkipPrevAction(4000.milliseconds))
+        val firstState = Reducer.reduce(MockModels.appState(), SkipNextAction(100.milliseconds))
+        val secondSeek = Reducer.reduce(firstState, SkipPrevAction(4000.milliseconds))
 
         assertThat(secondSeek.playbackInfo?.controlState?.isFastForwarding ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isRewinding ?: true).isFalse
@@ -224,8 +225,8 @@ class ReducerTest {
 
     @Test
     fun reduce_fastForward_clearsOtherSeekControls() {
-        Reducer.reduce(MockModels.appState(), RewindAction(100.milliseconds))
-        val secondSeek = Reducer.reduce(MockModels.appState(), FastForwardAction(4000.milliseconds))
+        val firstState = Reducer.reduce(MockModels.appState(), RewindAction(100.milliseconds))
+        val secondSeek = Reducer.reduce(firstState, FastForwardAction(4000.milliseconds))
 
         assertThat(secondSeek.playbackInfo?.controlState?.isPrevChapter ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isRewinding ?: true).isFalse
@@ -236,8 +237,8 @@ class ReducerTest {
 
     @Test
     fun reduce_rewind_clearsOtherSeekControls() {
-        Reducer.reduce(MockModels.appState(), FastForwardAction(100.milliseconds))
-        val secondSeek = Reducer.reduce(MockModels.appState(), RewindAction(4000.milliseconds))
+        val firstState = Reducer.reduce(MockModels.appState(), FastForwardAction(100.milliseconds))
+        val secondSeek = Reducer.reduce(firstState, RewindAction(4000.milliseconds))
 
         assertThat(secondSeek.playbackInfo?.controlState?.isPrevChapter ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isFastForwarding ?: true).isFalse
@@ -246,22 +247,33 @@ class ReducerTest {
         assertThat(secondSeek.playbackInfo?.controlState?.isSeeking).isTrue
     }
 
+    @Test
+    fun reduce_seekStart_setsSeekControls() {
+        val initialState = MockModels.appState()
+        val seekState = Reducer.reduce(initialState, SeekAction(true, 300.milliseconds))
+
+        assertThat(seekState.playbackInfo?.progress).isEqualTo(initialState.playbackInfo?.progress)
+        assertThat(seekState.playbackInfo?.controlState?.isSeeking).isTrue
+        assertThat(seekState.playbackInfo?.controlState?.seekTarget).isEqualTo(300.milliseconds)
+    }
+
     @Test
     fun reduce_seekFinish_allSeekControlsFalse() {
-        Reducer.reduce(MockModels.appState(), FastForwardAction(100.milliseconds))
-        val secondSeek = Reducer.reduce(MockModels.appState(), SeekAction(false, null))
+        val firstState = Reducer.reduce(MockModels.appState(), FastForwardAction(100.milliseconds))
+        val secondSeek = Reducer.reduce(firstState, SeekAction(false, 300.milliseconds))
 
         assertThat(secondSeek.playbackInfo?.controlState?.isFastForwarding ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isRewinding ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isPrevChapter ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isNextChapter ?: true).isFalse
         assertThat(secondSeek.playbackInfo?.controlState?.isSeeking).isFalse
+        assertThat(secondSeek.playbackInfo?.progress?.positionInDuration).isEqualTo(300.milliseconds)
     }
 
     @Test
     fun reduce_playbackStopDuringSeek_clearSeekControls() {
-        Reducer.reduce(MockModels.appState(), FastForwardAction(100000.milliseconds))
-        val state = Reducer.reduce(MockModels.appState(), PlayerStateAction(PlaybackState.NONE))
+        val firstState = Reducer.reduce(MockModels.appState(), FastForwardAction(100000.milliseconds))
+        val state = Reducer.reduce(firstState, PlayerStateAction(PlaybackState.NONE))
 
         assertThat(state.playbackInfo?.controlState?.isStopping).isTrue
         assertThat(state.playbackInfo?.controlState?.isFastForwarding ?: true).isFalse
diff --git a/RELEASE.md b/RELEASE.md
@@ -1,5 +1,9 @@
 # Project Armadillo Release Notes
 
+## 1.7.0
+- Fixes the playback progress being incorrect between a seek finishing and the next playback update. The progress position now updates 
+  immediately to the seek target when the seek finishes
+
 ## 1.6.9
 - Improves the crash fix from 1.6.8, so that even retries on EncryptedSharedPreferences does not crash.
 
diff --git a/gradle.properties b/gradle.properties
@@ -13,7 +13,7 @@ org.gradle.jvmargs=-Xmx1536m
 # org.gradle.parallel=true
 PACKAGE_NAME=com.scribd.armadillo
 GRADLE_PLUGIN_VERSION=7.2.0
-LIBRARY_VERSION=1.6.9
+LIBRARY_VERSION=1.7.0
 EXOPLAYER_VERSION=2.19.1
 RXJAVA_VERSION=2.2.4
 RXANDROID_VERSION=2.0.1

Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,7 @@ internal data class UpdateProgressAction(val isUpdated: Boolean, val currentChap`
`58`	`58`	`// Media Control State updates - updates control info in ArmadilloState / exposes player intent to users`
`59`	`59`
`60`	`60`	`/** For a beginning or end of a seek (discontinuity). when isSeeking is becoming false, the seek action is resolved. */`
`61`		`-internal data class SeekAction(val isSeeking: Boolean, val seekPositionTarget: Milliseconds?) : Action {`
	`61`	`+internal data class SeekAction(val isSeeking: Boolean, val seekPositionTarget: Milliseconds) : Action {`
`62`	`62`	`override val name = "Seeking: $isSeeking"`
`63`	`63`	`}`
`64`	`64`