diff --git a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt b/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt index e4fdb42..29a7a62 100644 --- a/android/app/src/main/java/com/remotedisplay/player/MainActivity.kt +++ b/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 diff --git a/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt b/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt index dd62c60..e621575 100644 --- a/android/app/src/main/java/com/remotedisplay/player/data/ServerConfig.kt +++ b/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() } diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt index 353439c..d1b2e42 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistController.kt +++ b/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?)? = 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 diff --git a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt b/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt index eefa073..483464c 100644 --- a/android/app/src/main/java/com/remotedisplay/player/player/PlaylistSelection.kt +++ b/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 + } +} diff --git a/android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt b/android/app/src/test/java/com/remotedisplay/player/player/PlaybackResumeTest.kt new file mode 100644 index 0000000..67f2f4b --- /dev/null +++ b/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)) + } +}