Fix skip last calculation at increased playback speeds

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))

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,40 +2373,40 @@ 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) {
             return
         }
 
         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

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),
+        )
+    }
+}