diff options
Diffstat (limited to 'app')
15 files changed, 340 insertions, 197 deletions
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<String>): List<String> = + 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<String>): 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<MediaItem> = 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<MutableList<MediaItem>> = 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,132 +330,83 @@ 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) val episode = episode("a") |