diff options
Diffstat (limited to 'app')
4 files changed, 204 insertions, 0 deletions
diff --git a/app/src/androidTest/java/org/terst/listen/playback/MediaBrowseTest.kt b/app/src/androidTest/java/org/terst/listen/playback/MediaBrowseTest.kt index 5e3086e..f452c9f 100644 --- a/app/src/androidTest/java/org/terst/listen/playback/MediaBrowseTest.kt +++ b/app/src/androidTest/java/org/terst/listen/playback/MediaBrowseTest.kt @@ -6,6 +6,7 @@ import androidx.media3.session.SessionToken import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.delay import kotlinx.coroutines.guava.await import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withContext @@ -188,4 +189,56 @@ class MediaBrowseTest { publishedAtMs = publishedAtMs, discoveredAtMs = 0, ) + + /** + * An episode started from the car resumes where it was left. + * + * The reported bug: "the play button, everywhere, should resume from the listened position, + * not start over". Every in-app surface was already correct -- the library row, the show + * list and the episode screen were each checked on a device -- because they all go through + * PlaybackAdvancer. Android Auto does not: it reaches playback through the session callback, + * which resolved a browse id into something playable and said nothing about where to start, + * so the player began at zero. + * + * Driven over the real protocol, because the whole defect lives in the boundary a direct + * call would skip. + */ + @Test + fun anEpisodeStartedFromTheCarResumesWhereItWasLeft() = runBlocking { + val graph = Graph.get(context) + graph.subscriptions.insert(seedShow("resume-show", "Resume Hour")) + val episode = seedEpisode("resume-ep", "resume-show", 0) + graph.episodes.insertAll(listOf(episode)) + graph.playbackState.updatePosition("resume-ep", LEFT_AT_MS) + + val browser = connect() + try { + withContext(Dispatchers.Main) { + val item = browser.getItem(LibraryTree.episode(episode).mediaId).await().value!! + browser.setMediaItem(item) + browser.prepare() + // A controller's view of the player is a cached copy the session updates + // asynchronously, so the position is polled rather than read once. + var settled = browser.currentPosition + val giveUpAt = System.currentTimeMillis() + 5_000 + while (settled == 0L && System.currentTimeMillis() < giveUpAt) { + delay(100) + settled = browser.currentPosition + } + assertEquals( + "a browsed episode must start where it was left, not at zero", + LEFT_AT_MS, + settled, + ) + } + } finally { + withContext(Dispatchers.Main) { browser.release() } + graph.subscriptions.delete("resume-show") + } + } + + private companion object { + /** Seven minutes in: far enough that starting over is unmistakable. */ + const val LEFT_AT_MS = 420_000L + } } diff --git a/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt b/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt new file mode 100644 index 0000000..987fe95 --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt @@ -0,0 +1,45 @@ +package org.terst.listen.playback + +import androidx.media3.common.C + +/** + * Where an episode handed to the session by an outside controller should start. + * + * Android Auto, a Bluetooth button and the system's own resumption all reach playback through + * `MediaSession.Callback`, not through the app's view models. That path builds a media item from + * a browse id and nothing else, so without this every episode started from the car began at + * zero -- "the play button, everywhere, should resume from the listened position, not start + * over". Every in-app surface was already correct, which is why the bug survived: it is only + * wrong where nobody developing the app presses play. + * + * Kept apart from the callback and pure so the rule can be tested at all. The callback itself + * needs a real session and a real controller. + */ +object BrowsedStart { + + /** + * The position to begin at, given what the controller asked for and where the episode was + * left. + * + * A controller that named a position wins. It knows something this does not -- a shared + * link carrying a timestamp arrives this way, and so does a seek issued with the media + * item. Only an unset position means "wherever this should start", which is the question + * [PlaybackAdvancer] already answers for every other entry point. + */ + fun positionMs(requestedMs: Long, resumeMs: Long): Long = + if (requestedMs == C.TIME_UNSET) resumeMs else requestedMs + + /** + * Which item of a list the position applies to. + * + * An unset index means the controller had no opinion, and the first item is what plays. + * Clamped because a position belonging to an item that is not there would be applied to + * whichever one happened to be at that index. + */ + fun index(requestedIndex: Int, count: Int): Int = when { + count <= 0 -> 0 + requestedIndex == C.INDEX_UNSET || requestedIndex < 0 -> 0 + requestedIndex >= count -> count - 1 + else -> requestedIndex + } +} diff --git a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt index 723c406..2d44a28 100644 --- a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt +++ b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt @@ -18,6 +18,7 @@ import com.google.common.util.concurrent.ListenableFuture import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.first +import kotlinx.coroutines.guava.await import kotlinx.coroutines.guava.future import org.terst.listen.data.Graph @@ -182,6 +183,45 @@ class ListenSessionCallback( }.toMutableList() } + /** + * Where an episode started from outside the app begins. + * + * Android Auto, a Bluetooth button and the system's own resumption all arrive here rather + * than through the view models, and [onAddMediaItems] alone resolves a browse id into + * something playable without saying where in it to start -- so the player began at zero and + * every episode started in the car started over. Every in-app surface was already correct, + * which is why it survived: it is only wrong where nobody developing the app presses play. + * + * The position comes from [PlaybackAdvancer], the one place that decides where an episode + * starts, so the car and the phone cannot drift apart on it. + */ + override fun onSetMediaItems( + mediaSession: MediaSession, + controller: MediaSession.ControllerInfo, + mediaItems: MutableList<MediaItem>, + startIndex: Int, + startPositionMs: Long, + ): ListenableFuture<MediaSession.MediaItemsWithStartPosition> = scope.future { + // The position is looked up from the items as they arrived, not from the resolved ones. + // Resolution turns a browse id into a playable item carrying the audio URI, and the + // browse id does not survive it -- which is the whole reason onGetItem exists. Reading + // the resolved item gives null, and null resumes at zero, which is the bug this fixes + // wearing a different hat. + val index = BrowsedStart.index(startIndex, mediaItems.size) + val resumeMs = mediaItems.getOrNull(index) + ?.let { LibraryTree.episodeId(it.mediaId) } + ?.let { graph.advancer.start(it) } + ?.let { (it as? StartOutcome.Ready)?.next?.startPositionMs } + ?: 0L + + val resolved = onAddMediaItems(mediaSession, controller, mediaItems).await() + MediaSession.MediaItemsWithStartPosition( + resolved, + BrowsedStart.index(startIndex, resolved.size), + BrowsedStart.positionMs(startPositionMs, resumeMs), + ) + } + // --- search ----------------------------------------------------------------------------- /** diff --git a/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt b/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt new file mode 100644 index 0000000..7c8466a --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt @@ -0,0 +1,66 @@ +package org.terst.listen.playback + +import androidx.media3.common.C +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * Where an episode started by an outside controller begins. + * + * Android Auto, a Bluetooth button and the system's resumption all reach playback through the + * session callback rather than the app's view models, and that path built a media item from a + * browse id and nothing else -- so every episode started in the car began at zero. Reported as + * "the play button, everywhere, should resume from the listened position, not start over". + * + * Every in-app surface was already correct, verified on a device before this was written: the + * library row, the show list and the episode screen all resume. That is exactly why it lasted + * -- it is only wrong where nobody developing the app presses play. + */ +class BrowsedStartTest { + + @Test + fun `an episode with nowhere named resumes where it was left`() { + assertEquals(420_000L, BrowsedStart.positionMs(C.TIME_UNSET, resumeMs = 420_000)) + } + + @Test + fun `an unheard episode still starts at the beginning`() { + assertEquals(0L, BrowsedStart.positionMs(C.TIME_UNSET, resumeMs = 0)) + } + + /** + * A controller that named a position knows something this does not: a link shared with a + * timestamp arrives this way, and so does a seek issued alongside the media item. + */ + @Test + fun `a position the controller asked for wins`() { + assertEquals(90_000L, BrowsedStart.positionMs(requestedMs = 90_000, resumeMs = 420_000)) + } + + /** Including the start, which is a real request and not the absence of one. */ + @Test + fun `an explicit zero is honoured rather than treated as unset`() { + assertEquals(0L, BrowsedStart.positionMs(requestedMs = 0, resumeMs = 420_000)) + } + + @Test + fun `no opinion about which item means the first one`() { + assertEquals(0, BrowsedStart.index(C.INDEX_UNSET, count = 3)) + assertEquals(0, BrowsedStart.index(-1, count = 3)) + } + + @Test + fun `the item the controller named is the one the position belongs to`() { + assertEquals(2, BrowsedStart.index(2, count = 3)) + } + + /** + * An index past the end would otherwise apply a position to whichever item happened to sit + * there, which is a seek into the middle of the wrong episode. + */ + @Test + fun `an index past the end is clamped`() { + assertEquals(2, BrowsedStart.index(9, count = 3)) + assertEquals(0, BrowsedStart.index(9, count = 0)) + } +} |