diff options
| -rw-r--r-- | .coverage-baseline | 2 | ||||
| -rw-r--r-- | CLAUDE.md | 28 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/data/Repositories.kt | 7 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt | 89 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/PlaybackService.kt | 105 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/PlayerConnection.kt | 43 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt | 42 | ||||
| -rw-r--r-- | app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt | 121 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt | 236 | ||||
| -rw-r--r-- | app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt | 113 |
10 files changed, 653 insertions, 133 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index e8aa377..5d8929f 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -66.23 +65.33 @@ -240,11 +240,13 @@ out of a possibly-partial MP3, and the JSON path covers the shows that mark chap slow drip on or off" rather than "Slow drip", because a description identical to the heading beside it makes the tap ambiguous. -- **`connectedDebugAndroidTest` can wipe the app's data.** A run cleared `listen.db` on the - emulator mid-session. It is the test harness's install path, not a migration: a real v2 - database seeded with a subscription, an episode at position 424242 and a rule was upgraded in - place by launching the v3 build, and all three survived with drip defaulting to off. Do not - read an empty database after an instrumented run as a migration failure -- but do check. +- **`connectedDebugAndroidTest` uninstalls the app when it finishes**, taking the database with + it. Isolated on the emulator: a plain `adb install -r` left four episodes in place, and the + very next instrumented run left `run-as: unknown package`. So an empty or missing database + after an instrumented run is the harness, not a migration -- and any device state you want to + keep has to be re-seeded afterwards. Migrations were separately proven safe by seeding a real + v2 database (a subscription, an episode at position 424242, a rule) and upgrading it in place + by launching the v3 build: all three survived, with drip defaulting to off. - **The drip released-set is a query parameter, not stored state.** `observeVisible`/`getVisible` take `dripEnabled` and hide unreleased episodes only when it is true, so switching drip off @@ -252,6 +254,22 @@ out of a possibly-partial MP3, and the JSON path covers the shows that mark chap left off. `getForSubscription` deliberately still returns everything: feed reconciliation must see held-back episodes or it re-inserts them on every refresh. +- **Completion belongs to the service, not to a view model.** Marking an episode played, + advancing the queue, the outro offset and ad-chapter skipping all live in `PlaybackService` + via `PlaybackAdvancer` and `PlayheadPolicy`. They used to live in `PlayerViewModel`, which is + scoped to the activity and releases its MediaController in `onCleared` -- so with the activity + gone the audio played on from the foreground service while nothing marked anything played or + moved the queue on. Background listening is the normal case for this app. Verified with + "don't keep activities" on: with no activity alive, an episode reached its outro line, was + marked played, and the next one started. + + `PlayerConnection` deliberately no longer handles `STATE_ENDED`, and `PlayerViewModel` no + longer has an `onFinished`. Putting either back means the queue advances twice. + +- **Where an episode starts is decided in exactly one place**, `PlaybackAdvancer.start`, used by + both the service (queue advance) and the view model (a tap on Play). Two copies of the resume + rules would drift. + - **A start position belongs to `setMediaItem`, not a `seekTo` after `prepare()`.** Seeking afterwards is silently dropped across the MediaController-to-service boundary. This was found wiring up the intro offset -- an episode set to start sixty seconds in began at zero -- and it diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index 773aa96..bcd132a 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -9,6 +9,7 @@ import org.terst.listen.download.EpisodeDownloader import org.terst.listen.feed.ChapterFetcher import org.terst.listen.feed.FeedFetcher import org.terst.listen.feed.FeedResolver +import org.terst.listen.playback.PlaybackAdvancer import org.terst.listen.queue.DripReleaser import org.terst.listen.queue.QueueOrdering import org.terst.listen.queue.RetentionPolicy @@ -59,6 +60,12 @@ class AppGraph(private val context: Context) { val chapters: ChapterFetcher by lazy { ChapterFetcher(client, episodes) } + // Shared with PlaybackService, so a user-started episode and one the queue moved on to are + // worked out the same way -- same resume position, same intro offset, same speed. + val advancer: PlaybackAdvancer by lazy { + PlaybackAdvancer(episodes, subscriptions, queue, chaptersFor = { chapters.chapters(it) }) + } + val repository: SubscriptionRepository by lazy { SubscriptionRepository( subscriptions, diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt new file mode 100644 index 0000000..6d529e8 --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt @@ -0,0 +1,89 @@ +package org.terst.listen.playback + +import org.terst.listen.data.EpisodeDao +import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.QueueRepository +import org.terst.listen.data.SubscriptionDao +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.feed.Chapter + +/** What the ticker needs to know about whatever is playing, read once per episode. */ +data class PlayingContext( + val show: SubscriptionEntity? = null, + val chapters: List<Chapter> = emptyList(), +) + +/** An episode ready to begin: which one, where from, and how fast. */ +data class NextUp( + val episode: EpisodeEntity, + /** Null when the show has since been unsubscribed; the episode still plays. */ + val show: SubscriptionEntity?, + val startPositionMs: Long, + val speed: Float, +) + +/** + * Finishing an episode, and working out what follows it. + * + * Owned by the service rather than by a view model. It used to live in one scoped to the + * activity, which meant swiping Listen off recents stopped the queue advancing and episodes + * being marked played while the audio carried on from the foreground service. + * + * Deliberately knows nothing about Media3: it reports which episode to start and the numbers to + * start it with, and the service turns that into a MediaItem. That keeps the decisions testable + * off-device, where the Android classes cannot be constructed. + */ +class PlaybackAdvancer( + private val episodes: EpisodeDao, + private val subscriptions: SubscriptionDao, + private val queue: QueueRepository, + /** + * How chapters are obtained. A collaborator rather than a ChapterFetcher so the decision to + * fetch at all -- only for a show that opted in -- stays here and stays testable. + */ + private val chaptersFor: suspend (EpisodeEntity) -> List<Chapter> = { emptyList() }, + private val progress: PlaybackProgress = PlaybackProgress(), + private val now: () -> Long = System::currentTimeMillis, +) { + + /** + * The show and chapters behind an episode. + * + * Read once when the playing item changes rather than on every tick: the ticker runs every + * two seconds for hours at a stretch and neither answer changes mid-episode. + */ + suspend fun contextFor(episodeId: String?): PlayingContext { + val episode = episodeId?.let { episodes.getById(it) } ?: return PlayingContext() + val show = subscriptions.getById(episode.subscriptionId) + // Only worth fetching for a show that asked for it; most publish no chapters at all. + val chapters = if (show?.skipAdChapters == true) chaptersFor(episode) else emptyList() + return PlayingContext(show, chapters) + } + + /** + * Records an episode as finished and reports what plays next, or null if nothing does. + * + * Marking played happens before advancing: advance rewrites the queue, and the other order + * loses the id. + */ + suspend fun finish(episodeId: String): NextUp? { + episodes.markPlayed(episodeId, now()) + return queue.advance()?.let { start(it) } + } + + /** Everything needed to begin [episodeId], or null if it has gone. */ + suspend fun start(episodeId: String): NextUp? { + val episode = episodes.getById(episodeId) ?: return null + val show = subscriptions.getById(episode.subscriptionId) + return NextUp( + episode = episode, + show = show, + startPositionMs = progress.resumePosition( + episode.positionMs, + episode.durationMs, + show?.skipIntroMs ?: 0, + ), + speed = PlaybackSpeed.forEpisode(episode.speedOverride, show?.playbackSpeed), + ) + } +} diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt index 5901b22..0135cdc 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt @@ -17,8 +17,8 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.launch import org.terst.listen.data.AppDatabase import org.terst.listen.data.Artwork -import org.terst.listen.data.EpisodeDao import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.Graph /** * Hosts the player so audio survives the app leaving the foreground. @@ -33,6 +33,12 @@ class PlaybackService : MediaLibraryService() { private var player: ExoPlayer? = null private var session: MediaLibrarySession? = null private var writer: PositionWriter? = null + private var advancer: PlaybackAdvancer? = null + private val progress = PlaybackProgress() + + /** What the ticker runs against, refreshed whenever the playing item changes. */ + private var context = PlayingContext() + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main) override fun onCreate() { @@ -63,11 +69,38 @@ class PlaybackService : MediaLibraryService() { .setCustomLayout(NotificationControls.customLayout()) .build() - val episodes = AppDatabase.get(this).episodes() - writer = PositionWriter(ExoPlayhead(exo), episodes) + val graph = Graph.get(this) + writer = PositionWriter(ExoPlayhead(exo), graph.episodes) + advancer = graph.advancer + + exo.addListener(completionListener) startPositionTicker() } + /** + * Completion, handled here rather than by whoever happens to be showing the UI. + * + * The queue used to advance from a view model scoped to the activity, so swiping Listen off + * recents left the audio playing from this service with nothing marking episodes played or + * moving the queue on. Background listening is the normal case for a podcast app, so the + * component that survives backgrounding is the one that has to own this. + */ + private val completionListener = object : Player.Listener { + override fun onPlaybackStateChanged(playbackState: Int) { + if (playbackState != Player.STATE_ENDED) return + // Read before anything else runs: starting the next episode replaces the media item, + // so the id is gone by the time this returns. + val finished = player?.currentMediaItem?.mediaId ?: return + finish(finished) + } + + override fun onMediaItemTransition(mediaItem: MediaItem?, reason: Int) { + // The cached show and chapters belong to whatever is playing; refresh them whenever + // that changes, including when something else in the app starts an episode. + loadContextFor(mediaItem?.mediaId) + } + } + override fun onGetSession(controllerInfo: MediaSession.ControllerInfo): MediaLibrarySession? = session /** @@ -81,12 +114,14 @@ class PlaybackService : MediaLibraryService() { override fun onDestroy() { flush() + player?.removeListener(completionListener) scope.cancel() session?.release() player?.release() session = null player = null writer = null + advancer = null super.onDestroy() } @@ -94,11 +129,73 @@ class PlaybackService : MediaLibraryService() { scope.launch { while (true) { delay(POSITION_TICK_MS) - if (player?.isPlaying == true) writer?.capture() + if (player?.isPlaying != true) continue + writer?.capture() + applyPlayheadPolicy() + } + } + } + + /** + * Acts on where the playhead is: an outro offset that ends the episode early, or a chapter + * the publisher marked as not-content. + * + * A tick means up to two seconds of a break plays before the jump. Living with that beats + * pre-empting it: seeking early on a boundary that turns out to be wrong removes audio the + * listener wanted, which is the one thing this feature must never do. + */ + private suspend fun applyPlayheadPolicy() { + val exo = player ?: return + val action = PlayheadPolicy.decide( + show = context.show, + chapters = context.chapters, + positionMs = exo.currentPosition, + durationMs = exo.duration.takeIf { it > 0 }, + progress = progress, + ) + when (action) { + is TickAction.SeekTo -> exo.seekTo(action.positionMs) + TickAction.Finish -> exo.currentMediaItem?.mediaId?.let(::finish) + TickAction.Nothing -> Unit + } + } + + /** Marks the episode played, moves the queue on, and starts what follows or stops. */ + private fun finish(episodeId: String) { + val current = advancer ?: return + scope.launch { + val next = current.finish(episodeId) + if (next == null) { + // Stopping matters because an episode can now finish before its file does. Left + // running, an outro offset marked the episode played and played the credits + // anyway -- the exact thing the offset exists to avoid. + player?.pause() + loadContextFor(null) + return@launch } + play(next) } } + /** Starts an episode on this service's own player, settings and all. */ + private fun play(next: NextUp) { + val exo = player ?: return + exo.setMediaItem( + next.episode.toMediaItem(next.show?.title, next.show?.author, next.show?.imageUrl), + next.startPositionMs, + ) + exo.prepare() + exo.applySpeed(next.speed, next.speed) + exo.play() + } + + /** Caches what the ticker needs for whatever is playing; the lookup is PlaybackAdvancer's. */ + private fun loadContextFor(episodeId: String?) { + context = PlayingContext() + val current = advancer ?: return + scope.launch { context = current.contextFor(episodeId) } + } + private fun flush() { val current = writer ?: return // Launched on the same scope so it is ordered behind any capture already in flight. diff --git a/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt b/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt index e44ff35..2296109 100644 --- a/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt +++ b/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt @@ -36,18 +36,6 @@ class PlayerConnection(private val context: Context) { private var controller: MediaController? = null - /** The resume rules. A field rather than a parameter: nothing ever passed a different one. */ - private val progress = PlaybackProgress() - - /** - * Fired when an episode plays to its end, with that episode's id. - * - * Exposed as a callback rather than handled here because what happens next -- mark played, - * advance the queue, start the next one -- is repository work this class has no business - * knowing about. - */ - var onEpisodeFinished: ((String) -> Unit)? = null - private val _nowPlaying = MutableStateFlow( NowPlaying(null, null, null, isPlaying = false, positionMs = 0, durationMs = null, speed = 1.0f) ) @@ -77,39 +65,22 @@ class PlayerConnection(private val context: Context) { override fun onIsPlayingChanged(isPlaying: Boolean) = publish() override fun onMediaItemTransition(mediaItem: MediaItem?, reason: Int) = publish() - override fun onPlaybackStateChanged(playbackState: Int) { - if (playbackState == Player.STATE_ENDED) { - // Read before anything else runs: advancing the queue replaces the media item, - // so the id is gone by the time the callback returns. - controller?.currentMediaItem?.mediaId?.let { finished -> - onEpisodeFinished?.invoke(finished) - } - } - publish() - } + // Completion is PlaybackService's job, not this connection's: it has to keep happening + // when the activity is gone and only the service is left playing. + override fun onPlaybackStateChanged(playbackState: Int) = publish() } /** - * Starts an episode where the listener should hear it. - * - * The decision belongs to PlaybackProgress, not here: a stored position past the end has to - * restart rather than seek past the end and stall, and a fresh start honours the show's - * intro offset. + * Starts an episode at a position already decided by PlaybackAdvancer. * - * The start position goes to `setMediaItem`, not to a `seekTo` after `prepare()`. Seeking + * The position goes to `setMediaItem`, not to a `seekTo` after `prepare()`. Seeking * afterwards is silently dropped across the controller-to-service boundary -- verified on a * device: an episode set to start sixty seconds in began at zero, and so, all along, did * every resume. Nothing failed and nothing logged; playback simply started at the top. */ - fun play( - item: MediaItem, - storedPositionMs: Long, - durationMs: Long?, - speed: Float, - skipIntroMs: Long = 0, - ) { + fun play(item: MediaItem, startPositionMs: Long, speed: Float) { val player = controller ?: return - player.setMediaItem(item, progress.resumePosition(storedPositionMs, durationMs, skipIntroMs)) + player.setMediaItem(item, startPositionMs) player.prepare() player.applySpeed(speed, speed) player.play() diff --git a/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt b/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt new file mode 100644 index 0000000..cdfc973 --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/PlayheadPolicy.kt @@ -0,0 +1,42 @@ +package org.terst.listen.playback + +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.feed.Chapter + +/** What the playback ticker should do about where the playhead currently is. */ +sealed interface TickAction { + data class SeekTo(val positionMs: Long) : TickAction + data object Finish : TickAction + data object Nothing : TickAction +} + +/** + * The tick decision, kept pure and away from anything with a lifecycle. + * + * This used to live in a view model scoped to the activity, which meant that swiping Listen off + * recents stopped the queue advancing and the outro firing while the audio carried on playing + * from the service. Deciding here and acting in the service keeps the two together. + */ +object PlayheadPolicy { + + fun decide( + show: SubscriptionEntity?, + chapters: List<Chapter>, + positionMs: Long, + durationMs: Long?, + progress: PlaybackProgress, + ): TickAction { + if (show == null) return TickAction.Nothing + + // Finishing beats skipping. A show with both an outro offset and a marked break at the + // end would otherwise seek into the credits it was told to skip, and sit there. + if (show.skipOutroMs > 0 && progress.isComplete(positionMs, durationMs, show.skipOutroMs)) { + return TickAction.Finish + } + + if (!show.skipAdChapters) return TickAction.Nothing + return AdChapters.skipTarget(chapters, positionMs, durationMs) + ?.let(TickAction::SeekTo) + ?: TickAction.Nothing + } +} diff --git a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt index c91a52f..1d9aa5d 100644 --- a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt @@ -7,15 +7,14 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.Graph import org.terst.listen.data.SubscriptionEntity import org.terst.listen.feed.Chapter -import org.terst.listen.playback.AdChapters import org.terst.listen.playback.NowPlaying -import org.terst.listen.playback.PlaybackProgress -import org.terst.listen.playback.PlaybackSpeed import org.terst.listen.playback.PlayerConnection import org.terst.listen.playback.SleepTimer import org.terst.listen.playback.toMediaItem @@ -37,19 +36,9 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) { _banner.value = null } - private val connection = PlayerConnection(app).also { connection -> - connection.connect() - // Without this the queue never moves on -- QueueRepository.advance existed with no - // caller, which is exactly how a queue stops being a queue. - connection.onEpisodeFinished = { episodeId -> onFinished(episodeId) } - } + private val connection = PlayerConnection(app).also { it.connect() } val nowPlaying: StateFlow<NowPlaying> = connection.nowPlaying - private val progress = PlaybackProgress() - - /** The episode whose outro has already been acted on, so the tick fires once per episode. */ - private var outroHandledFor: String? = null - /** Chapters for whatever is playing, empty when the show publishes none. */ private val _chapters = MutableStateFlow<List<Chapter>>(emptyList()) val chapters: StateFlow<List<Chapter>> = _chapters.asStateFlow() @@ -64,46 +53,31 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) { while (true) { delay(PROGRESS_TICK_MS) connection.publish() - finishIfPastTheOutro() - skipPastAnAdChapter() } } - } - /** - * Ends an episode early when its show has an outro offset set. - * - * Media3 reports STATE_ENDED at the real end of the file and nowhere else, so an offset that - * moves the finish line needs the tick to notice it. Guarded by episode id because the tick - * keeps running after the queue advances, and firing twice would mark the next episode - * played before a note of it had been played. - */ - private fun finishIfPastTheOutro() { - val outroMs = _currentShow.value?.skipOutroMs ?: return - if (outroMs <= 0) return - val playing = connection.nowPlaying.value - val episodeId = playing.episodeId ?: return - if (episodeId == outroHandledFor) return - if (!progress.isComplete(playing.positionMs, playing.durationMs, outroMs)) return - - outroHandledFor = episodeId - onFinished(episodeId) + // Follow whatever is playing rather than only what this screen started. The service + // moves the queue on by itself, so the show and chapters shown here have to come from + // the player, not from the last call to play(). + viewModelScope.launch { + connection.nowPlaying + .map { it.episodeId } + .distinctUntilChanged() + .collect { episodeId -> loadContext(episodeId) } + } } - /** - * Jumps over a chapter the publisher marked as not-content, when the show opts in. - * - * A tick means up to a second of the break plays before the jump. Living with that beats - * pre-empting it: seeking early on a chapter boundary that turns out to be wrong removes - * audio the listener wanted, and this feature only earns its place by never doing that. - */ - private fun skipPastAnAdChapter() { - if (_currentShow.value?.skipAdChapters != true) return - val playing = connection.nowPlaying.value - if (!playing.isPlaying) return - val target = AdChapters.skipTarget(_chapters.value, playing.positionMs, playing.durationMs) - ?: return - connection.seekTo(target) + private suspend fun loadContext(episodeId: String?) { + val episode = episodeId?.let { graph.episodes.getById(it) } + if (episode == null) { + _currentShow.value = null + _chapters.value = emptyList() + return + } + _currentShow.value = graph.subscriptions.getById(episode.subscriptionId) + // Cached on the episode after the first fetch, so this is a database read in the common + // case even though the service may have fetched it already. + _chapters.value = graph.chapters.chapters(episode) } override fun onCleared() { @@ -111,7 +85,13 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) { super.onCleared() } - /** Starts an episode, resuming where the listener left off and at the right speed. */ + /** + * Starts an episode the listener picked. + * + * Where to start it from is PlaybackAdvancer's answer, the same one the service uses when + * the queue moves on by itself -- so an episode begins in the same place however it got + * there. + */ fun play(episode: EpisodeEntity) { viewModelScope.launch { // Anything played becomes the head of the queue. Without that, finishing an @@ -119,45 +99,12 @@ class PlayerViewModel(app: Application) : AndroidViewModel(app) { // whatever was actually queued. graph.queue.playNow(episode.id) - val show = graph.subscriptions.getById(episode.subscriptionId) - _currentShow.value = show - val speed = PlaybackSpeed.forEpisode(episode.speedOverride, show?.playbackSpeed) - outroHandledFor = null - _chapters.value = emptyList() + val next = graph.advancer.start(episode.id) ?: return@launch connection.play( - item = episode.toMediaItem(showTitle = show?.title, author = show?.author, showImageUrl = show?.imageUrl), - storedPositionMs = episode.positionMs, - durationMs = episode.durationMs, - speed = speed, - skipIntroMs = show?.skipIntroMs ?: 0, + item = next.episode.toMediaItem(next.show?.title, next.show?.author, next.show?.imageUrl), + startPositionMs = next.startPositionMs, + speed = next.speed, ) - // After playback has started, never before: a chapter file is a second network - // round trip and nothing should wait on it to hear the episode. - _chapters.value = graph.chapters.chapters(episode) - } - } - - /** - * An episode reached its end: record it, drop it from the queue, start whatever is next. - * - * Marking played before advancing matters -- advance rewrites the queue, and doing it the - * other way round loses the id. - */ - private fun onFinished(episodeId: String) { - viewModelScope.launch { - graph.episodes.markPlayed(episodeId, System.currentTimeMillis()) - val nextId = graph.queue.advance() - val next = nextId?.let { graph.episodes.getById(it) } - if (next == null) { - // Stopping matters now that an episode can finish before its file does. Left - // running, an outro offset marked the episode played and then played the - // credits anyway, which is the exact thing the offset exists to avoid. When - // there is a next episode, starting it replaces the media item and has the - // same effect. - connection.pause() - return@launch - } - play(next) } } diff --git a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt new file mode 100644 index 0000000..e94bd06 --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt @@ -0,0 +1,236 @@ +package org.terst.listen.playback + +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.FakeEpisodeDao +import org.terst.listen.data.FakeQueueDao +import org.terst.listen.data.FakeSubscriptionDao +import org.terst.listen.data.QueueRepository +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.feed.Chapter + +/** + * Finishing an episode and deciding what follows it. + * + * This lived in a view model tied to the activity, so none of it happened once the activity was + * gone -- the queue stopped advancing mid-listen while the service played on. It is a class with + * fakes behind it now so the behaviour is pinned somewhere the lifecycle cannot reach. + */ +class PlaybackAdvancerTest { + + private val now = 1_700_000_000_000L + private val minute = 60_000L + + private lateinit var episodes: FakeEpisodeDao + private lateinit var subscriptions: FakeSubscriptionDao + private lateinit var queueDao: FakeQueueDao + private lateinit var queue: QueueRepository + private lateinit var advancer: PlaybackAdvancer + + @Before + fun setUp() { + episodes = FakeEpisodeDao() + subscriptions = FakeSubscriptionDao() + queueDao = FakeQueueDao() + queueDao.episodeSource = { episodes.rows.toList() } + queue = QueueRepository(queueDao) + advancer = PlaybackAdvancer(episodes, subscriptions, queue) { now } + } + + private suspend fun show( + id: String = "s1", + skipIntroMs: Long = 0, + playbackSpeed: Float? = null, + ): SubscriptionEntity { + val entity = SubscriptionEntity( + id = id, + feedUrl = "https://example.com/$id.xml", + title = "Show $id", + addedAtMs = 0, + skipIntroMs = skipIntroMs, + playbackSpeed = playbackSpeed, + ) + subscriptions.insert(entity) + return entity + } + + private fun episode( + id: String, + subscriptionId: String = "s1", + positionMs: Long = 0, + speedOverride: Float? = null, + ): EpisodeEntity { + val entity = EpisodeEntity( + id = id, + subscriptionId = subscriptionId, + guid = id, + title = id, + mediaUrl = "https://cdn/$id.mp3", + mediaType = "audio/mpeg", + durationMs = 30 * minute, + discoveredAtMs = 0, + positionMs = positionMs, + speedOverride = speedOverride, + ) + episodes.rows += entity + return entity + } + + private suspend fun queued() = queueDao.getEntries().map { it.episodeId } + + // --- finishing -------------------------------------------------------------------------- + + @Test + fun `finishing marks the episode played`() = runTest { + show() + episode("a") + queue.append("a") + + advancer.finish("a") + + assertEquals(now, episodes.rows.single().playedAtMs) + } + + @Test + fun `finishing drops it from the queue and reports what is next`() = runTest { + show() + episode("a") + episode("b") + queue.append("a") + queue.append("b") + + val next = advancer.finish("a") + + assertEquals("b", next?.episode?.id) + assertEquals(listOf("b"), queued()) + } + + /** Nothing left means nothing to start; the service stops rather than playing on. */ + @Test + fun `finishing the last episode reports nothing to play`() = runTest { + show() + episode("a") + queue.append("a") + + assertNull(advancer.finish("a")) + assertEquals(emptyList<String>(), queued()) + } + + /** + * Marking played before advancing matters: advance rewrites the queue, and doing it the + * other way round loses the id. + */ + @Test + fun `the finished episode is recorded even when the queue is empty`() = runTest { + show() + episode("a") + + assertNull(advancer.finish("a")) + assertEquals(now, episodes.rows.single().playedAtMs) + } + + // --- starting --------------------------------------------------------------------------- + + @Test + fun `a fresh episode starts past the show's intro`() = runTest { + show(skipIntroMs = 47_000) + episode("a") + + assertEquals(47_000L, advancer.start("a")?.startPositionMs) + } + + @Test + fun `a part-heard episode resumes where it was left`() = runTest { + show(skipIntroMs = 47_000) + episode("a", positionMs = 12 * minute) + + assertEquals(12 * minute, advancer.start("a")?.startPositionMs) + } + + @Test + fun `the show's speed is used, and a rule's beats it`() = runTest { + show(playbackSpeed = 1.5f) + episode("a") + episode("b", speedOverride = 1.0f) + + assertEquals(1.5f, advancer.start("a")?.speed) + assertEquals(1.0f, advancer.start("b")?.speed) + } + + @Test + fun `the next episode carries its own show's settings, not the finished one's`() = runTest { + show(id = "s1", skipIntroMs = 0) + show(id = "s2", skipIntroMs = 30_000, playbackSpeed = 2.0f) + episode("a", subscriptionId = "s1") + episode("b", subscriptionId = "s2") + queue.append("a") + queue.append("b") + + val next = advancer.finish("a") + + assertEquals(30_000L, next?.startPositionMs) + assertEquals(2.0f, next?.speed) + assertEquals("s2", next?.show?.id) + } + + @Test + fun `an episode that no longer exists starts nothing`() = runTest { + assertNull(advancer.start("gone")) + } + + // --- the context the ticker runs against -------------------------------------------------- + + @Test + fun `the context carries the episode's own show`() = runTest { + show(id = "s2") + episode("a", subscriptionId = "s2") + + assertEquals("s2", advancer.contextFor("a").show?.id) + } + + @Test + fun `nothing playing means an empty context`() = runTest { + val context = advancer.contextFor(null) + + assertNull(context.show) + assertEquals(emptyList<Chapter>(), context.chapters) + } + + /** Most shows publish no chapters, so a show that did not opt in is never asked for them. */ + @Test + fun `chapters are only fetched for a show that opted in`() = runTest { + var asked = 0 + val fetching = PlaybackAdvancer( + episodes, + subscriptions, + queue, + chaptersFor = { asked++; listOf(Chapter(startMs = 0, title = "One")) }, + ) { now } + + show(id = "s1") + episode("a") + assertEquals(emptyList<Chapter>(), fetching.contextFor("a").chapters) + assertEquals(0, asked) + + subscriptions.setSkipAdChapters("s1", true) + assertEquals(1, fetching.contextFor("a").chapters.size) + assertEquals(1, asked) + } + + /** An episode whose show was unsubscribed should still play rather than blocking the queue. */ + @Test + fun `an episode with no surviving show still starts`() = runTest { + episode("a", subscriptionId = "gone") + + val start = advancer.start("a") + + assertNotNull(start) + assertEquals(1.0f, start?.speed) + assertNull(start?.show) + } +} diff --git a/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt b/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt new file mode 100644 index 0000000..1488559 --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/PlayheadPolicyTest.kt @@ -0,0 +1,113 @@ +package org.terst.listen.playback + +import org.junit.Assert.assertEquals +import org.junit.Test +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.feed.Chapter + +/** + * What the playback ticker should do about where the playhead currently is. + * + * Pure, because this is the decision that used to live in a view model tied to the activity -- + * which meant it stopped happening the moment the activity went away, while the audio carried on. + */ +class PlayheadPolicyTest { + + private val minute = 60_000L + private val progress = PlaybackProgress() + + private val show = SubscriptionEntity( + id = "s1", + feedUrl = "https://example.com/feed.xml", + title = "A show", + addedAtMs = 0, + ) + + private val chapters = listOf( + Chapter(startMs = 0, title = "Headlines"), + Chapter(startMs = 2 * minute, title = "Sponsor", inToc = false), + Chapter(startMs = 3 * minute, title = "The interview"), + ) + + private fun decide( + show: SubscriptionEntity? = this.show, + chapters: List<Chapter> = emptyList(), + positionMs: Long, + durationMs: Long? = 30 * minute, + ) = PlayheadPolicy.decide(show, chapters, positionMs, durationMs, progress) + + @Test + fun `mid-episode, with nothing configured, there is nothing to do`() { + assertEquals(TickAction.Nothing, decide(positionMs = 10 * minute)) + } + + @Test + fun `an unknown show does nothing rather than guessing`() { + assertEquals(TickAction.Nothing, decide(show = null, positionMs = 10 * minute)) + } + + @Test + fun `past the outro line, the episode is finished`() { + assertEquals( + TickAction.Finish, + decide(show = show.copy(skipOutroMs = 10 * minute), positionMs = 25 * minute), + ) + } + + @Test + fun `short of the outro line, nothing happens`() { + assertEquals( + TickAction.Nothing, + decide(show = show.copy(skipOutroMs = 10 * minute), positionMs = 5 * minute), + ) + } + + /** Without an offset the real end is Media3's business, reported as STATE_ENDED. */ + @Test + fun `with no outro offset the ticker never finishes an episode itself`() { + assertEquals(TickAction.Nothing, decide(positionMs = 30 * minute - 1000)) + } + + @Test + fun `inside a marked break, on a show that opted in, it seeks past it`() { + assertEquals( + TickAction.SeekTo(3 * minute), + decide( + show = show.copy(skipAdChapters = true), + chapters = chapters, + positionMs = 2 * minute + 5_000, + ), + ) + } + + @Test + fun `a show that did not opt in keeps playing the break`() { + assertEquals( + TickAction.Nothing, + decide(show = show, chapters = chapters, positionMs = 2 * minute + 5_000), + ) + } + + /** + * Finishing beats skipping. + * + * A show with both an outro offset and a marked break at the end would otherwise seek into + * the credits it was told to skip, and sit there. + */ + @Test + fun `the outro wins when a break sits on the finish line`() { + val trailing = listOf( + Chapter(startMs = 0, title = "Content"), + Chapter(startMs = 18 * minute, title = "Outro ad", inToc = false), + ) + // Duration 30m less a 10m outro puts the finish line at 20m, and the break starts at 18m. + assertEquals( + TickAction.Finish, + decide( + show = show.copy(skipOutroMs = 10 * minute, skipAdChapters = true), + chapters = trailing, + positionMs = 19 * minute + 40_000, + ), + ) + } +} |