diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fa70fb90ff..3cd4e1b2e38 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ * New Features * Added explicit indicator to podcast page ([#5243](https://github.com/Automattic/pocket-casts-android/pull/5243)) +* Bug Fixes + * Fix Skip Last calculation at increased playback speeds + ([#5230](https://github.com/Automattic/pocket-casts-android/pull/5230)) * Updates * Make sure archived episode status is preserved across sync ([#5227](https://github.com/Automattic/pocket-casts-android/pull/5227)) diff --git a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt index b59d8292c9d..9bd7e4db143 100644 --- a/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt +++ b/modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt @@ -170,6 +170,26 @@ open class PlaybackManager @Inject constructor( private const val MAX_TIME_WITHOUT_FOCUS_FOR_RESUME_MINUTES = 30 private const val MAX_TIME_WITHOUT_FOCUS_FOR_RESUME = (MAX_TIME_WITHOUT_FOCUS_FOR_RESUME_MINUTES * 60 * 1000).toLong() private const val PAUSE_TIMER_DELAY = ((MAX_TIME_WITHOUT_FOCUS_FOR_RESUME_MINUTES + 1) * 60 * 1000).toLong() + + internal fun effectiveSkipLastMs(skipLastSecs: Int?, playbackSpeed: Double): Long { + val secs = skipLastSecs ?: 0 + val safeSpeed = (playbackSpeed.takeIf { it.isFinite() } ?: 1.0).coerceAtLeast(1.0) + return (secs * 1000L * safeSpeed).toLong() + } + + internal fun shouldSkipLast( + skipLastSecs: Int?, + playbackSpeed: Double, + positionMs: Int, + durationMs: Int?, + ): Boolean { + if (skipLastSecs == null || skipLastSecs <= 0) return false + if (durationMs == null || durationMs <= 0) return false + if (positionMs < 0) return false + val thresholdMs = effectiveSkipLastMs(skipLastSecs, playbackSpeed) + if (durationMs <= thresholdMs) return false + return durationMs - positionMs < thresholdMs + } } private var notificationPermissionChecker: NotificationPermissionChecker? = null @@ -2353,12 +2373,14 @@ open class PlaybackManager @Inject constructor( private suspend fun updateCurrentPosition() { val episode = getCurrentEpisode() ?: return - if (episode.uuid != playbackStateRelay.blockingFirst().episodeUuid) { + val currentPlaybackState = playbackStateRelay.blockingFirst() + if (episode.uuid != currentPlaybackState.episodeUuid) { Timber.d("Timer fired after onCompletion, ignoring") return } - val skipLast = playbackStateRelay.blockingFirst().podcast?.skipLastSecs + val skipLast = currentPlaybackState.podcast?.skipLastSecs + val playbackSpeed = currentPlaybackState.playbackSpeed val positionMs = player?.getCurrentPositionMs() ?: -1 if (positionMs < 0) { @@ -2366,27 +2388,25 @@ open class PlaybackManager @Inject constructor( } val durationMs = player?.durationMs() - @Suppress("KotlinConstantConditions") // This is a false positive - if (skipLast.isPositive() && durationMs.isPositive() && durationMs > skipLast) { - val timeRemaining = (durationMs - positionMs) / 1000 - if (timeRemaining < skipLast) { - if (isSleepAfterEpisodeEnabled()) { - sleepEndOfEpisode(episode) - episodeManager.markAsPlayedBlocking(episode, this, podcastManager) - } else { - eventHorizon.track( - PlayerEpisodeCompletedEvent( - podcastUuid = episode.podcastOrSubstituteUuid, - episodeUuid = episode.uuid, - ), - ) - statsManager.addTimeSavedAutoSkipping(timeRemaining.toLong() * 1000L) - episodeManager.markAsPlayedBlocking(episode, this, podcastManager) - LogBuffer.i(LogBuffer.TAG_PLAYBACK, "Skipping remainder of ${episode.title} with skip last $skipLast") - showToast(application.getString(LR.string.player_skipped_last, skipLast)) - } - return + if (shouldSkipLast(skipLast, playbackSpeed, positionMs, durationMs)) { + // shouldSkipLast guarantees durationMs is non-null and positive when it returns true. + val remainingMs = (durationMs!! - positionMs).coerceAtLeast(0) + if (isSleepAfterEpisodeEnabled()) { + sleepEndOfEpisode(episode) + episodeManager.markAsPlayedBlocking(episode, this, podcastManager) + } else { + eventHorizon.track( + PlayerEpisodeCompletedEvent( + podcastUuid = episode.podcastOrSubstituteUuid, + episodeUuid = episode.uuid, + ), + ) + statsManager.addTimeSavedAutoSkipping(remainingMs.toLong()) + episodeManager.markAsPlayedBlocking(episode, this, podcastManager) + LogBuffer.i(LogBuffer.TAG_PLAYBACK, "Skipping remainder of ${episode.title} with skip last $skipLast (speed=$playbackSpeed)") + showToast(application.getString(LR.string.player_skipped_last, skipLast)) } + return } episode.playedUpToMs = positionMs diff --git a/modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManagerSkipLastTest.kt b/modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManagerSkipLastTest.kt new file mode 100644 index 00000000000..b13ac132f28 --- /dev/null +++ b/modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManagerSkipLastTest.kt @@ -0,0 +1,162 @@ +package au.com.shiftyjelly.pocketcasts.repositories.playback + +import au.com.shiftyjelly.pocketcasts.repositories.playback.PlaybackManager.Companion.effectiveSkipLastMs +import au.com.shiftyjelly.pocketcasts.repositories.playback.PlaybackManager.Companion.shouldSkipLast +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +class PlaybackManagerSkipLastTest { + + @Test + fun `effectiveSkipLastMs at 1x equals configured seconds`() { + assertEquals(120_000L, effectiveSkipLastMs(skipLastSecs = 120, playbackSpeed = 1.0)) + } + + @Test + fun `effectiveSkipLastMs at 2x doubles the window`() { + assertEquals(240_000L, effectiveSkipLastMs(skipLastSecs = 120, playbackSpeed = 2.0)) + } + + @Test + fun `effectiveSkipLastMs at 1 point 5x scales linearly`() { + assertEquals(180_000L, effectiveSkipLastMs(skipLastSecs = 120, playbackSpeed = 1.5)) + } + + @Test + fun `effectiveSkipLastMs below 1x is clamped to 1x`() { + // Sub-1x users must never lose time they configured; the threshold stays at the 1x window. + assertEquals(120_000L, effectiveSkipLastMs(skipLastSecs = 120, playbackSpeed = 0.5)) + assertEquals(120_000L, effectiveSkipLastMs(skipLastSecs = 120, playbackSpeed = 0.8)) + } + + @Test + fun `effectiveSkipLastMs treats NaN speed as 1x`() { + assertEquals(120_000L, effectiveSkipLastMs(skipLastSecs = 120, playbackSpeed = Double.NaN)) + } + + @Test + fun `effectiveSkipLastMs is zero when skipLastSecs is null or zero`() { + assertEquals(0L, effectiveSkipLastMs(skipLastSecs = null, playbackSpeed = 2.0)) + assertEquals(0L, effectiveSkipLastMs(skipLastSecs = 0, playbackSpeed = 2.0)) + } + + @Test + fun `shouldSkipLast at 1x does not fire just before the window starts`() { + // 30m media, skipLast=120s. Window starts at 28:00 (1_680_000ms). One ms before → no fire. + assertFalse( + shouldSkipLast( + skipLastSecs = 120, + playbackSpeed = 1.0, + positionMs = 1_679_999, + durationMs = 1_800_000, + ), + ) + } + + @Test + fun `shouldSkipLast at 1x fires when entering the window`() { + // 30m media, skipLast=120s. Remaining = 119_999ms < 120_000ms → fire. + assertTrue( + shouldSkipLast( + skipLastSecs = 120, + playbackSpeed = 1.0, + positionMs = 1_680_001, + durationMs = 1_800_000, + ), + ) + } + + @Test + fun `shouldSkipLast at 2x does not fire just before the listening-time window`() { + // 30m media at 2x with skipLast=120s listening time = 240s media window. + // Before 26:00 media = still more than 4 listening minutes left, so no fire. + assertFalse( + shouldSkipLast( + skipLastSecs = 120, + playbackSpeed = 2.0, + positionMs = 1_559_999, + durationMs = 1_800_000, + ), + ) + } + + @Test + fun `shouldSkipLast at 2x fires when 2 listening minutes remain`() { + // 30m media at 2x with skipLast=120s listening time = 240s media window. + // Remaining = 239_999ms (< 240_000ms threshold) → fire. + assertTrue( + shouldSkipLast( + skipLastSecs = 120, + playbackSpeed = 2.0, + positionMs = 1_560_001, + durationMs = 1_800_000, + ), + ) + } + + @Test + fun `shouldSkipLast at 0 point 5x keeps the 1x window (never shrinks)`() { + // The user set Skip Last=120s. On 0.5x we must not reduce the media window below 120_000ms. + // At 120_001ms remaining (position 1_679_999), 1x would NOT fire, so neither should 0.5x. + assertFalse( + shouldSkipLast( + skipLastSecs = 120, + playbackSpeed = 0.5, + positionMs = 1_679_999, + durationMs = 1_800_000, + ), + ) + assertTrue( + shouldSkipLast( + skipLastSecs = 120, + playbackSpeed = 0.5, + positionMs = 1_680_001, + durationMs = 1_800_000, + ), + ) + } + + @Test + fun `shouldSkipLast returns false when skipLastSecs is zero or null`() { + assertFalse( + shouldSkipLast(skipLastSecs = 0, playbackSpeed = 2.0, positionMs = 1_790_000, durationMs = 1_800_000), + ) + assertFalse( + shouldSkipLast(skipLastSecs = null, playbackSpeed = 2.0, positionMs = 1_790_000, durationMs = 1_800_000), + ) + } + + @Test + fun `shouldSkipLast returns false when durationMs is null or non-positive`() { + assertFalse( + shouldSkipLast(skipLastSecs = 120, playbackSpeed = 1.0, positionMs = 1_000, durationMs = null), + ) + assertFalse( + shouldSkipLast(skipLastSecs = 120, playbackSpeed = 1.0, positionMs = 1_000, durationMs = 0), + ) + } + + @Test + fun `shouldSkipLast returns false when positionMs is negative`() { + // Player hasn't reported a position yet. + assertFalse( + shouldSkipLast(skipLastSecs = 120, playbackSpeed = 1.0, positionMs = -1, durationMs = 1_800_000), + ) + } + + @Test + fun `shouldSkipLast returns false when episode is shorter than the skip-last window`() { + // A 60s episode with skipLast=120s at 1x: the whole episode is inside the window, but we + // intentionally do NOT fire because the guard `durationMs > thresholdMs` filters it out + // to avoid skipping an entire short episode. + assertFalse( + shouldSkipLast(skipLastSecs = 120, playbackSpeed = 1.0, positionMs = 0, durationMs = 60_000), + ) + // Same at 2x: 120s episode, 240s window → still filtered out. + assertFalse( + shouldSkipLast(skipLastSecs = 120, playbackSpeed = 2.0, positionMs = 0, durationMs = 120_000), + ) + } +}