Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[APT-10128] Update Reducer logic to copy seek position to playbackinfo #33

Closed
wants to merge 3 commits into from
Closed

[APT-10128] Update Reducer logic to copy seek position to playbackinfo #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ae21b2
[APT-10128] Update Reducer logic to copy seek target to playback info…
griffinfscribd Jun 4, 2024
b5a1ea4
[APT-10128] Bump library version
griffinfscribd Jun 4, 2024
4418f8c
[APT-10128] Remove nullability on some jump to position actions
griffinfscribd Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion Armadillo/src/main/java/com/scribd/armadillo/Reducer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,14 +175,24 @@ internal object Reducer {
isSeeking = action.isSeeking,
seekTarget = action.seekPositionTarget)
} else MediaControlState()

val progress = if (action.seekPositionTarget != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For most of these, I think seekPositionTarget should really be non-null. It's only used in SeekAction to resolve the end of a seek. Can you make it non-null for all the other actions where it's never actually null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not notice that. Sure, I can make that change.

playbackInfo.progress.copy(
positionInDuration = action.seekPositionTarget,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the question to ask is "Should we consider the user at the new position when they start a discontinuous action like seeking or jumping?" I feel like the answer should be "no", but I'm willing to be convinced it makes sense.

How does the scrubber/UI get updated if you jump while paused? Can we hitch the progress update into whatever makes that update?

currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.seekPositionTarget))
} else {
playbackInfo.progress
}
oldState.copy(playbackInfo = playbackInfo.copy(
progress = progress,
controlState = controlState))
.apply { debugState = newDebug }
}
is FastForwardAction -> {
val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action)
oldState.copy(playbackInfo = playbackInfo.copy(
progress = playbackInfo.progress.copy(
positionInDuration = action.seekPositionTarget,
currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.seekPositionTarget)),
controlState = MediaControlState(
isSeeking = true,
isFastForwarding = true,
Expand All @@ -192,6 +202,9 @@ internal object Reducer {
is RewindAction -> {
val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action)
oldState.copy(playbackInfo = playbackInfo.copy(
progress = playbackInfo.progress.copy(
positionInDuration = action.seekPositionTarget,
currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.seekPositionTarget)),
controlState = MediaControlState(
isSeeking = true,
isRewinding = true,
Expand All @@ -201,6 +214,9 @@ internal object Reducer {
is SkipNextAction -> {
val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action)
oldState.copy(playbackInfo = playbackInfo.copy(
progress = playbackInfo.progress.copy(
positionInDuration = action.seekPositionTarget,
currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.seekPositionTarget)),
controlState = MediaControlState(
isSeeking = true,
isNextChapter = true,
Expand All @@ -210,6 +226,9 @@ internal object Reducer {
is SkipPrevAction -> {
val playbackInfo = oldState.playbackInfo ?: throw ActionBeforeSetup(action)
oldState.copy(playbackInfo = playbackInfo.copy(
progress = playbackInfo.progress.copy(
positionInDuration = action.seekPositionTarget,
currentChapterIndex = playbackInfo.audioPlayable.getChapterIndexAtOffset(action.seekPositionTarget)),
controlState = MediaControlState(
isSeeking = true,
isPrevChapter = true,
Expand Down
8 changes: 4 additions & 4 deletions Armadillo/src/main/java/com/scribd/armadillo/actions/Action.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,19 +61,19 @@ internal data class SeekAction(val isSeeking: Boolean, val seekPositionTarget: M
override val name = "Seeking: $isSeeking"
}

internal data class FastForwardAction(val seekPositionTarget: Milliseconds?) : Action {
internal data class FastForwardAction(val seekPositionTarget: Milliseconds) : Action {
override val name = "FastForwardAction: $seekPositionTarget"
}

internal data class RewindAction(val seekPositionTarget: Milliseconds?) : Action {
internal data class RewindAction(val seekPositionTarget: Milliseconds) : Action {
override val name = "RewindAction: $seekPositionTarget"
}

internal data class SkipNextAction(val seekPositionTarget: Milliseconds?) : Action {
internal data class SkipNextAction(val seekPositionTarget: Milliseconds) : Action {
override val name = "SkipNextAction: $seekPositionTarget"
}

internal data class SkipPrevAction(val seekPositionTarget: Milliseconds?) : Action {
internal data class SkipPrevAction(val seekPositionTarget: Milliseconds) : Action {
override val name = "SkipNextAction: $seekPositionTarget"
}

Expand Down
64 changes: 64 additions & 0 deletions Armadillo/src/test/java/com/scribd/armadillo/ReducerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,70 @@ class ReducerTest {
assertThat(newState.playbackInfo!!.playbackSpeed).isEqualTo(PLAYBACK_SPEED)
}

@Test
fun reduce_seek_updatesPlaybackAndControlState() {
val targetDistance = 10.milliseconds
val newState = Reducer.reduce(MockModels.appState(), SeekAction(true, targetDistance))
assertThat(newState.playbackInfo!!.progress.positionInDuration).isEqualTo(targetDistance)
assertThat(newState.playbackInfo!!.progress.currentChapterIndex).isEqualTo(0)

val controlState = newState.playbackInfo!!.controlState
assertThat(controlState.isSeeking).isTrue
assertThat(controlState.seekTarget).isEqualTo(targetDistance)
}

@Test
fun reduce_fastForward_updatesPlaybackAndControlState() {
val targetDistance = 10.milliseconds
val newState = Reducer.reduce(MockModels.appState(), FastForwardAction(targetDistance))
assertThat(newState.playbackInfo!!.progress.positionInDuration).isEqualTo(targetDistance)
assertThat(newState.playbackInfo!!.progress.currentChapterIndex).isEqualTo(0)

val controlState = newState.playbackInfo!!.controlState
assertThat(controlState.isSeeking).isTrue
assertThat(controlState.seekTarget).isEqualTo(targetDistance)
assertThat(controlState.isFastForwarding).isTrue
}

@Test
fun reduce_rewind_updatesPlaybackAndControlState() {
val targetDistance = 10.milliseconds
val newState = Reducer.reduce(MockModels.appState(), RewindAction(targetDistance))
assertThat(newState.playbackInfo!!.progress.positionInDuration).isEqualTo(targetDistance)
assertThat(newState.playbackInfo!!.progress.currentChapterIndex).isEqualTo(0)

val controlState = newState.playbackInfo!!.controlState
assertThat(controlState.isSeeking).isTrue
assertThat(controlState.seekTarget).isEqualTo(targetDistance)
assertThat(controlState.isRewinding).isTrue
}

@Test
fun reduce_skipNext_updatesPlaybackAndControlState() {
val targetDistance = 10.milliseconds
val newState = Reducer.reduce(MockModels.appState(), SkipNextAction(targetDistance))
assertThat(newState.playbackInfo!!.progress.positionInDuration).isEqualTo(targetDistance)
assertThat(newState.playbackInfo!!.progress.currentChapterIndex).isEqualTo(0)

val controlState = newState.playbackInfo!!.controlState
assertThat(controlState.isSeeking).isTrue
assertThat(controlState.seekTarget).isEqualTo(targetDistance)
assertThat(controlState.isNextChapter).isTrue
}

@Test
fun reduce_skipPrev_updatesPlaybackAndControlState() {
val targetDistance = 10.milliseconds
val newState = Reducer.reduce(MockModels.appState(), SkipPrevAction(targetDistance))
assertThat(newState.playbackInfo!!.progress.positionInDuration).isEqualTo(targetDistance)
assertThat(newState.playbackInfo!!.progress.currentChapterIndex).isEqualTo(0)

val controlState = newState.playbackInfo!!.controlState
assertThat(controlState.isSeeking).isTrue
assertThat(controlState.seekTarget).isEqualTo(targetDistance)
assertThat(controlState.isPrevChapter).isTrue
}

@Test
fun reduce_NewAudiobookAction() {
val newState = Reducer.reduce(MockModels.appState(),
Expand Down
3 changes: 3 additions & 0 deletions RELEASE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Project Armadillo Release Notes

## 1.3.2
- Added a fix for updating the playback info progress in the reducer for seek related controls

## 1.3.1
- Adds support for offline DRM for downloaded MPEG-DASH audio

Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ org.gradle.jvmargs=-Xmx1536m
# http://www.gradle.org/docs/current/userguide/multi_project_builds.html#sec:decoupled_projects
# org.gradle.parallel=true
PACKAGE_NAME=com.scribd.armadillo
LIBRARY_VERSION=1.3.1
LIBRARY_VERSION=1.3.2
EXOPLAYER_VERSION=2.17.1
RXJAVA_VERSION=2.2.4
RXANDROID_VERSION=2.0.1
Expand Down
Loading