Merge branch 'fix/resume-playlist-position-across-recreate'

android/app/src/main/java/com/remotedisplay/player/MainActivity.kt

@@ -227,7 +227,11 @@ class MainActivity : AppCompatActivity() {
                 val cid = item.contentId.ifEmpty { item.widgetId ?: "" }
                 if (event == "play_start") wsService?.sendPlayStart(cid, item.filename, item.durationSec)
                 else wsService?.sendPlayEnd(cid, item.filename, completed)
-            }
+            },
+            // #234: carry the playback position across Activity rebuilds. Without this a relaunch
+            // restarts the playlist at item 1, so anything after it never gets a turn.
+            loadResume = { config.resumeIndex.takeIf { it >= 0 }?.let { it to config.resumeAt } },
+            saveResume = { index, atMs -> config.resumeIndex = index; config.resumeAt = atMs }
         )
         // Screen-resilience: an item is playable only when its content is actually available —
         // a widget, a remote stream, or a fully-downloaded local file. A not-yet/failed download is

android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt

@@ -98,6 +98,17 @@ class ServerConfig(context: Context) {
         get() = prefs.getString("cached_playlist", "") ?: ""
         set(value) = prefs.edit().putString("cached_playlist", value).apply()
 
+    // #234: last playing index + when it started. Lives here, not in PlaylistController, precisely
+    // because the controller is rebuilt with every Activity — which is how a relaunch used to reset
+    // playback to the first item and starve everything after it.
+    var resumeIndex: Int
+        get() = prefs.getInt("resume_index", -1)
+        set(value) = prefs.edit().putInt("resume_index", value).apply()
+
+    var resumeAt: Long
+        get() = prefs.getLong("resume_at", 0L)
+        set(value) = prefs.edit().putLong("resume_at", value).apply()
+
     fun clearPlaylistCache() {
         prefs.edit().remove("cached_playlist").apply()
     }

android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt

@@ -39,7 +39,11 @@ class PlaylistController(
     private val onWaitingForContent: (() -> Unit)? = null,
     // Proof-of-play: emitted on each item show ("play_start") and when it's left ("play_end"),
     // so the caller can forward device:play-event to the server (populates play_logs / Reports).
-    private val onPlayLog: ((event: String, item: PlaylistItem, completed: Boolean) -> Unit)? = null
+    private val onPlayLog: ((event: String, item: PlaylistItem, completed: Boolean) -> Unit)? = null,
+    // #234: playback position, persisted OUTSIDE this object so it survives the controller being
+    // rebuilt with a new Activity. Null on both = today's behaviour (always start from the top).
+    private val loadResume: (() -> Pair<Int, Long>?)? = null,
+    private val saveResume: ((index: Int, atMs: Long) -> Unit)? = null
 ) {
     private companion object {
         const val CONTENT_RECHECK_MS = 3000L
@@ -289,7 +293,16 @@ class PlaylistController(
         if (firstActiveIndex() < 0) { showNothingScheduled(); return }
         // Screen-resilience: only start on an item whose content is downloaded; if the scheduled
         // content isn't ready yet, keep current/wait (never blank on a loading state).
-        val idx = PlaylistSelection.firstPlayableIndex(items.size) { playableNow(it) }
+        // #234: continue where we left off when this is a reload moments after playing (a new
+        // Activity => a brand-new controller), not a genuine cold start. Scanning from 0 every time
+        // is what pinned these playlists on their first item forever.
+        val saved = try { loadResume?.invoke() } catch (_: Throwable) { null }
+        val from = PlaybackResume.resumeIndex(
+            saved?.first ?: -1, saved?.second ?: 0L, System.currentTimeMillis(), items.size)
+        val idx = if (from > 0 && playableNow(from)) from
+                  else if (from > 0) PlaylistSelection.nextPlayableIndex(items.size, from - 1) { playableNow(it) }
+                  else PlaylistSelection.firstPlayableIndex(items.size) { playableNow(it) }
+        if (from > 0) Log.i("PlaylistController", "Resuming at index $from (reload within resume window)")
         if (idx >= 0) { currentIndex = idx; playCurrentItem() } else onContentNotReady()
     }
 
@@ -365,6 +378,8 @@ class PlaylistController(
         cancelRetry()
         val item = currentItem ?: return
         itemStartedAt = System.currentTimeMillis()
+        // #234: remember where we are so a controller rebuilt seconds from now can carry on.
+        try { saveResume?.invoke(currentIndex, itemStartedAt) } catch (_: Throwable) {}
         Log.i("PlaylistController", "Playing: ${item.filename} (index $currentIndex)")
         onItemChanged(item)
         hasContentOnScreen = true // a valid item is now rendered — protect it from being blanked

android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt

@@ -44,3 +44,35 @@ object PlaylistSelection {
     fun whenNonePlayable(hasContentOnScreen: Boolean): NonePlayable =
         if (hasContentOnScreen) NonePlayable.KEEP_CURRENT else NonePlayable.SHOW_WAITING
 }
+
+/**
+ * #234 — where playback should RESUME when a playlist is (re)loaded.
+ *
+ * PlaylistController is created fresh with every MainActivity instance, so a recreate always handed
+ * it an empty list and then a full one, which reads as "0 -> N items" and starts from the top. On a
+ * panel that re-registers and relaunches itself at each item boundary, item 2 therefore never
+ * survived more than a fraction of a second: the reporter of #234 had "never seen the photo, just
+ * the video", and prod play_logs showed the second item logging 0-1s durations while the first
+ * accumulated all the playtime.
+ *
+ * Starting from the top is only correct for a genuinely COLD start. If we were playing moments ago,
+ * the right thing is to carry on. Kept pure so the window arithmetic is testable without a device.
+ */
+object PlaybackResume {
+    /** How recently we must have been playing for a reload to count as a continuation. */
+    const val RESUME_WINDOW_MS = 90_000L
+
+    /**
+     * Index to begin scanning from. [savedIndex] < 0, an empty/short playlist, a stale save, or a
+     * clock that jumped backwards all fall back to 0 — i.e. to today's behaviour, so a real cold
+     * start is unaffected.
+     */
+    fun resumeIndex(savedIndex: Int, savedAtMs: Long, nowMs: Long, itemCount: Int): Int {
+        if (itemCount <= 0) return 0
+        if (savedIndex < 0 || savedIndex >= itemCount) return 0
+        if (savedAtMs <= 0L) return 0
+        val age = nowMs - savedAtMs
+        if (age < 0L || age > RESUME_WINDOW_MS) return 0
+        return savedIndex
+    }
+}

android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt

@@ -0,0 +1,63 @@
+package com.remotedisplay.player.player
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+/**
+ * #234 "Android player dont play playlist properly" — with two items only one ever played.
+ *
+ * PlaylistController is rebuilt with every MainActivity instance, so a relaunch handed it an empty
+ * list and then a full one ("0 -> N items") and it started from the top. On a panel that relaunches
+ * itself at each item boundary, the second item was preempted after ~135ms every single time —
+ * reproduced on an Android 9 emulator, and matching prod play_logs where the second item recorded
+ * 0-1s durations while the first accumulated all the playtime. The reporter had never once seen it.
+ *
+ * Starting at the top is right for a genuinely cold start and wrong for a reload seconds later.
+ */
+class PlaybackResumeTest {
+
+    private val NOW = 1_000_000L
+    private val W = PlaybackResume.RESUME_WINDOW_MS
+
+    @Test fun THE_BUG_a_reload_moments_after_playing_continues_where_it_was() {
+        assertEquals(1, PlaybackResume.resumeIndex(
+            savedIndex = 1, savedAtMs = NOW - 5_000, nowMs = NOW, itemCount = 2))
+    }
+
+    @Test fun a_genuine_cold_start_still_begins_at_the_top() {
+        // Nothing saved: unchanged behaviour, which is what makes this safe to ship.
+        assertEquals(0, PlaybackResume.resumeIndex(-1, 0L, NOW, 3))
+    }
+
+    @Test fun a_stale_save_is_ignored_it_is_a_cold_start_not_a_continuation() {
+        assertEquals(0, PlaybackResume.resumeIndex(2, NOW - (W + 1), NOW, 3))
+    }
+
+    @Test fun just_inside_the_window_still_resumes() {
+        assertEquals(2, PlaybackResume.resumeIndex(2, NOW - (W - 1), NOW, 3))
+    }
+
+    @Test fun an_index_past_the_end_falls_back_rather_than_selecting_nothing() {
+        // The playlist shrank while we were away.
+        assertEquals(0, PlaybackResume.resumeIndex(7, NOW - 1_000, NOW, 3))
+        assertEquals(0, PlaybackResume.resumeIndex(3, NOW - 1_000, NOW, 3))
+    }
+
+    @Test fun an_empty_playlist_never_resumes() {
+        assertEquals(0, PlaybackResume.resumeIndex(1, NOW - 1_000, NOW, 0))
+    }
+
+    @Test fun a_clock_that_jumped_backwards_is_treated_as_stale_not_as_fresh() {
+        // Signage panels do correct their clocks. A negative age must not read as "0ms ago".
+        assertEquals(0, PlaybackResume.resumeIndex(1, NOW + 60_000, NOW, 2))
+    }
+
+    @Test fun a_zero_timestamp_is_not_1970_it_is_no_save_at_all() {
+        assertEquals(0, PlaybackResume.resumeIndex(1, 0L, NOW, 2))
+    }
+
+    @Test fun resuming_at_index_0_is_indistinguishable_from_starting_fresh() {
+        // Deliberate: index 0 needs no special handling, and the caller treats >0 as "resume".
+        assertEquals(0, PlaybackResume.resumeIndex(0, NOW - 1_000, NOW, 2))
+    }
+}