diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-09-25 19:07:04 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-09-25 19:07:04 +0000 |
| commit | 4965d5971c1443ee69af42b9f86920fd6121fea5 (patch) | |
| tree | abb32766511b063154fc167dc0c4f3793200e45c | |
| parent | 3f650131627f6737e01585b97519e03413119b44 (diff) | |
Advance the queue from the service, not from the activity
Marking an episode played and moving the queue on lived in PlayerViewModel,
which is scoped to the activity and releases its MediaController in onCleared.
So with the activity gone -- swiped off recents, or reclaimed under memory
pressure -- the audio carried on from the foreground service while nothing
marked anything played and nothing advanced the queue. Background listening with
the screen off is the normal case for a podcast app, so this was broken in the
case that matters most. The outro offset and ad-chapter skipping added last
commit rode the same tick and had the same hole.
Completion now belongs to PlaybackService, which is the component that survives
backgrounding. Two new tested classes carry the thinking: PlayheadPolicy decides
what a tick means (finish early on an outro offset, seek past a marked break, or
nothing -- with finishing beating skipping, or a show with both would seek into
the credits it was told to skip and sit there), and PlaybackAdvancer owns
finishing, advancing, and working out where the next episode starts. Neither
knows about Media3, so both are testable off-device; the service turns their
answers into player calls.
Where an episode starts is now decided in exactly one place. PlaybackAdvancer
.start is used by the service when the queue moves on and by the view model when
you tap Play, so an episode begins in the same position however it got there.
PlayerConnection no longer carries its own copy of the resume rules.
PlayerConnection deliberately stops handling STATE_ENDED and PlayerViewModel
loses onFinished entirely -- with both in place the queue would advance twice.
The view model now follows whatever is playing rather than only what it started,
so the player screen's show, speed and chapter list stay right when the service
moves on by itself.
Verified on a device with "don't keep activities" enabled, which destroys the
activity the moment it is backgrounded: an episode was resumed, the app sent to
home with no activity records left, and the service took the episode to its
outro line, marked it played, dropped it from the queue and started the next
one, which went on playing.
Also isolated the data loss seen twice this session: connectedDebugAndroidTest
uninstalls the app when it finishes, taking the database with it. A plain adb
install -r left four episodes in place and the next instrumented run left
"run-as: unknown package". Not a migration fault, and now written down as such.
No permission changes, no schema change. 385 unit tests green, detekt and lint
clean. Coverage 66.23 -> 65.33 with an exception recorded: the logic moved out of
the coverage-excluded ui/** tree into the counted service, and the ExoPlayer glue
that remains cannot run off-device.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B
| -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, + ), + ) + } +} |
