diff options
29 files changed, 716 insertions, 204 deletions
diff --git a/.coverage-baseline b/.coverage-baseline index e04ecf2..f7407d8 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -94.58 +94.12 @@ -168,14 +168,17 @@ clip files on disk, and unsubscribing is the only thing that clears them. columns the on-device detector used to, which is why playback needed no changes when detection moved. No migration, no schema change, nothing for a real library to go wrong over. -Two things the deletion left standing that a future session should look at rather than assume: - -- **The per-show detection readout has nothing feeding it.** `ShowSettingsViewModel.loadDetection` - reads `ShowFingerprintDao.get` and `MidrollDao.getCandidates`, and nothing has written a - *candidate* row since detection moved: the server writes per-episode ranges and offsets, not the - confirmation state the readout describes. So "2 of 3 episodes agree" can never appear, and a - phone that never ran the old version shows "nothing found yet" for every show while the server - is finding and publishing breaks. Predates this deletion; it is the same migration's doing. +Two things the deletion left standing. The first is fixed; the second is not: + +- **The per-show detection readout had nothing feeding it — fixed 2026-11-05.** It read + `ShowFingerprintDao.get` and `MidrollDao.getCandidates`, the confirmation state the *on-device* + detector kept, and nothing had written a candidate row since detection moved. So every show + reported "Nothing found yet" for ever, including shows the server was actively publishing breaks + for — seen on Peter's phone. It is counted off the episodes now (`detectedIntroMs`, + `detectedOutroMs`, and the midroll ranges `ServerSkips` writes), worded by `SkipSummary.showState`, + and it keeps the distinction that was the point: *not checked* and *checked, nothing found* are + different answers. `show_midrolls` is consequently an entity nothing reads or writes; the table + stays because dropping it needs a migration and nothing is gained by one. - **`RetentionSweeper`'s pending-episode guard protects nothing.** It reads `getAllPendingEpisodeIds` and `getAllMidrollPendingEpisodeIds` so retention cannot reclaim a file mid-detection. Nothing sets a pending basis any more, so both are always empty — harmless, but a @@ -553,6 +556,38 @@ by default and correct; it is simply waiting for a publisher who marks breaks. ## Things a future session should know +- **Nothing the listener reads is written for a machine, and the build enforces it** + (`PlainLanguageTest`, added 2026-11-05). A red banner on Peter's phone read "Playback failed: + ERROR_CODE_IO_BAD_HTTP_STATUS"; his words were "arcane and sketchy" and both halves were fair. + The same pass found a downloader printing `${it.message}`, an OPML import printing + `permission denied`, and `FeedFetcher` putting the literal word `UnknownHostException` under a + podcast's name in the library — **and two tests that asserted the machine text came through**, + which is how it survived review. Causes go to logcat; the screen gets a sentence, from + `PlaybackFailureText`, `FeedFailureText` or `FailureText`. Accuracy is the harder half of the + rule: "it will be retried" would read better than "try the download button again" and would be + a lie, because a failed download is deliberately not retried. + +- **Every outbound request goes through one client, and the build enforces that too** + (`OutboundRequestsTest`). One `OkHttpClient`, built in `AppGraph`, carrying + `SecureSchemeInterceptor`; everything else is handed it, Coil's artwork fetches included. The + same arrangement the server has in `safefetch/audit_test.go`, for the same reason: this app + fetches URLs chosen by podcast publishers. + +- **Playback plays the app's own copy, never the publisher's** (changed 2026-11-05). + `toMediaItem` falls back to the enclosure URL when there is no local file, and that fallback was + reachable from every play: pressing play on an undownloaded episode streamed it straight from + the publisher's CDN. That is the automatic fallback the download path had removed on purpose; it + is an unasked-for request to a third party; and because nothing in playback checks + `audioFromServer`, the breaks the server measured on *its* copy were being applied to a copy with + different advertising in different places. `PlaybackAdvancer.start` now returns `NeedsFetch` for + any episode with no file, so the app fetches first. + + **One caller still reaches the fallback: Android Auto.** `onAddMediaItems` resolves a browse id + straight to a media item without going through the advancer, and a car that silently plays + nothing is worse than a car that streams. Fixing it properly means fetching inside the session + callback. `OutboundRequestsTest` pins the fallback to exactly one line so it cannot spread back. + + - **The episode screen is the player; there is no separate player screen** (merged 2026-10-19). Opening an episode puts you in its transport at the stored position, and pressing play changes nothing about the layout. Playback state still has one owner, `PlayerViewModel`; the screen diff --git a/app/src/main/java/org/terst/listen/data/Daos.kt b/app/src/main/java/org/terst/listen/data/Daos.kt index da31a88..e03a866 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -437,8 +437,6 @@ interface ShowFingerprintDao { */ @Dao interface MidrollDao { - @Query("SELECT * FROM show_midrolls WHERE subscriptionId = :subscriptionId") - suspend fun getCandidates(subscriptionId: String): List<ShowMidrollEntity> // Ordered by start: MidrollSkip.skipTarget filters by kind before firstOrNull, so an diff --git a/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt b/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt new file mode 100644 index 0000000..becbffe --- /dev/null +++ b/app/src/main/java/org/terst/listen/feed/FeedFailureText.kt @@ -0,0 +1,44 @@ +package org.terst.listen.feed + +/** + * Why a feed could not be refreshed, in words the listener can act on. + * + * These are read in two places -- the banner after trying to subscribe, and the error line under + * a show in the library -- and until now they were whatever the machine said. A dead host put + * the literal word `UnknownHostException` under a podcast's name; a moved feed said + * `HTTP 404 Not Found`. + * + * Accuracy is the constraint, not just plainness. A 404 and a 503 look the same to a library + * ("it didn't work") and are not the same thing to the person deciding whether to unsubscribe, + * so they say different things. Where the cause genuinely cannot be narrowed, the sentence + * stays vague rather than guessing. The exception goes to logcat. + */ +object FeedFailureText { + + private const val UNAUTHORISED = 401 + private const val FORBIDDEN = 403 + private const val NOT_FOUND = 404 + private const val GONE = 410 + private const val SERVER_ERROR = 500 + private const val LAST_STATUS = 599 + + /** A response that arrived and said no. */ + fun status(code: Int): String = when (code) { + NOT_FOUND, GONE -> "That feed is no longer at this address." + UNAUTHORISED, FORBIDDEN -> "That feed refused the request." + in SERVER_ERROR..LAST_STATUS -> "That feed's server is having trouble. It is worth trying later." + else -> "That feed's server could not provide it." + } + + /** The request never completed: no DNS, no route, a dropped connection, a timeout. */ + const val UNREACHABLE = "Could not reach that feed." + + /** The address itself is not usable, which is the one case the listener can fix directly. */ + const val NOT_AN_ADDRESS = "That does not look like a feed address." + + /** Something arrived, and it was not a feed. */ + const val UNREADABLE = "That feed could not be read." + + /** It parsed, but as something with no show in it -- a parked domain, a login wall. */ + const val NOT_A_FEED = "That address is not a podcast feed." +} diff --git a/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt b/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt index 671953a..5790c63 100644 --- a/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt +++ b/app/src/main/java/org/terst/listen/feed/FeedFetcher.kt @@ -2,6 +2,7 @@ package org.terst.listen.feed import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext +import android.util.Log import okhttp3.OkHttpClient import okhttp3.Request import java.io.IOException @@ -38,20 +39,23 @@ class FeedFetcher(private val client: OkHttpClient) { when { response.code == NOT_MODIFIED -> FeedFetchResult.NotModified !response.isSuccessful -> - FeedFetchResult.Failed("HTTP ${response.code} ${response.message}".trim()) + FeedFetchResult.Failed(FeedFailureText.status(response.code)) else -> parse(response.body?.byteStream(), response.header("ETag"), response.header("Last-Modified")) } } } catch (e: IOException) { - FeedFetchResult.Failed(e.message ?: e.javaClass.simpleName) + // The cause is for the log, not the library screen; see FeedFailureText. + Log.w(TAG, "could not reach $url", e) + FeedFetchResult.Failed(FeedFailureText.UNREACHABLE) } catch (e: IllegalArgumentException) { - FeedFetchResult.Failed("Not a usable feed URL: ${e.message}") + Log.w(TAG, "not a usable feed address: $url", e) + FeedFetchResult.Failed(FeedFailureText.NOT_AN_ADDRESS) } } private fun parse(stream: java.io.InputStream?, etag: String?, lastModified: String?): FeedFetchResult { - if (stream == null) return FeedFetchResult.Failed("Empty response") + if (stream == null) return FeedFetchResult.Failed(FeedFailureText.UNREADABLE) // Deliberately broad. This parses arbitrary bytes from an arbitrary server, and a // refresh sweeps every subscription in one pass -- one publisher serving something // malformed must degrade to a recorded error on that feed, not take down the batch. @@ -59,13 +63,14 @@ class FeedFetcher(private val client: OkHttpClient) { val feed = try { PodcastFeedParser.parse(stream) } catch (e: Exception) { - return FeedFetchResult.Failed("Could not parse feed: ${e.message ?: e.javaClass.simpleName}") + Log.w(TAG, "could not parse feed", e) + return FeedFetchResult.Failed(FeedFailureText.UNREADABLE) } // A 200 carrying a parked domain or a login wall parses as XML with nothing in it. A real // show that has not published yet still has a title, so requiring one of the two // distinguishes "no episodes yet" from "this is not a feed". if (feed.title == null && feed.episodes.isEmpty()) { - return FeedFetchResult.Failed("Response was not a podcast feed") + return FeedFetchResult.Failed(FeedFailureText.NOT_A_FEED) } return FeedFetchResult.Fetched(feed, etag, lastModified) } @@ -81,6 +86,7 @@ class FeedFetcher(private val client: OkHttpClient) { * proof-of-origin token or a borrowed login, which Listen still doesn't do. Listen/1.0 stays on * the end so the traffic is still attributable in a server log. */ + const val TAG = "ListenFeed" const val USER_AGENT = "Mozilla/5.0 (Linux; Android 14) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Mobile Safari/537.36 Listen/1.0" } diff --git a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt index 2f0545d..2d4bc49 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt @@ -4,8 +4,6 @@ import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity -import org.terst.listen.data.ShowFingerprintEntity -import org.terst.listen.data.ShowMidrollEntity /** * What the episode screen says about skipping, as three numbers and at most one status. @@ -145,32 +143,28 @@ object SkipSummary { */ fun isAdvertising(kind: String): Boolean = kind !in setOf("intro", "cue", "outro") - /** One end of a show's episodes -- its intro or its outro. */ - fun candidateState(state: ShowFingerprintEntity?, threshold: Int): String { - // No reference clip means no candidate: either nothing has been processed, or a single - // episode's clip is set aside with nothing yet to compare it against. Both are the same - // thing to a reader -- detection has not found anything. - if (state?.referenceClipPcm == null) return "Nothing found yet" - - return if (state.confirmedCount >= threshold) { - "Confirmed by ${state.confirmedCount} episodes" - } else { - "Found, ${state.confirmedCount} of $threshold episodes agree" - } + /** + * What the server has worked out for one show, across the episodes it has looked at. + * + * Replaces a pair of readouts that could not say anything. They read `show_fingerprints` and + * `show_midrolls`, the confirmation state the on-device detector kept, and nothing has + * written a row of either since detection moved to the server -- so every show reported + * "Nothing found yet" for ever, including shows breaks were actively being published for. + * + * The distinction that matters is still the one it always was: looked-and-found-nothing is a + * different answer from not-looked-yet, and a reader who cannot tell them apart has no idea + * whether to wait. [checked] is how many of the show's episodes the server has examined and + * [found] how many of those carry this kind of skip. + */ + fun showState(checked: Int, found: Int): String = when { + checked <= 0 -> "Not checked yet" + found <= 0 -> "None found in $checked ${episodes(checked)}" + found >= checked && checked == 1 -> "Found in the 1 episode checked" + found >= checked -> "Found in all $checked episodes" + else -> "Found in $found of $checked episodes" } - /** A show's midroll candidates, which unlike intro/outro can be several at once. */ - fun midrollState(candidates: List<ShowMidrollEntity>, threshold: Int): String { - if (candidates.isEmpty()) return "None found yet" - - val confirmed = candidates.count { it.confirmedCount >= threshold } - val gathering = candidates.size - confirmed - - return listOfNotNull( - "$confirmed confirmed".takeIf { confirmed > 0 }, - "$gathering still gathering".takeIf { gathering > 0 }, - ).joinToString(", ") - } + private fun episodes(n: Int) = if (n == 1) "episode" else "episodes" /** * Everything the episode screen says about skipping, decided here rather than in the 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 9007c51..f40adef 100644 --- a/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt +++ b/app/src/main/java/org/terst/listen/playback/PlaybackAdvancer.kt @@ -179,9 +179,20 @@ class PlaybackAdvancer( val episode = episodes.getById(episodeId) ?: return StartOutcome.Gone val show = subscriptions.getById(episode.subscriptionId) - // Nothing to play until yt-dlp has written a file. The download is the caller's job. + // Nothing plays until there is a file. The fetch is the caller's job. + // + // For YouTube and Facebook that is obvious -- their media URL is a watch page and yt-dlp + // has to write the audio first. For an ordinary podcast it is a correctness rule, and it + // was missing: `toMediaItem` falls back to `setUri(mediaUrl)`, so pressing play on an + // undownloaded episode opened the publisher's URL and streamed it. + // + // Three things wrong with that. It is the automatic publisher fallback the download path + // had removed on purpose. It is an outbound request to a third party that nothing asked + // for. And nothing in playback checks `audioFromServer`, so the breaks the server + // measured on its own copy were being applied to a copy with different advertising in + // different places -- skips landing in the wrong spot, with no way to tell why. val kind = show?.kind - if (kind?.needsExtraction == true && episode.localPath == null) { + if (episode.localPath == null && kind != null) { return StartOutcome.NeedsFetch(episode, kind) } diff --git a/app/src/main/java/org/terst/listen/playback/PlaybackFailureText.kt b/app/src/main/java/org/terst/listen/playback/PlaybackFailureText.kt new file mode 100644 index 0000000..f23a043 --- /dev/null +++ b/app/src/main/java/org/terst/listen/playback/PlaybackFailureText.kt @@ -0,0 +1,45 @@ +package org.terst.listen.playback + +/** + * What the listener is told when playback stops, in words rather than in codes. + * + * The banner used to print Media3's own constant: "Playback failed: ERROR_CODE_IO_BAD_HTTP_STATUS". + * That is arcane -- it names an implementation detail of a library the listener does not use -- + * and it reads as sketchy, because it announces an HTTP request without saying to whom or why. + * + * Both complaints were fair, and the second was pointing at a real bug rather than a wording + * problem: the player was streaming episodes straight from their publishers, so there genuinely + * was an unexplained request. See [PlaybackAdvancer.start]. + * + * Each line says what happened and, where there is one, what to do about it. Nothing here + * guesses: an unmapped code gets the sentence that is true of all of them. The raw name is + * logged, which is where a code belongs. + */ +object PlaybackFailureText { + + /** The sentence for one Media3 `errorCodeName`. */ + fun of(errorCodeName: String): String = when (errorCodeName) { + "ERROR_CODE_IO_BAD_HTTP_STATUS" -> "The server refused to send this episode's audio." + "ERROR_CODE_IO_FILE_NOT_FOUND" -> "This episode's download is missing. Download it again." + "ERROR_CODE_IO_NETWORK_CONNECTION_FAILED", + "ERROR_CODE_IO_NETWORK_CONNECTION_TIMEOUT", + -> "No connection, so this episode could not be played." + "ERROR_CODE_IO_NO_PERMISSION" -> "Listen was not allowed to open this episode's audio." + "ERROR_CODE_DECODING_FAILED", + "ERROR_CODE_PARSING_CONTAINER_MALFORMED", + "ERROR_CODE_PARSING_MANIFEST_MALFORMED", + -> "This episode's audio could not be played; the file may be damaged." + // Should no longer be reachable: MediaUrl upgrades a cleartext enclosure, and an episode + // is fetched rather than streamed. Kept because "no longer reachable" has been wrong before. + "ERROR_CODE_IO_CLEARTEXT_NOT_PERMITTED" -> "This episode is only offered over an insecure connection." + else -> UNKNOWN + } + + /** + * Everything true of a failure nobody mapped. + * + * Deliberately not "Playback failed: <code>". A sentence the listener can read beats a + * constant they cannot, and inventing a cause would be worse than either. + */ + const val UNKNOWN = "This episode stopped playing." +} 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 d994092..95229f9 100644 --- a/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt +++ b/app/src/main/java/org/terst/listen/playback/PlayerConnection.kt @@ -2,6 +2,7 @@ package org.terst.listen.playback import android.content.ComponentName import android.content.Context +import android.util.Log import androidx.media3.common.MediaItem import androidx.media3.common.Player import androidx.media3.session.MediaController @@ -122,6 +123,8 @@ class PlayerConnection(private val context: Context) : PlaybackControls { override fun onPlaybackStateChanged(playbackState: Int) = publish() override fun onPlayerError(error: androidx.media3.common.PlaybackException) { + // The code goes to the log, where a code is useful; the listener gets a sentence. + Log.w("ListenPlayback", "playback failed: ${error.errorCodeName}", error) onError?.invoke(error.errorCodeName) publish() } diff --git a/app/src/main/java/org/terst/listen/ui/FailureText.kt b/app/src/main/java/org/terst/listen/ui/FailureText.kt new file mode 100644 index 0000000..2fada1e --- /dev/null +++ b/app/src/main/java/org/terst/listen/ui/FailureText.kt @@ -0,0 +1,30 @@ +package org.terst.listen.ui + +/** + * What the listener is told when something the app was asked to do did not work. + * + * Every one of these used to end in `${it.message}` -- the exception's own text, printed to the + * screen. That is the same fault as "Playback failed: ERROR_CODE_IO_BAD_HTTP_STATUS": it is + * written for whoever wrote the library, it reads as something having gone wrong at a level the + * listener cannot reach, and on a bad day it puts a URL or a host name in front of them. + * + * What replaces it has to be *accurate* as well as plain, which is the harder half. "It will be + * retried" would be clearer and would also be a lie: a failed download is left FAILED on purpose + * and retry is the download button (see DownloadDrainer). So these say what is true and what the + * listener can actually do, and nothing else. The exception goes to logcat. + * + * [org.terst.listen.playback.PlaybackFailureText] is the same idea for playback, where Media3 + * gives a code worth mapping. Here there is nothing worth mapping: an IOException from a feed's + * CDN says nothing a listener can act on differently. + */ +object FailureText { + + /** A download the app tried and could not finish. Retry is deliberately manual. */ + const val DOWNLOAD = "Could not download that episode. Try the download button again." + + /** Reading a file the listener picked. The file is the thing they can change. */ + const val OPML_READ = "Could not read that file. It needs to be an OPML subscription list." + + /** Writing one out, where the place they chose is the thing they can change. */ + const val OPML_WRITE = "Could not save the file. Try somewhere else, or check there is room." +} diff --git a/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt b/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt index 0fe8132..b8df247 100644 --- a/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt @@ -2,6 +2,7 @@ package org.terst.listen.ui import org.terst.listen.download.WaitingForServerException import android.app.Application +import android.util.Log import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -242,7 +243,8 @@ class ListenViewModel(app: Application, private val graph: UiGraph) : AndroidVie _banner.value = if (it is WaitingForServerException) { Banner(WaitingForServerText.BANNER) } else { - Banner("Download failed: ${it.message}", isError = true) + Log.w("ListenDownload", "download failed", it) + Banner(FailureText.DOWNLOAD, isError = true) } } } @@ -255,7 +257,8 @@ class ListenViewModel(app: Application, private val graph: UiGraph) : AndroidVie fun downloadFromPublisher(episode: EpisodeEntity, onDone: () -> Unit = {}) { viewModelScope.launch { graph.downloader.downloadFromPublisher(episode).onFailure { - _banner.value = Banner("Download failed: ${it.message}", isError = true) + Log.w("ListenDownload", "publisher download failed", it) + _banner.value = Banner(FailureText.DOWNLOAD, isError = true) } onDone() } diff --git a/app/src/main/java/org/terst/listen/ui/OpmlViewModel.kt b/app/src/main/java/org/terst/listen/ui/OpmlViewModel.kt index 8443561..53d7f3c 100644 --- a/app/src/main/java/org/terst/listen/ui/OpmlViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/OpmlViewModel.kt @@ -1,6 +1,7 @@ package org.terst.listen.ui import android.app.Application +import android.util.Log import android.net.Uri import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope @@ -54,7 +55,8 @@ class OpmlViewModel(app: Application, private val graph: UiGraph) : AndroidViewM val feeds = runCatching { open()?.use { Opml.parse(it) }.orEmpty() }.getOrElse { - _banner.value = Banner("Could not read that file: ${it.message}", isError = true) + Log.w("ListenOpml", "import failed", it) + _banner.value = Banner(FailureText.OPML_READ, isError = true) return@launch } @@ -92,7 +94,10 @@ class OpmlViewModel(app: Application, private val graph: UiGraph) : AndroidViewM open()?.use { Opml.write(feeds, it) } }.fold( onSuccess = { _banner.value = Banner("Exported ${feeds.size} subscription(s)") }, - onFailure = { _banner.value = Banner("Export failed: ${it.message}", isError = true) }, + onFailure = { + Log.w("ListenOpml", "export failed", it) + _banner.value = Banner(FailureText.OPML_WRITE, isError = true) + }, ) } } 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 7ff96b4..1c42c13 100644 --- a/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/PlayerViewModel.kt @@ -12,6 +12,7 @@ import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.Graph import org.terst.listen.feed.Chapter import org.terst.listen.playback.NowPlaying +import org.terst.listen.playback.PlaybackFailureText import org.terst.listen.playback.PlaybackSpeed import org.terst.listen.playback.SleepTimer import org.terst.listen.data.UiGraph @@ -53,7 +54,8 @@ class PlayerViewModel( init { connection.connect() - connection.onError = { code -> _banner.value = Banner("Playback failed: $code", isError = true) } + // Words, not Media3's constant; the code itself goes to logcat. See PlaybackFailureText. + connection.onError = { code -> _banner.value = Banner(PlaybackFailureText.of(code), isError = true) } // Position advances with no callback of its own, so the bar is driven by a tick. viewModelScope.launch { diff --git a/app/src/main/java/org/terst/listen/ui/ShowSettingsViewModel.kt b/app/src/main/java/org/terst/listen/ui/ShowSettingsViewModel.kt index 6ed7bf6..6bdf4c4 100644 --- a/app/src/main/java/org/terst/listen/ui/ShowSettingsViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/ShowSettingsViewModel.kt @@ -8,7 +8,6 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.launch import org.terst.listen.data.ArchiveOutcome -import org.terst.listen.data.FingerprintKind import org.terst.listen.data.Graph import org.terst.listen.data.ShowTier import org.terst.listen.data.UiGraph @@ -158,19 +157,17 @@ class ShowSettingsViewModel(app: Application, private val graph: UiGraph) : Andr */ fun loadDetection(subscriptionId: String) { viewModelScope.launch { + // Counted off the episodes, because that is where the server's answers land. + // `detectedIntroMs`, `detectedOutroMs` and the midroll ranges are what ServerSkips + // writes; the show-level confirmation rows this used to read have had no writer + // since detection moved, so every show read "Nothing found yet" whatever was true. + val episodes = graph.episodes.getForSubscription(subscriptionId) + val checked = episodes.count { it.audioFromServer } + val withRanges = episodes.count { it.audioFromServer && graph.midrolls.getRanges(it.id).isNotEmpty() } _detection.value = ShowDetection( - intro = SkipSummary.candidateState( - graph.fingerprints.get(subscriptionId, FingerprintKind.INTRO), - SkipSummary.CONFIRM_THRESHOLD, - ), - outro = SkipSummary.candidateState( - graph.fingerprints.get(subscriptionId, FingerprintKind.OUTRO), - SkipSummary.CONFIRM_THRESHOLD, - ), - midrolls = SkipSummary.midrollState( - graph.midrolls.getCandidates(subscriptionId), - SkipSummary.CONFIRM_THRESHOLD, - ), + intro = SkipSummary.showState(checked, episodes.count { it.audioFromServer && it.detectedIntroMs != null }), + outro = SkipSummary.showState(checked, episodes.count { it.audioFromServer && it.detectedOutroMs != null }), + midrolls = SkipSummary.showState(checked, withRanges), ) } } diff --git a/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt new file mode 100644 index 0000000..85f179b --- /dev/null +++ b/app/src/test/java/org/terst/listen/OutboundRequestsTest.kt @@ -0,0 +1,111 @@ +package org.terst.listen + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import java.io.File +import org.junit.Test + +/** + * Every request this app makes goes out through one client, and nothing else opens a connection. + * + * Peter asked for this after a banner announced an HTTP failure he had not asked for: "all + * communication is whitelisted and invoked only when appropriate". The cause turned out to be + * real -- the player was streaming episodes from their publishers whenever one was not + * downloaded -- but the general point stands on its own. An app that fetches arbitrary URLs + * chosen by podcast publishers should have one way out, not five. + * + * The server has the same check for the same reason (safefetch/audit_test.go), written after an + * SSRF showed up in the third place that fetched a URL somebody else chose. + */ +class OutboundRequestsTest { + + private val mainDir = File("src/main/java/org/terst/listen") + + private fun sources() = mainDir.walkTopDown().filter { it.extension == "kt" }.toList() + + private fun linesOf(file: File) = file.readLines() + .map { it.substringBefore("//") } + .filterNot { it.trimStart().startsWith("*") } + + /** + * The one place a client is built. Everything else is handed it, so the interceptors, + * timeouts and user agent apply to every request including Coil's artwork fetches. + */ + private val clientHome = "Repositories.kt" + + @Test + fun `only one file builds an http client`() { + val builders = sources().filter { file -> + linesOf(file).any { it.contains("OkHttpClient.Builder()") || it.contains("OkHttpClient(") } + } + + assertEquals( + "A second client would miss the interceptors the first one carries. Inject " + + "AppGraph.client instead. Found: ${builders.map { it.name }}", + listOf(clientHome), + builders.map { it.name }, + ) + } + + /** The client everything shares has to be the one that upgrades cleartext. */ + @Test + fun `the shared client sends everything over TLS`() { + val home = sources().single { it.name == clientHome } + + assertTrue( + "$clientHome builds the app's only client, so it is the only place that can " + + "guarantee TLS; see SecureSchemeInterceptor", + home.readText().contains("SecureSchemeInterceptor"), + ) + } + + /** + * A connection opened any other way carries none of that: no TLS upgrade, no user agent, no + * timeout, and nothing in this test to notice it. + */ + @Test + fun `nothing opens a connection behind the client's back`() { + val banned = listOf("HttpURLConnection", "openConnection(", "URLConnection", "Socket(") + val offenders = mutableListOf<String>() + for (file in sources()) { + linesOf(file).forEachIndexed { index, line -> + banned.filter { line.contains(it) }.forEach { + offenders += "${file.name}:${index + 1} uses $it" + } + } + } + assertTrue("Route it through AppGraph.client:\n${offenders.joinToString("\n")}", offenders.isEmpty()) + } + + /** + * The app plays its own copy of an episode, not the publisher's. + * + * `toMediaItem` falls back to the enclosure URL when there is no file, and that fallback was + * how an unasked-for request reached a publisher's CDN -- and how the server's measured + * breaks came to be applied to a copy with different advertising in it. `PlaybackAdvancer` + * now fetches first, so the app's own play paths never reach the fallback. + * + * It survives for one caller: Android Auto hands back a browse id, `onAddMediaItems` + * resolves it straight to a media item, and a car that silently plays nothing is worse than + * a car that streams. Fixing that properly means fetching inside the session callback. + * + * Until then this pins it to one line, so it cannot spread back into the paths it was just + * taken out of. + */ + @Test + fun `the publisher stream fallback exists in exactly one place`() { + val fallbacks = sources().flatMap { file -> + linesOf(file).mapIndexedNotNull { index, line -> + "${file.name}:${index + 1}".takeIf { line.contains("MediaUrl.secured(") } + } + } + + assertEquals( + "Streaming an episode from its publisher plays different audio from the one the " + + "skips were measured on. If this grew, something is bypassing PlaybackAdvancer:\n" + + fallbacks.joinToString("\n"), + 1, + fallbacks.size, + ) + } +} diff --git a/app/src/test/java/org/terst/listen/PlainLanguageTest.kt b/app/src/test/java/org/terst/listen/PlainLanguageTest.kt new file mode 100644 index 0000000..bb451c0 --- /dev/null +++ b/app/src/test/java/org/terst/listen/PlainLanguageTest.kt @@ -0,0 +1,91 @@ +package org.terst.listen + +import org.junit.Assert.assertTrue +import java.io.File +import org.junit.Test + +/** + * Nothing the listener reads is written for a machine. + * + * Added after a red banner on Peter's phone said "Playback failed: ERROR_CODE_IO_BAD_HTTP_STATUS". + * His words were "arcane and sketchy", and both halves were fair: it names an internal of a + * library he does not use, and it announces an HTTP request without saying to whom or why. + * + * That was not one slip. The same pass found a downloader printing `${it.message}`, an OPML + * import printing `permission denied`, and a feed fetcher putting the literal word + * `UnknownHostException` under a podcast's name in the library -- and two tests that *asserted* + * the machine text came through, which is how it survived review. + * + * So this is the ratchet. Diagnosis goes to logcat, where a code is useful; the screen gets a + * sentence. The scope is deliberately narrow and honest: it reads the places errors actually + * surface, not every string in the app, because a check that tried to judge all prose would + * either be noise or be wrong. + */ +class PlainLanguageTest { + + private val mainDir = File("src/main/java/org/terst/listen") + + /** Where a cause is turned into something the listener reads. */ + private val surfaces = Regex("""Banner\(|reportAndFinish\(|FeedFetchResult\.Failed\(""") + + /** + * Interpolating any of these into a listener-facing string means the machine's words reach + * the screen. `it.message` is the one that kept happening. + */ + private val machineValue = Regex( + """\$\{?[\w.]*\b(message|localizedMessage|stackTrace|errorCodeName|javaClass|simpleName)\b""", + ) + + /** A constant like ERROR_CODE_IO_BAD_HTTP_STATUS, written out where a reader would see it. */ + private val machineConstant = Regex("""\b[A-Z][A-Z0-9]{2,}_[A-Z0-9_]{2,}\b""") + + /** + * Files allowed to name those constants, because naming them is their job: each maps a code + * to a sentence, and the map has to say which code. + */ + private val mappers = setOf("PlaybackFailureText.kt", "FeedFailureText.kt", "FailureText.kt") + + private fun sources() = mainDir.walkTopDown().filter { it.extension == "kt" }.toList() + + @Test + fun `no listener-facing message is built out of an exception`() { + val offenders = mutableListOf<String>() + for (file in sources()) { + file.readLines().forEachIndexed { index, line -> + val code = line.substringBefore("//") + if (!surfaces.containsMatchIn(code)) return@forEachIndexed + if (machineValue.containsMatchIn(code)) { + offenders += "${file.name}:${index + 1} ${code.trim()}" + } + } + } + assertTrue( + "These put a machine's words in front of the listener. Log the cause and show a " + + "sentence -- see PlaybackFailureText, FeedFailureText, FailureText:\n" + + offenders.joinToString("\n"), + offenders.isEmpty(), + ) + } + + @Test + fun `no listener-facing message spells out an error constant`() { + val offenders = mutableListOf<String>() + for (file in sources()) { + if (file.name in mappers) continue + file.readLines().forEachIndexed { index, line -> + val code = line.substringBefore("//") + if (!surfaces.containsMatchIn(code)) return@forEachIndexed + Regex(""""([^"]*)"""").findAll(code).forEach { literal -> + if (machineConstant.containsMatchIn(literal.groupValues[1])) { + offenders += "${file.name}:${index + 1} ${literal.value}" + } + } + } + } + assertTrue( + "A constant is not a sentence. Map it to one in a *FailureText object:\n" + + offenders.joinToString("\n"), + offenders.isEmpty(), + ) + } +} diff --git a/app/src/test/java/org/terst/listen/data/FakeDaos.kt b/app/src/test/java/org/terst/listen/data/FakeDaos.kt index fc2c558..ffb440a 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -349,9 +349,6 @@ class FakeMidrollDao : MidrollDao { /** Backs [getAllMidrollPendingEpisodeIds], which reads the real `subscriptions` table. */ var subscriptionRows: MutableList<SubscriptionEntity> = mutableListOf() - override suspend fun getCandidates(subscriptionId: String) = - candidateRows.filter { it.subscriptionId == subscriptionId } - override suspend fun getRanges(episodeId: String) = rangeRows.filter { it.episodeId == episodeId } diff --git a/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt b/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt index 1c8cda1..a6df6ff 100644 --- a/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt +++ b/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt @@ -11,6 +11,7 @@ import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test +import org.terst.listen.feed.FeedFailureText import org.terst.listen.feed.FeedFetcher /** @@ -110,7 +111,9 @@ class SubscriptionRepositoryTest { assertTrue(outcome is RefreshOutcome.Failed) val stored = subs.rows.single() assertNotNull("the reason must be stored, not just logged", stored.lastError) - assertTrue(stored.lastError!!.contains("500")) + // The reason is shown under the show's name in the library, so it is a sentence rather + // than a status line. What matters here is that something usable was stored. + assertEquals(FeedFailureText.status(500), stored.lastError) } @Test diff --git a/app/src/test/java/org/terst/listen/feed/FeedArchiveWalkerTest.kt b/app/src/test/java/org/terst/listen/feed/FeedArchiveWalkerTest.kt index b37e07c..7c3885a 100644 --- a/app/src/test/java/org/terst/listen/feed/FeedArchiveWalkerTest.kt +++ b/app/src/test/java/org/terst/listen/feed/FeedArchiveWalkerTest.kt @@ -119,6 +119,6 @@ class FeedArchiveWalkerTest { assertEquals(1, walk.pages) assertEquals(listOf("p2a"), walk.episodes.map { it.guid }) - assertTrue(walk.stoppedEarly.orEmpty(), walk.stoppedEarly.orEmpty().contains("500")) + assertEquals(FeedFailureText.status(500), walk.stoppedEarly) } } diff --git a/app/src/test/java/org/terst/listen/feed/FeedFetcherTest.kt b/app/src/test/java/org/terst/listen/feed/FeedFetcherTest.kt index 75b58d7..2d792e4 100644 --- a/app/src/test/java/org/terst/listen/feed/FeedFetcherTest.kt +++ b/app/src/test/java/org/terst/listen/feed/FeedFetcherTest.kt @@ -6,6 +6,7 @@ import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before @@ -84,13 +85,26 @@ class FeedFetcherTest { assertNull(request.getHeader("If-Modified-Since")) } - /** A failure has to be reportable, so it can be recorded against the subscription. */ + /** + * A failure has to be reportable, because it is recorded against the subscription and shown + * under the show's name in the library. It used to report "HTTP 500 Internal Server Error"; + * the status belongs in the log, and what the reader needs is whether to try again. + */ @Test - fun `a server error is a failure carrying its status`() = runTest { + fun `a server error says it is worth trying later, not what the status was`() = runTest { server.enqueue(MockResponse().setResponseCode(500)) val result = fetcher.fetch(server.url("/f").toString(), null, null) val failed = result as FeedFetchResult.Failed - assertTrue("message should name the status: ${failed.message}", failed.message.contains("500")) + assertEquals(FeedFailureText.status(500), failed.message) + assertFalse("a status code is not for the listener: ${failed.message}", failed.message.contains("500")) + } + + /** A feed that has moved is a different decision from one having a bad day, and says so. */ + @Test + fun `a missing feed says it has gone rather than to try later`() = runTest { + server.enqueue(MockResponse().setResponseCode(404)) + val failed = fetcher.fetch(server.url("/f").toString(), null, null) as FeedFetchResult.Failed + assertEquals("That feed is no longer at this address.", failed.message) } @Test diff --git a/app/src/test/java/org/terst/listen/fingerprint/ShowDetectionStateTest.kt b/app/src/test/java/org/terst/listen/fingerprint/ShowDetectionStateTest.kt new file mode 100644 index 0000000..6465893 --- /dev/null +++ b/app/src/test/java/org/terst/listen/fingerprint/ShowDetectionStateTest.kt @@ -0,0 +1,55 @@ +package org.terst.listen.fingerprint + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * What a show's settings sheet says the server has worked out. + * + * This replaces a readout that could not say anything. It read `show_fingerprints` and + * `show_midrolls` -- the confirmation state the *on-device* detector kept -- and nothing has + * written a row of either since detection moved to the server, which writes per-episode offsets + * and ranges instead. So every show reported "Nothing found yet" for ever, including shows the + * server was actively publishing breaks for. Confirmed on a phone before this was rewritten. + * + * The three states that matter are the ones CLAUDE.md names: looked-and-found, looked-and-found- + * nothing, and not-looked-yet. They are different answers and the old readout collapsed two of + * them. + */ +class ShowDetectionStateTest { + + @Test + fun `nothing checked yet is not the same as nothing found`() { + assertEquals("Not checked yet", SkipSummary.showState(checked = 0, found = 0)) + } + + @Test + fun `checked and found nothing says so, so it is not mistaken for still working`() { + assertEquals("None found in 8 episodes", SkipSummary.showState(checked = 8, found = 0)) + } + + @Test + fun `found says how widely, because one episode out of eight is a different thing from eight`() { + assertEquals("Found in 6 of 8 episodes", SkipSummary.showState(checked = 8, found = 6)) + assertEquals("Found in 1 of 8 episodes", SkipSummary.showState(checked = 8, found = 1)) + } + + /** Every episode carrying it is the ordinary healthy case, and reads better without a ratio. */ + @Test + fun `found everywhere drops the ratio`() { + assertEquals("Found in all 8 episodes", SkipSummary.showState(checked = 8, found = 8)) + assertEquals("Found in the 1 episode checked", SkipSummary.showState(checked = 1, found = 1)) + } + + /** Singular, because "None found in 1 episodes" is the kind of thing that erodes trust. */ + @Test + fun `one episode reads as one episode`() { + assertEquals("None found in 1 episode", SkipSummary.showState(checked = 1, found = 0)) + } + + /** Nonsense in must not produce confident nonsense out. */ + @Test + fun `found without checked cannot happen and does not pretend to`() { + assertEquals("Not checked yet", SkipSummary.showState(checked = 0, found = 3)) + } +} diff --git a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt index f1e6b8c..9794fda 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt @@ -8,7 +8,6 @@ import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity import org.terst.listen.data.FingerprintKind import org.terst.listen.data.ShowFingerprintEntity -import org.terst.listen.data.ShowMidrollEntity /** * The words the app uses to say what the skip detection has worked out. @@ -66,89 +65,6 @@ class SkipSummaryTest { audioFromServer = audioFromServer ?: false, ) - private fun candidate(id: String, confirmedCount: Int) = ShowMidrollEntity( - subscriptionId = "s1", - id = id, - referenceClipPath = "/clips/s1/$id.pcm", - sampleRate = 8000, - confirmedCount = confirmedCount, - lastConfirmedEpisodeId = null, - updatedAtMs = 0, - ) - - @Test - fun nothingSeenYet() { - assertEquals("Nothing found yet", SkipSummary.candidateState(null, threshold = 3)) - } - - /** One episode in, there is a clip set aside but nothing to compare it against. */ - @Test - fun waitingForASecondEpisode() { - assertEquals( - "Nothing found yet", - SkipSummary.candidateState(fingerprint(0, hasReference = false, pendingEpisodeId = "e1"), 3), - ) - } - - @Test - fun gatheringNamesHowFarOffItIs() { - assertEquals("Found, 2 of 3 episodes agree", SkipSummary.candidateState(fingerprint(2), 3)) - } - - /** The distinction that matters: below the threshold nothing is being skipped. */ - @Test - fun confirmedSaysSo() { - assertEquals("Confirmed by 3 episodes", SkipSummary.candidateState(fingerprint(3), 3)) - } - - @Test - fun confirmedKeepsCountingPastTheThreshold() { - assertEquals("Confirmed by 7 episodes", SkipSummary.candidateState(fingerprint(7), 3)) - } - - @Test - fun noMidrollCandidates() { - assertEquals("None found yet", SkipSummary.midrollState(emptyList(), threshold = 3)) - } - - @Test - fun midrollsCountBothStates() { - val candidates = listOf(candidate("a", 4), candidate("b", 3), candidate("c", 2)) - assertEquals("2 confirmed, 1 still gathering", SkipSummary.midrollState(candidates, 3)) - } - - @Test - fun midrollsGatheringOnly() { - assertEquals("1 still gathering", SkipSummary.midrollState(listOf(candidate("a", 2)), 3)) - } - - @Test - fun midrollsConfirmedOnly() { - assertEquals("1 confirmed", SkipSummary.midrollState(listOf(candidate("a", 3)), 3)) - } - - - /** - * The three numbers Peter asked for as a sanity check, on one line: what was served, what the - * feed declared, and how much the breaks cut. Served first because it is the one measured - * against the actual file. - */ - @Test - fun `shows served, declared and cut on one line`() { - val skips = SkipSummary.episode( - episodeRow(durationMs = 1_325_000, actualDurationMs = 1_774_000, midrollCheckedAtMs = 1), - listOf( - EpisodeMidrollEntity("e1", "c1", startMs = 120_000, endMs = 180_000), - EpisodeMidrollEntity("e1", "c2", startMs = 600_000, endMs = 690_000), - ), - ) - - assertEquals("29:34", skips.served) - assertEquals("22:05", skips.declared) - assertEquals("2:30", skips.cut) - assertEquals("Served 29:34 \u00b7 Declared 22:05 \u00b7 Cut 2:30", skips.numbers) - assertEquals(null, skips.status) - } /** Never played, so the file has not been measured: omitted, not shown as a zero or a dash. */ @Test 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 eba2629..04b31ff 100644 --- a/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt +++ b/app/src/test/java/org/terst/listen/playback/PlaybackAdvancerTest.kt @@ -4,6 +4,7 @@ import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.terst.listen.data.BreakVerdictEntity @@ -127,6 +128,43 @@ class PlaybackAdvancerTest { * Marking a break wrong has to stop it being skipped from the next tick, and the context is * read once per episode -- so the ticker asks for the verdicts afresh each time. */ + /* + An episode with no file is fetched, not streamed from the publisher. + + `toMediaItem` does `setUri(localPath ?: mediaUrl)`, so until now pressing play on an + undownloaded podcast opened the publisher's URL directly. Three things wrong with that, and + the first is the one that shows: + + - The publisher's CDN answers to Media3, not to the app's own fetch, and when it answers + with a non-2xx the listener gets "Playback failed: ERROR_CODE_IO_BAD_HTTP_STATUS". + - It is the automatic publisher fallback Peter removed from the download path on + 2026-10-17 ("Do not reintroduce an automatic fallback for a transient failure"), + surviving in the play path where nobody looked for it. + - Worst: nothing in playback checks `audioFromServer`, so the breaks the server measured on + *its* copy are applied to the publisher's, which has different advertising in different + places. The skips land in the wrong spot and the listener cannot tell why. + + Fetching first is what the download path already does, and what YouTube already does here. + */ + @Test + fun `an episode with no file is fetched rather than streamed from its publisher`() = runTest { + show("s1", kind = SubscriptionKind.PODCAST) + episode("e1", localPath = null) + + val outcome = advancer.start("e1") + + assertEquals(SubscriptionKind.PODCAST, (outcome as? StartOutcome.NeedsFetch)?.kind) + assertEquals("e1", (outcome as? StartOutcome.NeedsFetch)?.episode?.id) + } + + @Test + fun `an episode already on disk plays from it`() = runTest { + show("s1", kind = SubscriptionKind.PODCAST) + episode("e1", localPath = "/tmp/e1.mp3") + + assertTrue(advancer.start("e1") is StartOutcome.Ready) + } + @Test fun `fresh verdicts are read each time rather than once per episode`() = runTest { show() @@ -165,8 +203,8 @@ class PlaybackAdvancerTest { @Test fun `finishing drops it from the queue and reports what is next`() = runTest { show() - episode("a") - episode("b") + episode("a", localPath = "/audio/a.mp3") + episode("b", localPath = "/audio/b.mp3") queue.append("a") queue.append("b") @@ -205,7 +243,7 @@ class PlaybackAdvancerTest { @Test fun `a fresh episode starts past the show's intro`() = runTest { show(skipIntroMs = 47_000) - episode("a") + episode("a", localPath = "/audio/a.mp3") assertEquals(47_000L, advancer.start("a").ready()?.startPositionMs) } @@ -213,7 +251,7 @@ class PlaybackAdvancerTest { @Test fun `a part-heard episode resumes where it was left`() = runTest { show(skipIntroMs = 47_000) - episode("a", positionMs = 12 * minute) + episode("a", localPath = "/audio/a.mp3", positionMs = 12 * minute) assertEquals(12 * minute, advancer.start("a").ready()?.startPositionMs) } @@ -221,8 +259,8 @@ class PlaybackAdvancerTest { @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) + episode("a", localPath = "/audio/a.mp3") + episode("b", localPath = "/audio/b.mp3", speedOverride = 1.0f) assertEquals(1.5f, advancer.start("a").ready()?.speed) assertEquals(1.0f, advancer.start("b").ready()?.speed) @@ -232,8 +270,8 @@ class PlaybackAdvancerTest { 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") + episode("a", localPath = "/audio/a.mp3", subscriptionId = "s1") + episode("b", localPath = "/audio/b.mp3", subscriptionId = "s2") queue.append("a") queue.append("b") @@ -276,13 +314,19 @@ class PlaybackAdvancerTest { assertNotNull(advancer.start("a").ready()) } - /** An ordinary podcast enclosure is a real audio URL and streams without any of this. */ + /** + * It used to, and that was the bug. + * + * A podcast enclosure is a real audio URL, so streaming it worked -- which is why nobody + * noticed it was the publisher's differently-ad-inserted copy being played with the server's + * offsets applied to it. A podcast is fetched first now, the same as any other kind. + */ @Test - fun `a podcast episode starts without being downloaded`() = runTest { + fun `a podcast episode is not started straight off its publisher`() = runTest { show() episode("a") - assertNotNull(advancer.start("a").ready()) + assertEquals(SubscriptionKind.PODCAST, (advancer.start("a") as? StartOutcome.NeedsFetch)?.kind) } @Test diff --git a/app/src/test/java/org/terst/listen/playback/PlaybackFailureTextTest.kt b/app/src/test/java/org/terst/listen/playback/PlaybackFailureTextTest.kt new file mode 100644 index 0000000..0c47bee --- /dev/null +++ b/app/src/test/java/org/terst/listen/playback/PlaybackFailureTextTest.kt @@ -0,0 +1,80 @@ +package org.terst.listen.playback + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * What the listener is told when playback stops. + * + * It used to be the Media3 constant, printed raw: "Playback failed: ERROR_CODE_IO_BAD_HTTP_STATUS". + * Peter's words for that were "arcane and sketchy", and both halves are fair. It names an + * implementation detail of a library he does not use, and it says an HTTP request happened -- + * which was true, and was itself the bug: the player was streaming from the publisher. + * + * So these are checked for being sentences, not codes. The raw name still goes to logcat, where + * diagnosis belongs. + */ +class PlaybackFailureTextTest { + + @Test + fun `a refused request says who refused it, not what the code was called`() { + val text = PlaybackFailureText.of("ERROR_CODE_IO_BAD_HTTP_STATUS") + + assertEquals("The server refused to send this episode's audio.", text) + } + + @Test + fun `a missing file says the download has gone, which is the thing to act on`() { + assertEquals( + "This episode's download is missing. Download it again.", + PlaybackFailureText.of("ERROR_CODE_IO_FILE_NOT_FOUND"), + ) + } + + @Test + fun `a dropped connection says so plainly`() { + val expected = "No connection, so this episode could not be played." + assertEquals(expected, PlaybackFailureText.of("ERROR_CODE_IO_NETWORK_CONNECTION_FAILED")) + assertEquals(expected, PlaybackFailureText.of("ERROR_CODE_IO_NETWORK_CONNECTION_TIMEOUT")) + } + + @Test + fun `audio that will not decode says the file is the problem`() { + assertEquals( + "This episode's audio could not be played; the file may be damaged.", + PlaybackFailureText.of("ERROR_CODE_DECODING_FAILED"), + ) + } + + /** + * The honest answer when there is no better one. It must not pretend to a cause it does not + * know, and it must not fall back to printing the code. + */ + @Test + fun `an unrecognised failure says only what is true`() { + val text = PlaybackFailureText.of("ERROR_CODE_SOMETHING_NOBODY_MAPPED") + + assertEquals("This episode stopped playing.", text) + } + + /** The property that matters most, across every branch: no machine words reach the screen. */ + @Test + fun `no message leaks a code, an underscore constant or a class name`() { + val codes = listOf( + "ERROR_CODE_IO_BAD_HTTP_STATUS", "ERROR_CODE_IO_FILE_NOT_FOUND", + "ERROR_CODE_IO_NETWORK_CONNECTION_FAILED", "ERROR_CODE_IO_NETWORK_CONNECTION_TIMEOUT", + "ERROR_CODE_DECODING_FAILED", "ERROR_CODE_IO_CLEARTEXT_NOT_PERMITTED", + "ERROR_CODE_SOMETHING_NOBODY_MAPPED", "", "java.io.IOException", + ) + for (code in codes) { + val text = PlaybackFailureText.of(code) + assertFalse("$code leaked a constant: $text", text.contains("ERROR_")) + assertFalse("$code leaked an underscore: $text", text.contains("_")) + assertFalse("$code leaked a class name: $text", text.contains("Exception")) + assertTrue("$code produced nothing", text.isNotBlank()) + assertTrue("$code should end as a sentence: $text", text.trimEnd().endsWith(".")) + } + } +} diff --git a/app/src/test/java/org/terst/listen/ui/FakeUiGraph.kt b/app/src/test/java/org/terst/listen/ui/FakeUiGraph.kt index 5b43bdb..705d830 100644 --- a/app/src/test/java/org/terst/listen/ui/FakeUiGraph.kt +++ b/app/src/test/java/org/terst/listen/ui/FakeUiGraph.kt @@ -7,6 +7,7 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceUntilIdle import okhttp3.OkHttpClient +import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.FakeBreakVerdictDao import org.terst.listen.data.FakeEpisodeDao @@ -169,6 +170,14 @@ fun episodeRow( subscriptionId: String = "s1", publishedAtMs: Long? = 1_000, positionMs: Long = 0, + /** + * Downloaded by default, because that is the only state that plays. + * + * PlaybackAdvancer fetches an episode with no file rather than streaming its publisher, so a + * fixture without one never reaches the player at all -- which is a fine thing to test, and + * not what most of these tests are about. + */ + localPath: String? = "/audio/$id.mp3", ) = EpisodeEntity( id = id, subscriptionId = subscriptionId, @@ -177,6 +186,8 @@ fun episodeRow( mediaUrl = "https://cdn/$id.mp3", mediaType = "audio/mpeg", discoveredAtMs = 0, + localPath = localPath, + downloadState = if (localPath != null) DownloadState.DOWNLOADED else DownloadState.NONE, publishedAtMs = publishedAtMs, positionMs = positionMs, durationMs = 600_000, diff --git a/app/src/test/java/org/terst/listen/ui/ListenViewModelTest.kt b/app/src/test/java/org/terst/listen/ui/ListenViewModelTest.kt index 4d60762..89fa45f 100644 --- a/app/src/test/java/org/terst/listen/ui/ListenViewModelTest.kt +++ b/app/src/test/java/org/terst/listen/ui/ListenViewModelTest.kt @@ -314,7 +314,8 @@ class ListenViewModelTest { val banner = awaitBanner() assertTrue(banner.isError) - assertTrue(banner.text.startsWith("Download failed: ")) + // Not "Download failed: <whatever OkHttp said>"; the exception goes to logcat. + assertEquals(FailureText.DOWNLOAD, banner.text) } /** diff --git a/app/src/test/java/org/terst/listen/ui/OpmlViewModelTest.kt b/app/src/test/java/org/terst/listen/ui/OpmlViewModelTest.kt index af44f48..a0a3804 100644 --- a/app/src/test/java/org/terst/listen/ui/OpmlViewModelTest.kt +++ b/app/src/test/java/org/terst/listen/ui/OpmlViewModelTest.kt @@ -8,6 +8,7 @@ import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before @@ -120,12 +121,17 @@ class OpmlViewModelTest { assertEquals(Banner("No subscriptions found in that file", isError = true), awaitBanner()) } + /** + * It used to append the exception's own text, so a listener got "permission denied" or an + * SELinux path. What they can act on is the file, so that is what it names. + */ @Test - fun `a file that cannot be read says why`() { + fun `a file that cannot be read says so without quoting the exception`() { vm.importFrom { throw IOException("permission denied") } val banner = awaitBanner() - assertEquals("Could not read that file: permission denied", banner.text) + assertEquals(FailureText.OPML_READ, banner.text) + assertFalse("the exception's text is for logcat", banner.text.contains("permission denied")) assertTrue(banner.isError) } @@ -155,11 +161,12 @@ class OpmlViewModelTest { } @Test - fun `an export that cannot be written says why`() { + fun `an export that cannot be written says so without quoting the exception`() { vm.exportTo { throw IOException("read-only") } val banner = awaitBanner() - assertEquals("Export failed: read-only", banner.text) + assertEquals(FailureText.OPML_WRITE, banner.text) + assertFalse("the exception's text is for logcat", banner.text.contains("read-only")) assertTrue(banner.isError) } diff --git a/app/src/test/java/org/terst/listen/ui/PlayerViewModelTest.kt b/app/src/test/java/org/terst/listen/ui/PlayerViewModelTest.kt index 3195f46..1fb75f8 100644 --- a/app/src/test/java/org/terst/listen/ui/PlayerViewModelTest.kt +++ b/app/src/test/java/org/terst/listen/ui/PlayerViewModelTest.kt @@ -6,6 +6,7 @@ import kotlinx.coroutines.test.resetMain import kotlinx.coroutines.test.setMain import org.junit.After import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertNotNull import org.junit.Assert.assertNull import org.junit.Assert.assertTrue @@ -14,6 +15,7 @@ import org.junit.Test import org.terst.listen.data.SubscriptionEntity import org.terst.listen.data.SubscriptionKind import org.terst.listen.playback.BedtimePolicy +import org.terst.listen.playback.PlaybackFailureText import org.terst.listen.playback.PlaybackSpeed import org.terst.listen.playback.SleepTimer @@ -111,7 +113,7 @@ class PlayerViewModelTest { @Test fun `a youtube episode is fetched before it plays, and says so while it is`() { show(kind = SubscriptionKind.YOUTUBE_CHANNEL) - graph.episodes.rows += episodeRow("y1").copy(title = "A video") + graph.episodes.rows += episodeRow("y1", localPath = null).copy(title = "A video") val seen = mutableListOf<String?>() graph.fetchAudio = { episode, _ -> seen += vm.preparing.value @@ -135,7 +137,7 @@ class PlayerViewModelTest { @Test fun `a fetch that fails says why, and stops claiming to be preparing`() { show(kind = SubscriptionKind.YOUTUBE_CHANNEL) - graph.episodes.rows += episodeRow("y1") + graph.episodes.rows += episodeRow("y1", localPath = null) graph.fetchAudio = { _, _ -> Result.failure(IllegalStateException("video unavailable")) } vm.play(graph.episodes.rows.single()) @@ -195,10 +197,13 @@ class PlayerViewModelTest { controls.onError!!("ERROR_CODE_IO_BAD_HTTP_STATUS") + // The sentence, not the constant. PlaybackFailureText owns the wording and is tested + // there; what this pins is that the view model asks it rather than printing the code. assertEquals( - Banner("Playback failed: ERROR_CODE_IO_BAD_HTTP_STATUS", isError = true), + Banner(PlaybackFailureText.of("ERROR_CODE_IO_BAD_HTTP_STATUS"), isError = true), vm.banner.value, ) + assertFalse("a Media3 constant must not reach the screen", vm.banner.value!!.text.contains("ERROR_")) vm.dismissBanner() assertNull(vm.banner.value) } diff --git a/app/src/test/java/org/terst/listen/ui/ShowSettingsViewModelTest.kt b/app/src/test/java/org/terst/listen/ui/ShowSettingsViewModelTest.kt index 4ec0554..0ebd559 100644 --- a/app/src/test/java/org/terst/listen/ui/ShowSettingsViewModelTest.kt +++ b/app/src/test/java/org/terst/listen/ui/ShowSettingsViewModelTest.kt @@ -13,9 +13,7 @@ import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test -import org.terst.listen.data.FingerprintKind -import org.terst.listen.data.ShowFingerprintEntity -import org.terst.listen.data.ShowMidrollEntity +import org.terst.listen.data.EpisodeMidrollEntity import org.terst.listen.data.ShowTier import org.terst.listen.data.SubscriptionEntity import java.util.concurrent.TimeUnit @@ -267,51 +265,57 @@ class ShowSettingsViewModelTest { // --- the detection readout ---------------------------------------------------------- + /* + The readout is counted off the episodes, because that is where the server's answers land. + + It used to read show-level confirmation rows written by the on-device detector, and nothing + has written one since detection moved. Every show said "Nothing found yet" for ever -- + verified on a phone, on a show the server was actively publishing breaks for. + */ @Test - fun `a show nothing has been found for reads as three empty states`() { + fun `a show whose episodes the server has not supplied reads as not checked`() { show() + graph.episodes.rows += episodeRow("e1").copy(audioFromServer = false) vm.loadDetection("s1") assertEquals( - ShowDetection( - intro = "Nothing found yet", - outro = "Nothing found yet", - midrolls = "None found yet", - ), + ShowDetection("Not checked yet", "Not checked yet", "Not checked yet"), vm.detection.value, ) } + /** Looked and found nothing is a different answer from not looked, and reads differently. */ @Test - fun `a confirmed intro and a gathering midroll read differently`() { + fun `episodes the server supplied with no offsets read as none found`() { show() - graph.fingerprints.rows += ShowFingerprintEntity( - subscriptionId = "s1", - kind = FingerprintKind.INTRO, - referenceClipPcm = byteArrayOf(1, 2), - pendingEpisodeId = null, - sampleRate = 8_000, - confirmedCount = 9, - lastConfirmedEpisodeId = "e1", - updatedAtMs = 0, + graph.episodes.rows += episodeRow("e1").copy(audioFromServer = true) + graph.episodes.rows += episodeRow("e2").copy(audioFromServer = true) + + vm.loadDetection("s1") + + assertEquals( + ShowDetection("None found in 2 episodes", "None found in 2 episodes", "None found in 2 episodes"), + vm.detection.value, ) - graph.midrolls.candidateRows += ShowMidrollEntity( - subscriptionId = "s1", - id = "c1", - referenceClipPath = "/dev/null", - sampleRate = 8_000, - confirmedCount = 1, - lastConfirmedEpisodeId = "e1", - updatedAtMs = 0, + } + + @Test + fun `what the server did find is counted, and the ends are read separately`() { + show() + graph.episodes.rows += episodeRow("e1").copy(audioFromServer = true, detectedIntroMs = 47_000) + graph.episodes.rows += episodeRow("e2").copy(audioFromServer = true, detectedIntroMs = 46_000) + graph.episodes.rows += episodeRow("e3").copy(audioFromServer = true) + graph.midrolls.rangeRows += EpisodeMidrollEntity( + episodeId = "e1", candidateId = "c1", startMs = 60_000, endMs = 120_000, kind = "ad", ) vm.loadDetection("s1") val detection = vm.detection.value!! - assertEquals("Confirmed by 9 episodes", detection.intro) - assertEquals("the ends are read separately", "Nothing found yet", detection.outro) - assertEquals("1 still gathering", detection.midrolls) + assertEquals("Found in 2 of 3 episodes", detection.intro) + assertEquals("the ends are read separately", "None found in 3 episodes", detection.outro) + assertEquals("Found in 1 of 3 episodes", detection.midrolls) } private companion object { |