diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-11-05 08:42:28 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-11-05 08:42:28 +0000 |
| commit | 32df466c5c851c1a3e88c973627a9c07d2634e2b (patch) | |
| tree | 3c4e7cee54f4a999d7e8122573b016ae818951b9 /app | |
| parent | 9678aa3e980d1e44defb3808812291968565f869 (diff) | |
Say what happened in words, and stop streaming from publishers
Three things, from one red banner on Peter's phone reading "Playback failed:
ERROR_CODE_IO_BAD_HTTP_STATUS". His words were "arcane and sketchy". Both halves were fair, and
the second was pointing at a real bug rather than a wording problem.
**The app was streaming episodes from their publishers.** `toMediaItem` falls back to the
enclosure URL when there is no local file, and nothing stopped a play reaching it: press play on
an undownloaded episode and it opened the publisher's CDN directly. Three things wrong with that.
It is the automatic publisher fallback the download path had removed on purpose in October ("Do
not reintroduce an automatic fallback for a transient failure"), surviving where nobody looked.
It is an outbound request to a third party that nothing asked for -- the sketchy part, and Peter
was right to read it that way. And worst, nothing in playback checks `audioFromServer`, so the
breaks the server measured on *its* 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.
`PlaybackAdvancer.start` now returns NeedsFetch for any episode with no file, which routes through
the download path that already honours the server rule. One caller still reaches the fallback --
Android Auto resolves a browse id straight to a media item without the advancer, and a car that
silently plays nothing is worse than a car that streams. That needs fetching inside the session
callback; until then OutboundRequestsTest pins the fallback to exactly one line.
Ruled out first, with evidence rather than theory: the skip server (every request in its access
log is a 200), the user agent (both the app's and Media3's get 200 from the enclosure), and my own
deploy the day before.
**The messages.** Media3's constant is now a sentence (`PlaybackFailureText`), and it was not the
only one. The same pass found `Download failed: ${it.message}`, `Could not read that file:
permission denied`, and `FeedFetcher` putting the literal word `UnknownHostException` under a
podcast's name in the library -- that one reaches two surfaces, because the text is also stored as
the subscription's `lastError`. **Two existing tests asserted the machine text came through**,
which is how it survived review; they assert the opposite now.
Accuracy turned out to be the harder half. "It will be retried" reads better than "Try the
download button again" and is a lie: a failed download is deliberately left FAILED. A 404 and a
503 both mean "it didn't work" to a library and are different decisions for the person choosing
whether to unsubscribe, so they say different things. Where the cause cannot be narrowed the
sentence stays vague rather than guessing. Every cause goes to logcat -- which detekt caught me
failing to do in FeedFetcher, three swallowed exceptions after I removed their messages.
**The guardrails Peter asked for**, both watched failing before being trusted:
- `PlainLanguageTest` -- no listener-facing message may be built out of an exception or spell out
an error constant. Deliberately scoped to where errors surface rather than judging all prose.
- `OutboundRequestsTest` -- one OkHttpClient, built in AppGraph, carrying SecureSchemeInterceptor;
nothing may open a connection another way; and the publisher-stream fallback must stay at one
site. The same arrangement the server has in safefetch/audit_test.go and for the same reason:
this app fetches URLs chosen by podcast publishers.
**And the readout, which Peter asked for directly.** The per-show "Detected skips" lines read the
confirmation rows the on-device detector used to write, so since detection moved they could only
ever say "Nothing found yet" -- on every show, including ones the server was publishing breaks
for. Counted off the episodes now. Verified on a device against a staged state rather than argued:
three of five episodes carrying a detected intro, one carrying an ad break, reading "Found in 3 of
5 episodes" / "None found in 5 episodes" / "Found in 1 of 5 episodes". *Not checked* and *checked,
found nothing* are different answers again, which was the whole point of the readout.
1016 tests, detekt 0, lint clean. Coverage 94.58% -> 94.12%, COVERAGE_EXCEPTION for the same
reason as yesterday: the deleted confirmation-row code was covered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B
Diffstat (limited to 'app')
27 files changed, 672 insertions, 195 deletions
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 { |
