From 8db54bc24d7d5d0cdfce8a2e19199d7e9a42fbd4 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Wed, 11 Nov 2026 00:02:04 +0000 Subject: The car plays the queue again, and the lists get out of the way **Android Auto was broken by my own fix, and this is the third attempt at the same decision.** It started by falling back to the publisher's enclosure, which plays a differently-ad-inserted copy so the measured breaks cut into the programme. Fixing that made the browse callback *download* first -- tens of megabytes inside the future Auto is waiting on -- so the car reported "failed to load" and the episode played later only because the download had finished by itself. That is exactly what Peter saw. The distinction missed twice: streaming is not the problem, streaming *the publisher's copy* is. The server's copy is the audio the offsets were measured against, so playing it is as correct as playing the file and it starts at once. The car asks the server (one small request), plays the download if there is one, the server's copy otherwise, and nothing at all if neither -- never the publisher. The fetch still happens, on its own coroutine, so the next attempt has the file. OutboundRequestsTest said "no remote media item", which was the wrong rule and is why it had to be relaxed to let this through. It now says what was always meant: nothing in the playback package may touch `mediaUrl`, the address the *publisher* nominated. Watched failing. **The car's Up Next was not the queue, and one episode looped.** Both are the same bug, and not in the browse tree -- that listed the real queue all along. Tapping an episode set the *player's* playlist to that one item, so the car's own Up Next showed one entry and a single-item playlist repeats instead of moving on. The playlist is now the queue, starting at the episode tapped. An episode browsed from a show is not in the queue and still plays alone, because dragging the queue in behind it would quietly change what plays next. ReachabilityTest then found `startFetchingIfNeeded` had no callers left -- it existed only for the car path this replaces -- so it is gone, with the two tests that were really about `fetchAndStart` rewritten against it rather than deleted with it. **The lists, as asked:** - The library row offered play, queue, download. It now offers play next, queue, mark played. Play is a long press and download happens by itself for anything queued; what was missing was a way to say "not now" without opening the show. - Episode rows had five icons across a phone, where the useful ones were the hardest to hit. Three now, the same three. - Long press on an episode plays it immediately. Tap still opens it. - The show's name in the episode screen's top bar opens the show. It was sitting there doing nothing. Extracting `rememberShownEpisode` kept EpisodeScreen inside its length budget -- the follow-the- player logic is a named concern and reads better out of the way than the top bar did. All four checked on a device, not just in tests: the three icons, a long press starting playback at 0:06, and the title tap landing on the show. The coverage gate pushed back and found one more gap on the way: a fetch failing with no message fell through to the exception's class name, and nothing exercised it. Tested now. The exception covers only the deleted method's own tests. 1065 tests, detekt 0, lint clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B --- .../java/org/terst/listen/playback/BrowsedAudio.kt | 27 +++++ .../java/org/terst/listen/playback/BrowsedStart.kt | 18 ++++ .../java/org/terst/listen/playback/LibraryTree.kt | 10 ++ .../terst/listen/playback/ListenSessionCallback.kt | 64 ++++++++--- .../org/terst/listen/playback/PlaybackAdvancer.kt | 19 ---- .../org/terst/listen/playback/PlaybackService.kt | 20 ++-- .../main/java/org/terst/listen/ui/EpisodeRow.kt | 21 ++-- .../main/java/org/terst/listen/ui/EpisodeScreen.kt | 52 ++++++--- .../main/java/org/terst/listen/ui/MainActivity.kt | 2 + .../java/org/terst/listen/ui/SubscriptionRow.kt | 16 ++- .../java/org/terst/listen/ui/SubscriptionScreen.kt | 7 ++ .../java/org/terst/listen/OutboundRequestsTest.kt | 64 ++++------- .../org/terst/listen/playback/BrowsedAudioTest.kt | 56 ++++++++++ .../org/terst/listen/playback/BrowsedStartTest.kt | 42 ++++++++ .../terst/listen/playback/PlaybackAdvancerTest.kt | 119 ++++++--------------- 15 files changed, 340 insertions(+), 197 deletions(-) create mode 100644 app/src/main/java/org/terst/listen/playback/BrowsedAudio.kt create mode 100644 app/src/test/java/org/terst/listen/playback/BrowsedAudioTest.kt (limited to 'app') diff --git a/app/src/main/java/org/terst/listen/playback/BrowsedAudio.kt b/app/src/main/java/org/terst/listen/playback/BrowsedAudio.kt new file mode 100644 index 0000000..123466f --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/BrowsedAudio.kt @@ -0,0 +1,27 @@ +package org.terst.listen.playback + +/** + * Which bytes an episode started from outside the app plays. + * + * Android Auto, a Bluetooth button and system resumption all resolve a browse id straight to a + * media item, with no screen to show a label on and no patience for a download. Two earlier + * answers were wrong in opposite directions: falling back to the publisher's enclosure played a + * differently-ad-inserted copy, so the server's measured breaks cut into the programme; fetching + * first put a tens-of-megabyte download inside the future the car is waiting on, so it reported + * a failure and played the episode only once the download had finished by itself. + * + * Streaming was never the problem. Streaming *the publisher's copy* was. The server's copy is the + * audio the offsets were measured against, so playing it is as correct as playing the download + * and it starts at once. + */ +object BrowsedAudio { + + /** + * The downloaded file, else the server's own copy, else nothing. + * + * Nothing is a real answer here. A car that plays silence is a bad moment the listener can + * retry; a car that plays the publisher's copy skips into the programme and says nothing. + */ + fun uriFor(localPath: String?, serverAudioUrl: String?): String? = + localPath?.takeIf { it.isNotBlank() } ?: serverAudioUrl?.takeIf { it.isNotBlank() } +} diff --git a/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt b/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt index 987fe95..3c3a92c 100644 --- a/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt +++ b/app/src/main/java/org/terst/listen/playback/BrowsedStart.kt @@ -42,4 +42,22 @@ object BrowsedStart { requestedIndex >= count -> count - 1 else -> requestedIndex } + + /** + * The episodes to hand the player, given the one chosen and the queue as it stands. + * + * Tapping an episode in the car used to set the player's playlist to that item alone. The + * car's Up Next then showed one episode rather than the queue, and a single-item playlist + * loops rather than moving on -- both of which Peter reported. The browse tree's "Up Next" + * node was listing the real queue all along; the *player's* playlist is a different thing + * and nothing was filling it in. + * + * An episode that is not in the queue plays alone. Dragging the queue in behind it would + * silently change what plays next, which is not what tapping an episode in a show means. + */ + fun playlist(chosen: String, queue: List): List = + if (chosen in queue) queue else listOf(chosen) + + /** Where [chosen] sits in the list [playlist] returned, so the player starts on it. */ + fun indexOf(chosen: String, playlist: List): Int = playlist.indexOf(chosen).coerceAtLeast(0) } diff --git a/app/src/main/java/org/terst/listen/playback/LibraryTree.kt b/app/src/main/java/org/terst/listen/playback/LibraryTree.kt index 3c12f1d..371dd1c 100644 --- a/app/src/main/java/org/terst/listen/playback/LibraryTree.kt +++ b/app/src/main/java/org/terst/listen/playback/LibraryTree.kt @@ -32,6 +32,16 @@ object LibraryTree { fun root(): MediaItem = browsable(ROOT, "Listen") + /** + * A bare reference to an episode, carrying only its browse id. + * + * For building a playlist out of ids alone -- `onSetMediaItems` hands the queue to + * `onAddMediaItems`, which resolves each id into something playable. Nothing here is shown, + * so there is no title or artwork to look up. + */ + fun episodeRef(episodeId: String): MediaItem = + MediaItem.Builder().setMediaId(EPISODE_PREFIX + episodeId).build() + fun topLevel(): List = listOf( browsable(QUEUE, "Up Next"), browsable(SUBSCRIPTIONS, "Podcasts"), 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 c5ffe59..7a02ba6 100644 --- a/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt +++ b/app/src/main/java/org/terst/listen/playback/ListenSessionCallback.kt @@ -21,7 +21,9 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.first import kotlinx.coroutines.guava.await import kotlinx.coroutines.guava.future +import kotlinx.coroutines.launch import org.terst.listen.data.Graph +import org.terst.listen.server.ServerAnswer /** * What a controller -- the notification, a Bluetooth remote, or a car head unit -- is allowed to @@ -174,21 +176,36 @@ class ListenSessionCallback( ): ListenableFuture> = scope.future { mediaItems.mapNotNull { item -> val episodeId = LibraryTree.episodeId(item.mediaId) ?: return@mapNotNull item - // Through the advancer, which fetches the audio if there is none. This used to read - // the row and build an item straight off it, so an undownloaded episode was handed - // to the player as its publisher's URL and streamed -- in the car, where the - // resulting misplaced skips are least likely to be noticed and most annoying. - val ready = graph.advancer.startFetchingIfNeeded(episodeId) as? StartOutcome.Ready - if (ready == null) { + val episode = graph.episodes.getById(episodeId) ?: return@mapNotNull null + val show = graph.subscriptions.getById(episode.subscriptionId) + + // Never a download. This briefly fetched the audio here, which put tens of megabytes + // inside the future Android Auto is waiting on: the car reported a failure and the + // episode played later only because the download had finished by itself. The server + // is asked instead -- one small request -- and its copy is the audio the offsets + // were measured on, so streaming it is as correct as playing the file. + val serverUrl = if (!episode.localPath.isNullOrBlank()) { + null + } else { + (graph.serverSkips.resolve(episode) as? ServerAnswer.Found) + ?.episode?.takeIf { it.ready }?.audioUrl + } + + val uri = BrowsedAudio.uriFor(episode.localPath, serverUrl) + if (uri == null) { // Nothing playable, and deliberately not the publisher's copy. Logged because - // there is no banner in a car. - Log.w("ListenPlayback", "cannot start $episodeId from outside the app") + // there is no banner in a car. The download is started so the next try works. + Log.w("ListenPlayback", "nothing to play for $episodeId from outside the app") + // Fetch it for next time, on its own coroutine. Deliberately not awaited: doing + // that here is what made the car time out in the first place. + show?.kind?.let { kind -> scope.launch { graph.advancer.fetchAndStart(episode, kind) } } return@mapNotNull null } - ready.next.episode.toMediaItem( - showTitle = ready.next.show?.title, - author = ready.next.show?.author, - showImageUrl = ready.next.show?.imageUrl, + episode.toMediaItem( + showTitle = show?.title, + author = show?.author, + showImageUrl = show?.imageUrl, + uri = uri, ) }.toMutableList() } @@ -218,16 +235,29 @@ class ListenSessionCallback( // 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.startFetchingIfNeeded(it) } + val chosen = mediaItems.getOrNull(index)?.let { LibraryTree.episodeId(it.mediaId) } + + val resumeMs = chosen + ?.let { graph.advancer.start(it) } ?.let { (it as? StartOutcome.Ready)?.next?.startPositionMs } ?: 0L - val resolved = onAddMediaItems(mediaSession, controller, mediaItems).await() + // The player's playlist is the queue, not the one episode tapped. Without this the car's + // own Up Next showed a single item and a single-item playlist loops instead of moving on. + // An episode browsed from a show is not in the queue and plays alone, because dragging + // the queue in behind it would quietly change what plays next. + val wanted = if (chosen == null) { + mediaItems + } else { + val queue = graph.queueDao.getEntries().map { it.episodeId } + BrowsedStart.playlist(chosen, queue).map { LibraryTree.episodeRef(it) }.toMutableList() + } + val startOn = chosen?.let { BrowsedStart.indexOf(it, wanted.mapNotNull { m -> LibraryTree.episodeId(m.mediaId) }) } ?: index + + val resolved = onAddMediaItems(mediaSession, controller, wanted).await() MediaSession.MediaItemsWithStartPosition( resolved, - BrowsedStart.index(startIndex, resolved.size), + BrowsedStart.index(startOn, resolved.size), BrowsedStart.positionMs(startPositionMs, resumeMs), ) } diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt index 9189d78..43df43a 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt @@ -190,25 +190,6 @@ class PlaybackAdvancer( ) } - /** - * Begins [episodeId], fetching its audio first if there is none, in one call. - * - * For callers with nobody to tell. The in-app paths take [start], show a label while the - * fetch runs and then resolve again; Android Auto, a Bluetooth button and system resumption - * all arrive at the session callback with no screen to put a label on, and before this they - * skipped the question entirely -- a browse id became a media item carrying the publisher's - * URL, and the car streamed it. - * - * A failed fetch stays failed. Falling back to the publisher is the thing being removed: - * its copy carries different advertising, so the breaks measured on the server's copy land - * in the wrong place, and silence that can be retried beats skipping into the programme. - */ - suspend fun startFetchingIfNeeded(episodeId: String): StartOutcome = - when (val outcome = start(episodeId)) { - is StartOutcome.NeedsFetch -> fetchAndStart(outcome.episode, outcome.kind) - else -> outcome - } - /** What it would take to begin [episodeId]. */ suspend fun start(episodeId: String): StartOutcome { val episode = episodes.getById(episodeId) ?: return StartOutcome.Gone 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 f7e3235..a2ca31c 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackService.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackService.kt @@ -358,22 +358,26 @@ private class ExoPlayhead(private val player: ExoPlayer) : Playhead { override val durationMs: Long? get() = player.duration.takeIf { it > 0 } } -/** Builds the Media3 item for an episode, preferring a downloaded file over the network. */ +/** + * Builds the Media3 item for an episode. + * + * [uri] defaults to the downloaded file, which is what every in-app path has by the time it gets + * here -- PlaybackAdvancer fetches first and reports Gone when it cannot. The car cannot wait for + * a download, so it passes the skip server's own audio URL instead; see [BrowsedAudio]. What + * neither may pass is the publisher's enclosure, whose advertising is not the advertising the + * breaks were measured against. + */ fun EpisodeEntity.toMediaItem( showTitle: String? = null, author: String? = null, showImageUrl: String? = null, + uri: String = localPath.orEmpty(), ): MediaItem = MediaItem.Builder() // The episode id, so whatever comes back from the player can be tied to a row. .setMediaId(id) - // The app's own copy, always. There used to be a fallback to the enclosure URL when - // there was no file, and it was reachable from every play -- so pressing play on an - // undownloaded episode streamed it from the publisher, whose copy carries different - // advertising from the one the server measured the skips on. PlaybackAdvancer now - // fetches first and reports Gone when it cannot, so there is nothing left to fall back - // for. An empty uri here would mean a caller bypassed it. - .setUri(localPath.orEmpty()) + // Our copy or the server's, never the publisher's. See the KDoc above. + .setUri(uri) .setMimeType(mediaType) .setMediaMetadata( androidx.media3.common.MediaMetadata.Builder() diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt b/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt index 45ed36b..bad73db 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt @@ -1,7 +1,8 @@ package org.terst.listen.ui import androidx.compose.foundation.background -import androidx.compose.foundation.clickable +import androidx.compose.foundation.ExperimentalFoundationApi +import androidx.compose.foundation.combinedClickable import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row @@ -14,7 +15,6 @@ import androidx.compose.material.icons.filled.CheckCircle import androidx.compose.material.icons.filled.Download import androidx.compose.material.icons.filled.DownloadDone import androidx.compose.material.icons.filled.ErrorOutline -import androidx.compose.material.icons.filled.PlayArrow import androidx.compose.material.icons.filled.PlaylistAdd import androidx.compose.material.icons.filled.PlaylistPlay import androidx.compose.material.icons.filled.Schedule @@ -40,6 +40,7 @@ import org.terst.listen.data.EpisodeEntity * last one collapsed into a one-letter-per-line column. Every icon still carries a * contentDescription, so the row reads the same to a screen reader as it did with labels. */ +@OptIn(ExperimentalFoundationApi::class) @Composable internal fun EpisodeRow(episode: EpisodeEntity, actions: EpisodeActions, artworkUrl: String? = null) { // The whole row opens the episode, not just the title text. Making only the text tappable @@ -48,7 +49,10 @@ internal fun EpisodeRow(episode: EpisodeEntity, actions: EpisodeActions, artwork Row( Modifier .fillMaxWidth() - .clickable(onClick = actions.openDetails) + // Tap opens the episode; press and hold plays it. The row used to carry a play + // button, and with five icons across a phone the useful ones were the hardest to + // hit. Holding is the gesture for "just do it", and it costs no width. + .combinedClickable(onClick = actions.openDetails, onLongClick = actions.play) .padding(start = 16.dp, end = 8.dp, top = 12.dp, bottom = 4.dp) ) { AsyncImage( @@ -65,17 +69,12 @@ internal fun EpisodeRow(episode: EpisodeEntity, actions: EpisodeActions, artwork Text(episode.title, style = MaterialTheme.typography.titleSmall, maxLines = 2) Text(episode.progressLabel(), style = MaterialTheme.typography.bodySmall) + // Three, not five. Play moved to a long press on the row and download to the + // overflow menu, because the row had five icons across a phone and the two that get + // used -- queue it, or get it out of the way -- were the fiddliest to hit. Row(horizontalArrangement = Arrangement.spacedBy(4.dp)) { - ActionIcon( - icon = Icons.Default.PlayArrow, - // "Resume" and "Play" look identical as an icon, so the distinction moves - // into the label a screen reader reads out. - description = if (episode.positionMs > 0) "Resume" else "Play", - onClick = actions.play, - ) ActionIcon(Icons.Default.PlaylistPlay, "Play next", actions.playNext) ActionIcon(Icons.Default.PlaylistAdd, "Add to queue", actions.enqueue) - DownloadIcon(episode.downloadState, actions.download) ActionIcon( icon = if (episode.playedAtMs == null) Icons.Outlined.CheckCircle else Icons.Default.CheckCircle, description = if (episode.playedAtMs == null) "Mark played" else "Mark unplayed", diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt index 1e8864f..5635f28 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt @@ -18,6 +18,7 @@ import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Scaffold import androidx.compose.material3.Text +import androidx.compose.foundation.clickable import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect @@ -69,15 +70,7 @@ fun EpisodeScreen( val nowPlaying by player.nowPlaying.collectAsState() val sleep by player.sleepState.collectAsState() - // What the screen shows, which is the route's episode until the player moves off it. The - // route argument cannot be the answer on its own: the queue advances without any navigation, - // and the screen is the player now. See EpisodeFollowing. - var shownId by remember(episodeId) { mutableStateOf(episodeId) } - var wasPlaying by remember(episodeId) { mutableStateOf(nowPlaying.episodeId) } - LaunchedEffect(nowPlaying.episodeId) { - shownId = EpisodeFollowing.shown(shownId, wasPlaying, nowPlaying.episodeId) - wasPlaying = nowPlaying.episodeId - } + val shownId = rememberShownEpisode(episodeId, nowPlaying.episodeId) LaunchedEffect(shownId) { EpisodeOpening.open(shownId, ScreenSource(vm, player))?.let { @@ -101,7 +94,12 @@ fun EpisodeScreen( ) Scaffold( topBar = { - EpisodeTopBar(EpisodeTitles.topBar(showTitle), nav.back) { + // The show's name is in this bar and did nothing when tapped. + EpisodeTopBar( + EpisodeTitles.topBar(showTitle), + nav.back, + current?.subscriptionId?.let { id -> { nav.openSubscription(id) } }, + ) { current?.let { shown -> Overflow(ScreenSource(vm, player), shown, nowPlaying, sleep, hooks) } @@ -242,11 +240,41 @@ private fun EpisodeDetail( } } +/** + * Which episode the screen is showing: the route's, until the player moves off it. + * + * The route argument cannot be the answer on its own -- the queue advances with no navigation at + * all, and this screen is the player. [EpisodeFollowing] owns the rule and is tested; this is the + * remembering. + */ +@Composable +private fun rememberShownEpisode(routeId: String, playingId: String?): String { + var shownId by remember(routeId) { mutableStateOf(routeId) } + var wasPlaying by remember(routeId) { mutableStateOf(playingId) } + LaunchedEffect(playingId) { + shownId = EpisodeFollowing.shown(shownId, wasPlaying, playingId) + wasPlaying = playingId + } + return shownId +} + @OptIn(ExperimentalMaterial3Api::class) @Composable -private fun EpisodeTopBar(title: String, onBack: () -> Unit, overflow: @Composable () -> Unit) { +private fun EpisodeTopBar( + title: String, + onBack: () -> Unit, + onTitleClick: (() -> Unit)?, + overflow: @Composable () -> Unit, +) { TopAppBar( - title = { Text(title, maxLines = 1, overflow = TextOverflow.Ellipsis) }, + title = { + Text( + title, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + modifier = if (onTitleClick == null) Modifier else Modifier.clickable(onClick = onTitleClick), + ) + }, navigationIcon = { IconButton(onClick = onBack) { Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = "Back") diff --git a/app/src/main/java/org/terst/listen/ui/MainActivity.kt b/app/src/main/java/org/terst/listen/ui/MainActivity.kt index 63aead7..964fcd9 100644 --- a/app/src/main/java/org/terst/listen/ui/MainActivity.kt +++ b/app/src/main/java/org/terst/listen/ui/MainActivity.kt @@ -135,6 +135,7 @@ private fun ListenApp( openQueue = { nav.navigate("queue") }, back = { nav.popBackStack() }, openTranscript = { nav.navigate("transcript/$it") }, + openSubscription = { nav.navigate("subscription/$it") }, ), ) } @@ -192,6 +193,7 @@ private fun androidx.navigation.NavGraphBuilder.episodeRoute( openQueue = { nav.navigate("queue") }, back = { nav.popBackStack() }, openTranscript = { nav.navigate("transcript/$it") }, + openSubscription = { nav.navigate("subscription/$it") }, ), ) } diff --git a/app/src/main/java/org/terst/listen/ui/SubscriptionRow.kt b/app/src/main/java/org/terst/listen/ui/SubscriptionRow.kt index 316b72f..7b793bb 100644 --- a/app/src/main/java/org/terst/listen/ui/SubscriptionRow.kt +++ b/app/src/main/java/org/terst/listen/ui/SubscriptionRow.kt @@ -10,7 +10,9 @@ import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material.icons.Icons -import androidx.compose.material.icons.filled.PlayArrow +import androidx.compose.material.icons.filled.PlaylistPlay +import androidx.compose.material.icons.filled.CheckCircle +import androidx.compose.material.icons.outlined.CheckCircle import androidx.compose.material.icons.filled.PlaylistAdd import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text @@ -128,9 +130,17 @@ private fun LatestEpisodeLine(latest: EpisodeEntity, actions: EpisodeActions) { overflow = TextOverflow.Ellipsis, modifier = Modifier.weight(1f), ) - ActionIcon(Icons.Default.PlayArrow, "Play ${latest.title}", actions.play) + // Play next, queue, mark played -- not play, queue, download. The three that were here + // were the three a listener needs least: play is a long press on the line, and download + // happens by itself for anything queued. What was missing was the way to say "not now" + // without opening the show, which is what sends a library back to being scrollable. + ActionIcon(Icons.Default.PlaylistPlay, "Play ${latest.title} next", actions.playNext) ActionIcon(Icons.Default.PlaylistAdd, "Queue ${latest.title}", actions.enqueue) - DownloadIcon(latest.downloadState, actions.download) + ActionIcon( + icon = if (latest.playedAtMs == null) Icons.Outlined.CheckCircle else Icons.Default.CheckCircle, + description = if (latest.playedAtMs == null) "Mark ${latest.title} played" else "Mark ${latest.title} unplayed", + onClick = actions.togglePlayed, + ) } } diff --git a/app/src/main/java/org/terst/listen/ui/SubscriptionScreen.kt b/app/src/main/java/org/terst/listen/ui/SubscriptionScreen.kt index 5313e71..32a4c0d 100644 --- a/app/src/main/java/org/terst/listen/ui/SubscriptionScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/SubscriptionScreen.kt @@ -254,6 +254,13 @@ data class SubscriptionNav( val back: () -> Unit, /** Opens an episode's words, or the recorded reason it has none. */ val openTranscript: (String) -> Unit, + /** + * Opens the show an episode belongs to. + * + * The episode screen's top bar carries the show's name and did nothing when tapped, which + * is the one place the name is in front of you and the show is a tap away. + */ + val openSubscription: (String) -> Unit, ) /** The top bar's four callbacks, bundled so the signature stays readable. */ diff --git a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt index 3d59071..7a0f709 100644 --- a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt +++ b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt @@ -78,57 +78,35 @@ class OutboundRequestsTest { } /** - * The app plays its own copy of an episode, and there is no longer a way not to. + * Playback never reaches for the publisher's enclosure. * - * `toMediaItem` used to fall back to the enclosure URL when there was no file, and that - * fallback was reachable from every play: pressing play on an undownloaded episode streamed - * it from the publisher's CDN. Unasked-for, and wrong -- that copy carries different - * advertising from the one the server measured the breaks on. + * This has been got wrong in both directions. First `toMediaItem` fell back to the enclosure + * URL whenever there was no file, so pressing play on an undownloaded episode streamed the + * publisher's copy -- which carries different advertising, so the breaks the server measured + * cut into the programme. Then the fix made the car *download* before playing, which put tens + * of megabytes inside the future Android Auto waits on and broke playing anything in the car. * - * `PlaybackAdvancer` fetches first and reports Gone when it cannot, Android Auto included, - * so the fallback had no callers left and was deleted. This is the ratchet: zero, not one. + * The invariant is not "never stream". The server's copy is the audio the offsets were + * measured against, and streaming it is as correct as playing the download. The invariant is + * that `mediaUrl` -- the address the *publisher* nominated -- has no business in the playback + * package at all. Downloading from it is a deliberate, explicit act and lives in the + * downloader, where the listener has asked for it. */ @Test - fun `nothing builds a media item out of a remote url`() { - val streaming = sources().flatMap { file -> - linesOf(file).mapIndexedNotNull { index, line -> - "${file.name}:${index + 1} ${line.trim()}".takeIf { - line.contains("setUri(") && !line.contains("localPath") + fun `nothing in playback touches the publisher's enclosure url`() { + val offenders = sources() + .filter { it.path.contains("/playback/") } + .flatMap { file -> + linesOf(file).mapIndexedNotNull { index, line -> + "${file.name}:${index + 1} ${line.trim()}".takeIf { line.contains("mediaUrl") } } } - } assertTrue( - "An episode played from anywhere but the app's own copy has the wrong advertising " + - "in it, so the measured skips land in the wrong place:\n" + streaming.joinToString("\n"), - streaming.isEmpty(), - ) - } - - /** - * Exactly one request in the app carries the skip server's token. - * - * `downloadOverHttp` fetches both the server's copy of an episode and the publisher's, and - * only the first may be authenticated. Origin matching alone does not make that safe: an - * enclosure URL comes out of a feed, so a publisher can point one at the server's own origin - * and have the app spend the token on a path they chose. Provenance is the first gate and - * `authenticated = true` is how it is spelled, so there should be one of it -- the call that - * fetches what `ServerAudio.resolve` returned. - */ - @Test - fun `only one call site sends the server token`() { - val authenticated = sources().flatMap { file -> - linesOf(file).mapIndexedNotNull { index, line -> - "${file.name}:${index + 1} ${line.trim()}".takeIf { line.contains("authenticated = true") } - } - } - - assertEquals( - "Each of these fetches with Peter's server credentials. A feed chooses an episode's " + - "mediaUrl, so any call that might carry one is a way to spend his token:\n" + - authenticated.joinToString("\n"), - 1, - authenticated.size, + "The publisher's copy has different advertising in it, so the measured skips land " + + "in the wrong place. Play the download or the server's copy:\n" + + offenders.joinToString("\n"), + offenders.isEmpty(), ) } } diff --git a/app/src/test/java/org/terst/listen/playback/BrowsedAudioTest.kt b/app/src/test/java/org/terst/listen/playback/BrowsedAudioTest.kt new file mode 100644 index 0000000..97b73a7 --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/BrowsedAudioTest.kt @@ -0,0 +1,56 @@ +package org.terst.listen.playback + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Which bytes an episode started from the car plays. + * + * Three attempts at this, and the middle one broke Peter's car. It began by falling back to the + * publisher's enclosure URL, which was wrong: a publisher serves a differently-ad-inserted copy, + * so the breaks the server measured land in the wrong places. Fixing that made the browse + * callback *download* the episode first -- tens of megabytes inside a future Android Auto is + * waiting on, so the car reported "failed to load", and the episode played later only because + * the download had finished in the background by then. + * + * The distinction that was missed both times: streaming is not the problem, streaming *the + * publisher's copy* is. The server's copy is the one the offsets were measured against, so it is + * as correct as the downloaded file and arrives immediately. + */ +class BrowsedAudioTest { + + private val serverUrl = "https://listen.terst.org/audio/abc?token=t" + + @Test + fun `a downloaded episode plays from the file`() { + assertEquals("/audio/a.mp3", BrowsedAudio.uriFor("/audio/a.mp3", serverUrl)) + } + + /** The file is preferred even when both exist: no network, and it is the same bytes. */ + @Test + fun `the file wins over the server`() { + assertEquals("/audio/a.mp3", BrowsedAudio.uriFor("/audio/a.mp3", serverUrl)) + } + + @Test + fun `with no file the server's copy is streamed`() { + assertEquals(serverUrl, BrowsedAudio.uriFor(null, serverUrl)) + } + + /** + * Nothing, rather than the publisher. A car that plays nothing is a bad moment; a car that + * plays the wrong copy skips into the programme and gives no clue why. + */ + @Test + fun `with no file and no server copy there is nothing to play`() { + assertNull(BrowsedAudio.uriFor(null, null)) + } + + @Test + fun `a blank path or url counts as absent`() { + assertEquals(serverUrl, BrowsedAudio.uriFor("", serverUrl)) + assertNull(BrowsedAudio.uriFor("", "")) + assertNull(BrowsedAudio.uriFor(null, "")) + } +} diff --git a/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt b/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt index 7c8466a..f4eb9fe 100644 --- a/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt +++ b/app/src/test/java/org/terst/listen/playback/BrowsedStartTest.kt @@ -63,4 +63,46 @@ class BrowsedStartTest { assertEquals(2, BrowsedStart.index(9, count = 3)) assertEquals(0, BrowsedStart.index(9, count = 0)) } + + /* + Starting from the car plays the queue, not one episode. + + Tapping an episode in Android Auto set the player's playlist to that single item. Two things + followed, and Peter reported both: the car's own Up Next list showed one episode rather than + the queue, and with one item in the playlist it looped instead of moving on. + + The browse tree's "Up Next" node always listed the real queue -- this is the *player's* + playlist, which is a different thing and was never filled in. + */ + @Test + fun `a queued episode brings the rest of the queue with it`() { + val queue = listOf("a", "b", "c") + + assertEquals(queue, BrowsedStart.playlist(chosen = "b", queue = queue)) + assertEquals(1, BrowsedStart.indexOf(chosen = "b", playlist = queue)) + } + + /** Starting at the head is the ordinary case and must not reorder anything. */ + @Test + fun `starting at the head keeps the queue in order`() { + val queue = listOf("a", "b", "c") + + assertEquals(queue, BrowsedStart.playlist(chosen = "a", queue = queue)) + assertEquals(0, BrowsedStart.indexOf(chosen = "a", playlist = queue)) + } + + /** + * An episode browsed from a show is not in the queue, and must not drag the queue in behind + * it -- that would silently reorder what plays next. + */ + @Test + fun `an episode outside the queue plays alone`() { + assertEquals(listOf("z"), BrowsedStart.playlist(chosen = "z", queue = listOf("a", "b"))) + assertEquals(0, BrowsedStart.indexOf(chosen = "z", playlist = listOf("z"))) + } + + @Test + fun `an empty queue plays the chosen episode alone`() { + assertEquals(listOf("a"), BrowsedStart.playlist(chosen = "a", queue = emptyList())) + } } diff --git a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt index 2023833..3d0e23f 100644 --- a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt @@ -330,131 +330,82 @@ class PlaybackAdvancerTest { assertEquals(SubscriptionKind.PODCAST, (advancer.start("a") as? StartOutcome.NeedsFetch)?.kind) } - /* - Starting from outside the app, where there is nobody to show a "fetching" label to. - - Android Auto, a Bluetooth button and system resumption all reach the session callback, which - resolves a browse id straight to a media item. It never consulted the advancer about whether - the audio existed, so an undownloaded episode was handed to the player as its publisher's URL - and streamed -- in the car, which is where most of this library is actually listened to, and - the one place the wrong-copy skips would be least obvious. - - The in-app paths show a label and fetch in two steps. A car cannot, so this composes the two. - */ @Test - fun `starting from outside the app fetches the audio first`() = runTest { - show() - episode("a") - val fetching = PlaybackAdvancer( - episodes, subscriptions, queue, - sources = FakeSources(onFetch = { target -> - episodes.updateDownload(target.id, DownloadState.DOWNLOADED, "/audio/a.mp3", now) - Result.success(Unit) - }), - ) { now } - - val outcome = fetching.startFetchingIfNeeded("a") + fun `an episode whose show is gone and whose file is gone is not playable`() = runTest { + episode("a", subscriptionId = "vanished") - assertEquals("/audio/a.mp3", (outcome as? StartOutcome.Ready)?.next?.episode?.localPath) + assertTrue(advancer.start("a") is StartOutcome.Gone) } - /** Already downloaded: no fetch, because the common case must not pay for the rare one. */ @Test - fun `starting from outside the app does not refetch what is already there`() = runTest { - show() - episode("a", localPath = "/audio/a.mp3") - var fetches = 0 - val fetching = PlaybackAdvancer( - episodes, subscriptions, queue, - sources = FakeSources(onFetch = { fetches++; Result.success(Unit) }), - ) { now } + fun `an episode whose show is gone still plays from the file it has`() = runTest { + episode("a", subscriptionId = "vanished", localPath = "/audio/a.mp3") - assertTrue(fetching.startFetchingIfNeeded("a") is StartOutcome.Ready) - assertEquals(0, fetches) + assertTrue(advancer.start("a") is StartOutcome.Ready) } /** - * A fetch that fails reports it rather than falling back to the publisher. + * A fetch that throws is a failure, not a crash. * - * Playing the publisher's copy is what this whole change is removing: it carries different - * advertising, so the breaks measured on the server's copy land in the wrong place. Silence - * the listener can retry beats skips that quietly cut the programme. + * fetchAndStart folds a Result, so anything the fetch *throws* went straight past it -- and + * a feed can carry an enclosure URL that OkHttp refuses to parse. One bad enclosure in one + * feed would otherwise stop episodes starting rather than failing its own. */ @Test - fun `a fetch that fails from outside the app does not fall back to the publisher`() = runTest { + fun `a fetch that throws is reported rather than escaping`() = runTest { show() - episode("a") - val fetching = PlaybackAdvancer( + val ep = episode("a") + val throwing = PlaybackAdvancer( episodes, subscriptions, queue, - sources = FakeSources(onFetch = { Result.failure(IllegalStateException("waiting for the server")) }), + sources = FakeSources(onFetch = { throw IllegalArgumentException("not a url") }), ) { now } - assertTrue(fetching.startFetchingIfNeeded("a") is StartOutcome.FetchFailed) + assertTrue(throwing.fetchAndStart(ep, SubscriptionKind.PODCAST) is StartOutcome.FetchFailed) } /** - * A fetch that throws is a failure, not a crash. + * A cancelled start is not a failed one. * - * fetchAndStart folds a Result, so anything the fetch *throws* went straight past it. Every - * play goes through here now, so one feed with a malformed enclosure would stop episodes - * starting at all rather than failing the one that is broken. + * The catch that stops a thrown fetch escaping is deliberately broad, and would otherwise + * swallow cancellation -- so navigating away mid-fetch would report a failure and leave a + * false error on screen. Structured concurrency relies on it propagating too. */ /** - * An orphaned episode with no file has nothing to play and no way to get one. + * A failure with nothing to say still says something. * - * `start` reported Ready when the show was gone, because an unsubscribed show's episode - * still plays from a file it already has. With no file there is no kind to fetch with - * either, and the old answer was to stream the publisher. Nothing is the honest one, and it - * is what lets toMediaItem stop carrying a remote fallback at all. + * `FetchFailed` carries the reason because it is the only place the real cause surfaces -- + * "Sign in to confirm you're not a bot" and "video unavailable" are very different problems + * and look identical without it. Plenty of exceptions carry no message at all, and the + * fallback to the class name had never been exercised. */ @Test - fun `an episode whose show is gone and whose file is gone is not playable`() = runTest { - episode("a", subscriptionId = "vanished") - - assertTrue(advancer.start("a") is StartOutcome.Gone) - } + fun `a fetch failing with no message still reports a reason`() = runTest { + show() + val ep = episode("a") + val nameless = PlaybackAdvancer( + episodes, subscriptions, queue, + sources = FakeSources(onFetch = { Result.failure(IllegalStateException()) }), + ) { now } - @Test - fun `an episode whose show is gone still plays from the file it has`() = runTest { - episode("a", subscriptionId = "vanished", localPath = "/audio/a.mp3") + val outcome = nameless.fetchAndStart(ep, SubscriptionKind.PODCAST) - assertTrue(advancer.start("a") is StartOutcome.Ready) + assertEquals("IllegalStateException", (outcome as? StartOutcome.FetchFailed)?.reason) } - /** - * A cancelled start is not a failed one. - * - * The catch that stops a thrown fetch escaping is deliberately broad, which would otherwise - * swallow cancellation too -- so navigating away mid-fetch would report "could not fetch - * audio" and leave a false error on screen. Structured concurrency also relies on - * CancellationException propagating. - */ @Test fun `a cancelled fetch stays cancelled rather than becoming a failure`() = runTest { show() - episode("a") + val ep = episode("a") val cancelling = PlaybackAdvancer( episodes, subscriptions, queue, sources = FakeSources(onFetch = { throw CancellationException("navigated away") }), ) { now } - val thrown = runCatching { cancelling.startFetchingIfNeeded("a") }.exceptionOrNull() + val thrown = runCatching { cancelling.fetchAndStart(ep, SubscriptionKind.PODCAST) }.exceptionOrNull() assertTrue("got $thrown", thrown is CancellationException) } - @Test - fun `a fetch that throws is reported rather than escaping`() = runTest { - show() - episode("a") - val throwing = PlaybackAdvancer( - episodes, subscriptions, queue, - sources = FakeSources(onFetch = { throw IllegalArgumentException("not a url") }), - ) { now } - - assertTrue(throwing.startFetchingIfNeeded("a") is StartOutcome.FetchFailed) - } - @Test fun `fetching then starting plays from the file yt-dlp wrote`() = runTest { show(kind = SubscriptionKind.YOUTUBE_CHANNEL) -- cgit v1.2.3