From 4a925b6c735a3de75a91c6a7c0878f2401365342 Mon Sep 17 00:00:00 2001 From: Tamas Szelezsan Date: Thu, 16 Apr 2026 20:37:15 +0200 Subject: [PATCH 1/5] Fix skip last calculation on increased playback speeds --- .../repositories/playback/PlaybackManager.kt | 61 ++++++++++++------- 1 file changed, 40 insertions(+), 21 deletions(-) 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 824ffc2617f..4aa523567ec 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 @@ -169,6 +169,25 @@ 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 + return (secs * 1000L * playbackSpeed.coerceAtLeast(1.0)).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 @@ -2330,7 +2349,9 @@ open class PlaybackManager @Inject constructor( return } - val skipLast = playbackStateRelay.blockingFirst().podcast?.skipLastSecs + val currentPlaybackState = playbackStateRelay.blockingFirst() + val skipLast = currentPlaybackState.podcast?.skipLastSecs + val playbackSpeed = currentPlaybackState.playbackSpeed val positionMs = player?.getCurrentPositionMs() ?: -1 if (positionMs < 0) { @@ -2338,27 +2359,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 + 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 From a495d507a169e9d6e5ceac86e0f289b51d335fcc Mon Sep 17 00:00:00 2001 From: Tamas Szelezsan Date: Thu, 16 Apr 2026 20:39:47 +0200 Subject: [PATCH 2/5] Add tests --- .../repositories/playback/PlaybackManager.kt | 2 +- .../playback/PlaybackManagerSkipLastTest.kt | 157 ++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManagerSkipLastTest.kt 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 4aa523567ec..d2d2d6198a1 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 @@ -174,7 +174,7 @@ open class PlaybackManager @Inject constructor( val secs = skipLastSecs ?: 0 return (secs * 1000L * playbackSpeed.coerceAtLeast(1.0)).toLong() } - + internal fun shouldSkipLast( skipLastSecs: Int?, playbackSpeed: Double, 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..8eb2fd85abf --- /dev/null +++ b/modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManagerSkipLastTest.kt @@ -0,0 +1,157 @@ +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 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 60_001ms remaining (position 1_739_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. + // (Preserves prior behaviour for very short episodes.) + 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), + ) + } +} From be09e04b5720690a49849adbc105a89bdea9c7d0 Mon Sep 17 00:00:00 2001 From: Tamas Szelezsan Date: Thu, 16 Apr 2026 20:46:10 +0200 Subject: [PATCH 3/5] Update changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a759156cc2a..f0fc2c9d13f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,8 @@ 8.11 ----- - +* Bug Fixes + * Fix Skip Last calculation at increased playback speeds + ([#5230](https://github.com/Automattic/pocket-casts-android/pull/5230)) 8.10 ----- From 7961dc83d8ea20b181adfdd6da867ff6d6846fe3 Mon Sep 17 00:00:00 2001 From: Tamas Szelezsan Date: Wed, 22 Apr 2026 11:38:34 +0200 Subject: [PATCH 4/5] Address PR comments --- .../repositories/playback/PlaybackManagerSkipLastTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 index 8eb2fd85abf..acc77288ba9 100644 --- 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 @@ -94,7 +94,7 @@ class PlaybackManagerSkipLastTest { @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 60_001ms remaining (position 1_739_999), 1x would NOT fire, so neither should 0.5x. + // At 120_001ms remaining (position 1_679_999), 1x would NOT fire, so neither should 0.5x. assertFalse( shouldSkipLast( skipLastSecs = 120, From 5907f0b3bd2e80f2e9a894723f87a8f48912acc6 Mon Sep 17 00:00:00 2001 From: Tamas Szelezsan Date: Fri, 8 May 2026 10:20:18 +0200 Subject: [PATCH 5/5] Address PR review comments - Guard effectiveSkipLastMs against NaN playback speed - Capture playbackStateRelay once in updateCurrentPosition - Clamp remainingMs to avoid negative time-saved stats - Fix misleading test comment about prior behaviour --- .../pocketcasts/repositories/playback/PlaybackManager.kt | 9 +++++---- .../repositories/playback/PlaybackManagerSkipLastTest.kt | 9 +++++++-- 2 files changed, 12 insertions(+), 6 deletions(-) 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 46072bcfb26..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 @@ -173,7 +173,8 @@ open class PlaybackManager @Inject constructor( internal fun effectiveSkipLastMs(skipLastSecs: Int?, playbackSpeed: Double): Long { val secs = skipLastSecs ?: 0 - return (secs * 1000L * playbackSpeed.coerceAtLeast(1.0)).toLong() + val safeSpeed = (playbackSpeed.takeIf { it.isFinite() } ?: 1.0).coerceAtLeast(1.0) + return (secs * 1000L * safeSpeed).toLong() } internal fun shouldSkipLast( @@ -2372,12 +2373,12 @@ 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 currentPlaybackState = playbackStateRelay.blockingFirst() val skipLast = currentPlaybackState.podcast?.skipLastSecs val playbackSpeed = currentPlaybackState.playbackSpeed @@ -2389,7 +2390,7 @@ open class PlaybackManager @Inject constructor( val durationMs = player?.durationMs() if (shouldSkipLast(skipLast, playbackSpeed, positionMs, durationMs)) { // shouldSkipLast guarantees durationMs is non-null and positive when it returns true. - val remainingMs = durationMs!! - positionMs + val remainingMs = (durationMs!! - positionMs).coerceAtLeast(0) if (isSleepAfterEpisodeEnabled()) { sleepEndOfEpisode(episode) episodeManager.markAsPlayedBlocking(episode, this, podcastManager) 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 index acc77288ba9..b13ac132f28 100644 --- 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 @@ -31,6 +31,11 @@ class PlaybackManagerSkipLastTest { 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)) @@ -144,8 +149,8 @@ class PlaybackManagerSkipLastTest { @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. - // (Preserves prior behaviour for very short episodes.) + // 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), )