diff options
Diffstat (limited to 'app/src')
25 files changed, 870 insertions, 66 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 6a67184..2ab6041 100644 --- a/app/src/main/java/org/terst/listen/data/Daos.kt +++ b/app/src/main/java/org/terst/listen/data/Daos.kt @@ -301,6 +301,19 @@ interface EpisodeDao { queued: DownloadState = DownloadState.QUEUED, ): List<EpisodeEntity> + /** + * Episodes waiting on the skip server's copy, newest first so a bounded retry spends its + * budget on what is most likely still wanted. + */ + @Query( + """SELECT * FROM episodes WHERE downloadState = :waiting + ORDER BY COALESCE(publishedAtMs, discoveredAtMs) DESC LIMIT :limit""" + ) + suspend fun waitingForServer( + limit: Int, + waiting: DownloadState = DownloadState.WAITING_FOR_SERVER, + ): List<EpisodeEntity> + /** Candidates for retention: played, downloaded, and past their grace period. */ @Query( """SELECT * FROM episodes diff --git a/app/src/main/java/org/terst/listen/data/Entities.kt b/app/src/main/java/org/terst/listen/data/Entities.kt index ce160b5..8db5a8b 100644 --- a/app/src/main/java/org/terst/listen/data/Entities.kt +++ b/app/src/main/java/org/terst/listen/data/Entities.kt @@ -31,7 +31,14 @@ enum class SubscriptionKind { val needsExtraction: Boolean get() = this != PODCAST } -enum class DownloadState { NONE, QUEUED, DOWNLOADING, DOWNLOADED, FAILED } +/** + * Stored by name in a TEXT column (see `Converters`), so adding a value needs no migration. + * + * [WAITING_FOR_SERVER] is not a failure: the skip server is configured but cannot hand over its + * copy of the audio yet, and the next refresh will ask again. Falling back to the publisher's + * copy is a choice the listener makes, never something done on their behalf. + */ +enum class DownloadState { NONE, QUEUED, DOWNLOADING, DOWNLOADED, FAILED, WAITING_FOR_SERVER } @Entity( tableName = "subscriptions", diff --git a/app/src/main/java/org/terst/listen/data/Repositories.kt b/app/src/main/java/org/terst/listen/data/Repositories.kt index ad1cfd7..476a7e3 100644 --- a/app/src/main/java/org/terst/listen/data/Repositories.kt +++ b/app/src/main/java/org/terst/listen/data/Repositories.kt @@ -8,6 +8,7 @@ import kotlinx.coroutines.flow.first import okhttp3.OkHttpClient import org.terst.listen.server.OkHttpSkipTransport import org.terst.listen.server.ServerSkips +import org.terst.listen.download.WaitingDownloads import org.terst.listen.server.SkipRefresher import org.terst.listen.server.SkipServer import org.terst.listen.server.SkipServerConfig @@ -153,6 +154,7 @@ class AppGraph(private val context: Context) { probe = { url -> ChaquopyAudioSource(context).probe(url) }, onRefreshed = { feedUrls -> skipServer.pushSubscriptions(feedUrls) }, refreshSkips = { skipRefresher.refreshAll() }, + retryWaiting = { waitingDownloads.retryAll() }, ), ) } @@ -199,6 +201,11 @@ class AppGraph(private val context: Context) { ) } + /** Retries episodes waiting for the server's copy; fired by every refresh. */ + val waitingDownloads: WaitingDownloads by lazy { + WaitingDownloads(episodes, subscriptions) { episode, kind -> downloader.download(episode, kind) } + } + val retention: RetentionPolicy by lazy { RetentionPolicy(gracePeriodMs = TimeUnit.DAYS.toMillis(1)) } diff --git a/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt b/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt index 1281b43..933c838 100644 --- a/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt +++ b/app/src/main/java/org/terst/listen/data/SubscriptionRepository.kt @@ -72,6 +72,13 @@ data class SubscriptionCollaborators( * refresh's results. */ val refreshSkips: suspend () -> Unit = {}, + /** + * Retries downloads waiting for the skip server's copy. + * + * Fired from `refreshAll` for the same reason as [refreshSkips]: the choke point every + * refresh route passes through, the library's refresh button included. + */ + val retryWaiting: suspend () -> Unit = {}, ) class SubscriptionRepository( @@ -149,6 +156,9 @@ class SubscriptionRepository( // improve with no new episodes published at all, since the server keeps re-running // detection on its own schedule. collaborators.refreshSkips() + // Last, so a refresh that has just told the server about a new feed gives it the + // longest head start before the first retry asks for that feed's episodes. + collaborators.retryWaiting() return outcomes } diff --git a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt index 1479812..dfc4aad 100644 --- a/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt +++ b/app/src/main/java/org/terst/listen/download/EpisodeDownloader.kt @@ -29,7 +29,18 @@ import java.io.IOException * database writes do not. */ interface ServerAudio { - /** What the server has for this episode, or null to download from the publisher. */ + /** + * Whether there is a server to ask about this episode at all: one is configured, and the + * episode's feed can be identified to it. + * + * Not the same question as whether [resolve] has an answer. `resolve` returns null both when + * nothing is configured and when the server is merely unreachable, and the downloader must + * treat those oppositely -- nothing configured means the publisher's copy is the only copy + * there is, while an unreachable server means its copy is coming. + */ + suspend fun canAsk(episode: EpisodeEntity): Boolean + + /** What the server has for this episode, or null if it has no answer right now. */ suspend fun resolve(episode: EpisodeEntity): ServerEpisode? /** Records the server's offsets, once its audio is actually the file on disk. */ @@ -42,7 +53,8 @@ class EpisodeDownloader( private val libraryDir: File, private val extractor: ExtractedAudioSource? = null, /** - * The skip server, preferred over the publisher when it has the episode. + * The skip server. When one is configured, its copy is the only one downloaded + * automatically; an episode it cannot supply yet waits (see [WaitingForServerException]). * * Not an optimisation. A show using dynamic ad insertion serves a different file on every * request -- one real episode measured 22:05 on one fetch and 28:18 on another -- so an @@ -86,11 +98,45 @@ class EpisodeDownloader( // localPath is cleared: a FAILED row pointing at a path invites something downstream // to try playing a file that is not there. audioFromServer defaults to false on this // call, clearing any stale claim from an earlier download the same way. - onFailure = { episodes.updateDownload(episode.id, DownloadState.FAILED, null, null) }, + // + // A wait is not a failure and has its own state: FAILED means the download itself + // went wrong and has its own UI, where this is an episode that will be fetched on a + // later refresh with no one needing to do anything. + onFailure = { + val state = if (it is WaitingForServerException) { + DownloadState.WAITING_FOR_SERVER + } else { + DownloadState.FAILED + } + episodes.updateDownload(episode.id, state, null, null) + }, ) return result.map { it.file } } + /** + * The listener's deliberate choice to take the publisher's copy of a podcast episode instead + * of waiting for the server's. + * + * Always recorded as not the server's: the publisher serves a different file on every + * request, so the skips measured on the server's copy do not apply to it, and + * `audioFromServer = false` is what stops the skip-refresh sweep stamping them on anyway. + * A failure here is a plain FAILED -- the listener asked for this, so it should show. + */ + suspend fun downloadFromPublisher(episode: EpisodeEntity): Result<File> { + episodes.updateDownload(episode.id, DownloadState.DOWNLOADING, null, null) + val result = downloadOverHttp(episode, episode.mediaUrl) + result.fold( + onSuccess = { + episodes.updateDownload( + episode.id, DownloadState.DOWNLOADED, it.absolutePath, now(), audioFromServer = false, + ) + }, + onFailure = { episodes.updateDownload(episode.id, DownloadState.FAILED, null, null) }, + ) + return result + } + /** What actually landed on disk, and whether it was the skip server's own copy of it. */ private data class AudioResult(val file: File, val fromServer: Boolean) @@ -105,25 +151,35 @@ class EpisodeDownloader( } /** - * Prefers the skip server, falling back to the publisher. + * Takes the server's copy, or waits for it; the publisher's copy only when there is no + * server to ask. * - * The fallback is deliberate and matters: skips improve playback rather than being a - * precondition for it, so a server that is down or mid-fetch must not turn a reachable - * episode into a failed download. Offsets are recorded only when the server's audio is - * actually what landed on disk. + * The publisher serves a different file on every request, so skips measured on the server's + * copy are wrong for anything else. Falling back silently therefore produced an episode whose + * skips could never apply, and the owner has ruled that out: with a server configured, an + * episode it cannot supply yet (pending, unreachable, a bad token, an unreadable answer, or + * audio that then fails to arrive) fails with [WaitingForServerException] and is retried on + * the next refresh. Taking the publisher's copy is [downloadFromPublisher], on request. + * + * With no server configured, or no feed to identify the episode to it, there is nothing to + * wait for, and this downloads from the publisher exactly as the app did before the server + * existed. Offsets are recorded only when the server's audio is what landed on disk. */ private suspend fun downloadPodcast(episode: EpisodeEntity): Result<AudioResult> { - val fromServer = serverAudio?.resolve(episode) - val serverUrl = fromServer?.audioUrl?.takeIf { fromServer.ready } - - if (serverUrl != null) { - val result = downloadOverHttp(episode, serverUrl) - if (result.isSuccess) { - serverAudio?.record(episode, fromServer) - return result.map { AudioResult(it, fromServer = true) } - } + val server = serverAudio + if (server == null || !server.canAsk(episode)) { + return downloadOverHttp(episode, episode.mediaUrl).map { AudioResult(it, fromServer = false) } } - return downloadOverHttp(episode, episode.mediaUrl).map { AudioResult(it, fromServer = false) } + + val info = server.resolve(episode) + val serverUrl = info?.audioUrl?.takeIf { info.ready } + ?: return Result.failure(WaitingForServerException()) + + val result = downloadOverHttp(episode, serverUrl) + if (result.isFailure) return Result.failure(WaitingForServerException(result.exceptionOrNull())) + + server.record(episode, info) + return result.map { AudioResult(it, fromServer = true) } } private suspend fun downloadViaExtractor(episode: EpisodeEntity): Result<File> { @@ -210,6 +266,15 @@ class EpisodeDownloader( } } +/** + * The skip server is configured but cannot supply this episode's audio yet. + * + * Carried as a failed [Result] so existing callers need no new return type, but never stored as + * a failure: [EpisodeDownloader.download] records it as `WAITING_FOR_SERVER`. + */ +class WaitingForServerException(cause: Throwable? = null) : + IOException("Waiting for the server's copy; will retry on the next refresh", cause) + /** Indirection so the Chaquopy-backed downloader can be swapped out in tests. */ interface ExtractedAudioSource { suspend fun download(url: String, targetDir: File): File diff --git a/app/src/main/java/org/terst/listen/download/WaitingDownloads.kt b/app/src/main/java/org/terst/listen/download/WaitingDownloads.kt new file mode 100644 index 0000000..bfbfbd9 --- /dev/null +++ b/app/src/main/java/org/terst/listen/download/WaitingDownloads.kt @@ -0,0 +1,73 @@ +package org.terst.listen.download + +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.sync.Semaphore +import kotlinx.coroutines.sync.withPermit +import org.terst.listen.data.EpisodeDao +import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.SubscriptionDao +import org.terst.listen.data.SubscriptionKind +import java.io.File +import java.util.concurrent.atomic.AtomicInteger + +/** + * Retries the downloads that are waiting for the skip server's copy, on a feed refresh. + * + * An episode lands in `WAITING_FOR_SERVER` when a server is configured but could not supply its + * audio yet. The server has started fetching it, so asking again later usually succeeds; this is + * that later. Run from `SubscriptionRepository.refreshAll` -- the one choke point every refresh + * route passes through, the library's refresh button included. + * + * Unlike [org.terst.listen.server.SkipRefresher] each retry may be a real, tens-of-megabytes + * download, so it is bounded twice: [CONCURRENT] at a time, and at most [MAX_PER_REFRESH] per + * refresh, newest first. The rest simply wait for the following refresh. + */ +class WaitingDownloads( + private val episodes: EpisodeDao, + private val subscriptions: SubscriptionDao, + private val download: suspend (EpisodeEntity, SubscriptionKind) -> Result<File>, +) { + + /** + * Returns how many waiting episodes were downloaded. + * + * One episode's failure must not stop the rest, so each is isolated; [CancellationException] + * is the one throwable deliberately not caught, or a cancelled refresh would keep running -- + * the bug already fixed once in `skipFurniture()`. `coroutineScope` keeps every retry a + * structured child of this call. + */ + suspend fun retryAll(): Int = coroutineScope { + val semaphore = Semaphore(CONCURRENT) + val downloaded = AtomicInteger() + episodes.waitingForServer(MAX_PER_REFRESH).map { episode -> + async { + semaphore.withPermit { + // Deliberately broad, for the reason above. Logged rather than discarded. + @Suppress("TooGenericExceptionCaught") + try { + // A show that has gone has no kind to download by; guessing PODCAST could + // fetch a watch page over plain HTTP. + val kind = subscriptions.getById(episode.subscriptionId)?.kind + if (kind != null && download(episode, kind).isSuccess) downloaded.incrementAndGet() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + android.util.Log.w("WaitingDownloads", "retry failed for ${episode.id}", e) + } + } + } + }.awaitAll() + downloaded.get() + } + + companion object { + /** Downloads are heavy; a couple at once is plenty and keeps the refresh button quick. */ + const val CONCURRENT = 2 + + /** One refresh's worth, like [org.terst.listen.work.DownloadDrainer]'s batch. */ + const val MAX_PER_REFRESH = 10 + } +} diff --git a/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt b/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt new file mode 100644 index 0000000..70c68c2 --- /dev/null +++ b/app/src/main/java/org/terst/listen/download/WaitingForServerText.kt @@ -0,0 +1,42 @@ +package org.terst.listen.download + +import org.terst.listen.data.DownloadState +import org.terst.listen.data.EpisodeEntity + +/** + * Everything the app says about an episode waiting for the skip server's copy, in one testable + * place rather than inside composables -- the same arrangement as `SkipSummary`, whose readout + * tests pin this wording too. + */ +object WaitingForServerText { + + /** The episode screen's readout while an episode waits on the server's copy. */ + const val STATUS = "Waiting for the server's copy; will retry on the next refresh" + + /** The one-line list-row tag for the same state. */ + const val ROW_LABEL = "waiting for server" + + /** What the snackbar says after tapping download on a waiting episode that still is not ready. */ + const val BANNER = "The server doesn't have this one yet. It will be retried on the next refresh." + + const val PUBLISHER_ACTION_LABEL = "Download from publisher instead" + + /** + * The trade, said plainly: ads are inserted per download, so the publisher's file is not the + * one the skips were measured on. + */ + const val PUBLISHER_ACTION_WARNING = + "The publisher's file can differ from the server's copy, because ads are inserted per " + + "download. The skips were measured on the server's copy, so they will not apply to this one." + + /** The explicit way out of a wait: label for the button, warning shown beside it. */ + data class PublisherOffer(val label: String, val warning: String) + + /** Offered only while the episode is waiting; any other state has nothing to choose. */ + fun publisherOffer(episode: EpisodeEntity): PublisherOffer? = + if (episode.downloadState == DownloadState.WAITING_FOR_SERVER) { + PublisherOffer(PUBLISHER_ACTION_LABEL, PUBLISHER_ACTION_WARNING) + } else { + null + } +} 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 0c0a4e3..9c2576d 100644 --- a/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt +++ b/app/src/main/java/org/terst/listen/fingerprint/SkipSummary.kt @@ -1,5 +1,6 @@ package org.terst.listen.fingerprint +import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity @@ -183,6 +184,10 @@ object SkipSummary { outro = offset(episode.detectedOutroMs, show?.skipOutroMs ?: 0), breaks = when { breaks.isNotEmpty() -> breaks + // Nothing has been downloaded yet, so the skips cannot have been checked either; + // say what is actually happening instead of "Not checked yet". + episode.downloadState == DownloadState.WAITING_FOR_SERVER -> + listOf(BreakLine("Breaks", WaitingForServerText.STATUS)) // A download that is not the server's own copy can never be refreshed in // place -- a show with dynamic ad insertion serves a different file on every // request, so the server's timestamps only mean anything against its own diff --git a/app/src/main/java/org/terst/listen/server/ServerSkips.kt b/app/src/main/java/org/terst/listen/server/ServerSkips.kt index aec261b..2a182cf 100644 --- a/app/src/main/java/org/terst/listen/server/ServerSkips.kt +++ b/app/src/main/java/org/terst/listen/server/ServerSkips.kt @@ -23,6 +23,9 @@ class ServerSkips( private val midrolls: MidrollDao, ) : ServerAudio { + override suspend fun canAsk(episode: EpisodeEntity): Boolean = + server.isConfigured() && subscriptions.getById(episode.subscriptionId) != null + /** * Asks the server about an episode, identifying it the way the server keys them. * diff --git a/app/src/main/java/org/terst/listen/server/SkipServer.kt b/app/src/main/java/org/terst/listen/server/SkipServer.kt index a1d3ccc..69dbeec 100644 --- a/app/src/main/java/org/terst/listen/server/SkipServer.kt +++ b/app/src/main/java/org/terst/listen/server/SkipServer.kt @@ -96,6 +96,16 @@ class SkipServer( ) { /** + * Whether an address and token are set at all. + * + * Separate from [episode] returning null because that null also covers an unreachable server, + * and the download path must treat the two oppositely: no server means download from the + * publisher as the app always did, while a server that merely did not answer means wait. + */ + suspend fun isConfigured(): Boolean = config() != null + + + /** * Tells the server what the phone subscribes to. * * The phone owns this list: everything already built around it -- the share sheet, OPML diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt b/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt index 009c8c0..a108e04 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeDetailActions.kt @@ -41,6 +41,11 @@ data class EpisodeDetailActions( val play: () -> Unit, val enqueue: () -> Unit, val download: () -> Unit, + /** + * Takes the publisher's copy of a waiting episode. Deliberately its own action rather than + * a mode of [download], so it can only be reached from the button that states the trade. + */ + val downloadFromPublisher: () -> Unit = {}, val togglePlayed: () -> Unit, val markOlderPlayed: () -> Unit, /** Play/pause when this episode is the one playing; starts it when it is not. */ diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt b/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt index 3574d25..dd91e18 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeRow.kt @@ -17,6 +17,7 @@ import androidx.compose.material.icons.filled.ErrorOutline import androidx.compose.material.icons.filled.PlayArrow import androidx.compose.material.icons.filled.PlaylistAdd import androidx.compose.material.icons.filled.PlaylistPlay +import androidx.compose.material.icons.filled.Schedule import androidx.compose.material.icons.outlined.CheckCircle import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.Icon @@ -31,6 +32,7 @@ import androidx.compose.ui.unit.dp import coil3.compose.AsyncImage import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity +import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.playbackDurationMs import java.util.concurrent.TimeUnit @@ -109,6 +111,14 @@ private fun DownloadIcon(state: DownloadState, onDownload: () -> Unit) { tint = MaterialTheme.colorScheme.primary, ) + DownloadState.WAITING_FOR_SERVER -> + IconButton(onClick = onDownload) { + Icon( + Icons.Default.Schedule, + contentDescription = "Waiting for the server's copy, tap to try now", + ) + } + DownloadState.FAILED -> IconButton(onClick = onDownload) { Icon( @@ -132,6 +142,7 @@ internal fun EpisodeEntity.progressLabel(): String { if (positionMs > 0) parts += "${formatDuration(positionMs)} in" if (playedAtMs != null) parts += "played" if (downloadState == DownloadState.FAILED) parts += "download failed" + if (downloadState == DownloadState.WAITING_FOR_SERVER) parts += WaitingForServerText.ROW_LABEL return parts.joinToString(" · ") } diff --git a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt index 70f4a77..b3a7c96 100644 --- a/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt +++ b/app/src/main/java/org/terst/listen/ui/EpisodeScreen.kt @@ -36,6 +36,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableFloatStateOf import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -45,7 +46,9 @@ import androidx.compose.ui.geometry.Size import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import coil3.compose.AsyncImage +import kotlinx.coroutines.launch import org.terst.listen.data.EpisodeEntity +import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.EpisodeMidrollEntity import org.terst.listen.data.SubscriptionEntity import org.terst.listen.data.playbackDurationMs @@ -77,6 +80,7 @@ fun EpisodeScreen( episode = vm.episode(episodeId) ranges = vm.midrollRanges(episodeId) } + val scope = rememberCoroutineScope() val rules by vm.rules.collectAsState() val shows by vm.subscriptions.collectAsState() val nowPlaying by player.nowPlaying.collectAsState() @@ -108,6 +112,8 @@ fun EpisodeScreen( play = { player.play(current); nav.openPlayer() }, enqueue = { vm.enqueue(current.id) }, download = { vm.download(current) }, + // Re-reads afterwards, or the screen keeps offering the choice it just acted on. + downloadFromPublisher = { vm.downloadFromPublisher(current) { scope.launch { episode = vm.episode(episodeId) } } }, togglePlayed = { if (current.playedAtMs == null) vm.markPlayed(current.id) else vm.markUnplayed(current.id) episode = current.copy(playedAtMs = if (current.playedAtMs == null) 1L else null) @@ -172,6 +178,15 @@ private fun EpisodeDetail( WatchButton(WatchLink.at(episode.mediaUrl, episode.positionMs), "Watch") } + // The way out of a wait, and what it costs, side by side. Wording is SkipSummary's so a + // test pins it. + WaitingForServerText.publisherOffer(episode)?.let { offer -> + Text(offer.warning, style = MaterialTheme.typography.bodySmall) + OutlinedButton(onClick = actions.downloadFromPublisher, modifier = Modifier.fillMaxWidth()) { + Text(offer.label) + } + } + // The import case: a freshly imported back catalogue is entirely unplayed, and this is // the only practical way to say "I've heard everything up to here". OutlinedButton(onClick = actions.markOlderPlayed, modifier = Modifier.fillMaxWidth()) { 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 d03c7a5..4365729 100644 --- a/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt +++ b/app/src/main/java/org/terst/listen/ui/ListenViewModel.kt @@ -1,5 +1,6 @@ package org.terst.listen.ui +import org.terst.listen.download.WaitingForServerException import android.app.Application import androidx.lifecycle.AndroidViewModel import androidx.lifecycle.viewModelScope @@ -12,6 +13,7 @@ import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import org.terst.listen.data.EpisodeEntity +import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.Graph import org.terst.listen.data.Preferences import org.terst.listen.data.RefreshOutcome @@ -218,8 +220,26 @@ class ListenViewModel(app: Application) : AndroidViewModel(app) { val subscription = graph.subscriptions.getById(episode.subscriptionId) val kind = subscription?.kind ?: SubscriptionKind.PODCAST graph.downloader.download(episode, kind).onFailure { + // Not an error: the episode is now waiting and the next refresh retries it. + _banner.value = if (it is WaitingForServerException) { + Banner(WaitingForServerText.BANNER) + } else { + Banner("Download failed: ${it.message}", isError = true) + } + } + } + } + + /** + * The listener's explicit choice to take the publisher's copy of a waiting episode. The + * screen shows the trade (see [WaitingForServerText.publisherOffer]) before this can be tapped. + */ + fun downloadFromPublisher(episode: EpisodeEntity, onDone: () -> Unit = {}) { + viewModelScope.launch { + graph.downloader.downloadFromPublisher(episode).onFailure { _banner.value = Banner("Download failed: ${it.message}", isError = true) } + onDone() } } diff --git a/app/src/test/java/org/terst/listen/data/ConvertersTest.kt b/app/src/test/java/org/terst/listen/data/ConvertersTest.kt new file mode 100644 index 0000000..1387799 --- /dev/null +++ b/app/src/test/java/org/terst/listen/data/ConvertersTest.kt @@ -0,0 +1,19 @@ +package org.terst.listen.data + +import org.junit.Assert.assertEquals +import org.junit.Test + +/** + * [DownloadState] is stored by name, which is why adding [DownloadState.WAITING_FOR_SERVER] + * needs no schema migration: the column is plain TEXT and nothing constrains its values. + */ +class ConvertersTest { + @Test + fun `every download state survives a round trip through its stored form`() { + val converters = Converters() + DownloadState.entries.forEach { + assertEquals(it, converters.toDownloadState(converters.fromDownloadState(it))) + } + assertEquals("WAITING_FOR_SERVER", converters.fromDownloadState(DownloadState.WAITING_FOR_SERVER)) + } +} 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 c9937fb..9515c3d 100644 --- a/app/src/test/java/org/terst/listen/data/FakeDaos.kt +++ b/app/src/test/java/org/terst/listen/data/FakeDaos.kt @@ -237,6 +237,11 @@ class FakeEpisodeDao : EpisodeDao { override suspend fun downloadedFromServer(downloaded: DownloadState) = rows.filter { it.downloadState == downloaded && it.audioFromServer } + override suspend fun waitingForServer(limit: Int, waiting: DownloadState) = + rows.filter { it.downloadState == waiting } + .sortedByDescending { it.publishedAtMs ?: it.discoveredAtMs } + .take(limit) + override suspend fun setChapters(id: String, json: String?) { replace(id) { it.copy(chaptersJson = json) } } 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 63ac219..1c8cda1 100644 --- a/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt +++ b/app/src/test/java/org/terst/listen/data/SubscriptionRepositoryTest.kt @@ -570,4 +570,25 @@ class SubscriptionRepositoryTest { assertEquals(1, calls) } + + /** Same choke point, same reason: the refresh button must retry waiting downloads too. */ + @Test + fun `refreshAll retries waiting downloads exactly once, after the refresh`() = runTest { + var calls = 0 + val repoWithRetry = SubscriptionRepository( + subs, + eps, + FeedFetcher(OkHttpClient()), + SubscriptionCollaborators(retryWaiting = { calls++ }), + ) { clock } + + server.enqueue(MockResponse().setBody(feed())) + repoWithRetry.subscribe(url()) + assertEquals("subscribing is not a refresh", 0, calls) + + server.enqueue(MockResponse().setBody(feed())) + repoWithRetry.refreshAll() + + assertEquals(1, calls) + } } diff --git a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt index d524200..46e15c0 100644 --- a/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt +++ b/app/src/test/java/org/terst/listen/download/EpisodeDownloaderTest.kt @@ -171,8 +171,13 @@ class EpisodeDownloaderTest { // wrong for a copy the phone fetched itself. Downloading from the server is what makes its // numbers apply to the bytes being played. - private class FakeServerAudio(private val info: ServerEpisode?) : ServerAudio { + private class FakeServerAudio( + private val info: ServerEpisode?, + /** Whether a server is configured at all -- a different thing from it having no answer. */ + private val configured: Boolean = true, + ) : ServerAudio { var recorded: Pair<String, ServerEpisode>? = null + override suspend fun canAsk(episode: EpisodeEntity) = configured override suspend fun resolve(episode: EpisodeEntity) = info override suspend fun record(episode: EpisodeEntity, info: ServerEpisode) { recorded = episode.id to info @@ -224,19 +229,21 @@ class EpisodeDownloaderTest { } @Test - fun `downloads from the publisher when the server has nothing yet`() = runTest { + fun `downloads from the publisher at once when no server is configured`() = runTest { server.enqueue(MockResponse().setBody("publisher-bytes")) val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } - val result = withServer(FakeServerAudio(null)).download(ep, SubscriptionKind.PODCAST) + // resolve() answers null here exactly as it does for an unreachable server; only + // canAsk tells the two apart, which is what this test pins. + val result = withServer(FakeServerAudio(null, configured = false)).download(ep, SubscriptionKind.PODCAST) assertTrue(result.isSuccess) assertEquals("/original.mp3", server.takeRequest().path) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single().downloadState) } @Test fun `records nothing when the server had nothing`() = runTest { - server.enqueue(MockResponse().setBody("publisher-bytes")) val audio = FakeServerAudio(null) val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } @@ -245,44 +252,117 @@ class EpisodeDownloaderTest { assertNull(audio.recorded) } - /** - * The server going down must not cost Peter the episode. Skips are an improvement on - * playback, not a precondition for it -- and a dependency that turns a reachable publisher - * into a failed download would make the app worse than before the server existed. - */ + // ---- waiting for the server's copy, instead of silently taking the publisher's ---- + // + // The publisher serves a different file on every request, so a silent fallback produces an + // episode whose skips cannot apply. With a server configured the episode waits; taking the + // publisher's copy is the listener's call (downloadFromPublisher). + + private fun pending() = ServerEpisode( + ready = false, audioUrl = null, durationMs = null, introMs = null, outroMs = null, detected = false, + ) + + private fun brokenReady() = ServerEpisode( + ready = true, audioUrl = server.url("/audio/broken").toString(), + durationMs = null, introMs = 1, outroMs = 2, detected = true, + ) + + private fun assertWaiting(result: Result<File>) { + assertTrue("a wait is reported as not-yet-downloaded", result.isFailure) + val stored = dao.rows.single() + assertEquals(DownloadState.WAITING_FOR_SERVER, stored.downloadState) + assertNull(stored.localPath) + assertFalse(stored.audioFromServer) + assertEquals("nothing should be left in the library", 0, library.listFiles()!!.size) + } + + @Test + fun `a pending server copy waits and never touches the publisher`() = runTest { + val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + + val result = withServer(FakeServerAudio(pending())).download(ep, SubscriptionKind.PODCAST) + + assertWaiting(result) + assertEquals("the publisher must not be contacted", 0, server.requestCount) + } + @Test - fun `falls back to the publisher when the server's own audio fails`() = runTest { + fun `no answer from a configured server waits and never touches the publisher`() = runTest { + val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + + val result = withServer(FakeServerAudio(null, configured = true)).download(ep, SubscriptionKind.PODCAST) + + assertWaiting(result) + assertEquals(0, server.requestCount) + } + + @Test + fun `a ready server whose audio then fails waits, is not failed, and skips the publisher`() = runTest { server.enqueue(MockResponse().setResponseCode(502)) - server.enqueue(MockResponse().setBody("publisher-bytes")) - val audio = FakeServerAudio( - ServerEpisode(ready = true, audioUrl = server.url("/audio/broken").toString(), - durationMs = null, introMs = 1, outroMs = 2, detected = true) - ) + val audio = FakeServerAudio(brokenReady()) val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } val result = withServer(audio).download(ep, SubscriptionKind.PODCAST) - assertTrue("expected the publisher fallback to succeed", result.isSuccess) - assertEquals("publisher-bytes", result.getOrThrow().readText()) + assertWaiting(result) + assertEquals(1, server.requestCount) assertEquals("/audio/broken", server.takeRequest().path) - assertEquals("/original.mp3", server.takeRequest().path) + assertNull("offsets describe the server's copy, which never landed", audio.recorded) } - /** Offsets describe the server's copy, so they must not be recorded against a file that - * came from the publisher instead. */ @Test - fun `records nothing when the audio came from the publisher after all`() = runTest { - server.enqueue(MockResponse().setResponseCode(502)) + fun `an extracted video never waits even when a server is configured`() = runTest { + val extractor = object : ExtractedAudioSource { + override suspend fun download(url: String, targetDir: File) = + File(targetDir, "v.m4a").also { it.writeText("video-audio") } + + override suspend fun probe(url: String): org.terst.listen.data.ProbedVideo? = null + } + val audio = FakeServerAudio(null, configured = true) + val youtube = EpisodeDownloader(OkHttpClient(), dao, library, extractor, audio) { 1_000L } + val ep = episode(url = "https://www.youtube.com/watch?v=abc").also { dao.rows += it } + + val result = youtube.download(ep, SubscriptionKind.YOUTUBE_CHANNEL) + assertTrue(result.isSuccess) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single().downloadState) + + val fb = episode(id = "fb", url = "https://facebook.com/watch?v=1").also { dao.rows += it } + assertTrue(youtube.download(fb, SubscriptionKind.FACEBOOK_VIDEO).isSuccess) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single { it.id == "fb" }.downloadState) + } + + // ---- the explicit choice -------------------------------------------------------------- + + @Test + fun `downloading from the publisher on request lands the file and says it is not the server's`() = runTest { server.enqueue(MockResponse().setBody("publisher-bytes")) - val audio = FakeServerAudio( - ServerEpisode(ready = true, audioUrl = server.url("/audio/broken").toString(), - durationMs = null, introMs = 1, outroMs = 2, detected = true) - ) - val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } + val ep = episode(url = server.url("/original.mp3").toString()) + .copy(downloadState = DownloadState.WAITING_FOR_SERVER) + .also { dao.rows += it } + val audio = FakeServerAudio(pending()) - withServer(audio).download(ep, SubscriptionKind.PODCAST) + val result = withServer(audio).downloadFromPublisher(ep) - assertNull("recorded offsets for audio it did not download", audio.recorded) + assertTrue(result.isSuccess) + assertEquals("/original.mp3", server.takeRequest().path) + val stored = dao.rows.single() + assertEquals(DownloadState.DOWNLOADED, stored.downloadState) + assertFalse("the skip sweep keys off this", stored.audioFromServer) + assertEquals("publisher-bytes", File(stored.localPath!!).readText()) + assertNull("the server was not consulted or recorded", audio.recorded) + } + + @Test + fun `a failed publisher download on request is a failure the listener can see`() = runTest { + server.enqueue(MockResponse().setResponseCode(500)) + val ep = episode(url = server.url("/original.mp3").toString()) + .copy(downloadState = DownloadState.WAITING_FOR_SERVER) + .also { dao.rows += it } + + val result = withServer(FakeServerAudio(pending())).downloadFromPublisher(ep) + + assertTrue(result.isFailure) + assertEquals(DownloadState.FAILED, dao.rows.single().downloadState) } // ---- provenance: which file actually landed ----------------------------------------- @@ -312,28 +392,12 @@ class EpisodeDownloaderTest { server.enqueue(MockResponse().setBody("publisher-bytes")) val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } - withServer(FakeServerAudio(null)).download(ep, SubscriptionKind.PODCAST) + withServer(FakeServerAudio(null, configured = false)).download(ep, SubscriptionKind.PODCAST) assertFalse("the publisher's own audio must not be marked as the server's", dao.rows.single().audioFromServer) } - @Test - fun `falling back to the publisher after the server fails is not marked as the server's`() = runTest { - server.enqueue(MockResponse().setResponseCode(502)) - server.enqueue(MockResponse().setBody("publisher-bytes")) - val audio = FakeServerAudio( - ServerEpisode(ready = true, audioUrl = server.url("/audio/broken").toString(), - durationMs = null, introMs = 1, outroMs = 2, detected = true) - ) - val ep = episode(url = server.url("/original.mp3").toString()).also { dao.rows += it } - - withServer(audio).download(ep, SubscriptionKind.PODCAST) - - assertFalse("the fallback file is the publisher's, not the server's", - dao.rows.single().audioFromServer) - } - /** A YouTube/Facebook download never goes near the skip server at all. */ @Test fun `an extracted video download is not marked as server-sourced`() = runTest { @@ -364,7 +428,7 @@ class EpisodeDownloaderTest { File(dao.rows.single().localPath!!).delete() server.enqueue(MockResponse().setBody("publisher-bytes")) - withServer(FakeServerAudio(null)).download(dao.rows.single(), SubscriptionKind.PODCAST) + withServer(FakeServerAudio(null, configured = false)).download(dao.rows.single(), SubscriptionKind.PODCAST) assertFalse( "a previous download's provenance must not survive a re-download from elsewhere", diff --git a/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt b/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt new file mode 100644 index 0000000..6162cae --- /dev/null +++ b/app/src/test/java/org/terst/listen/download/WaitForServerWiringTest.kt @@ -0,0 +1,123 @@ +package org.terst.listen.download + +import kotlinx.coroutines.test.runTest +import okhttp3.OkHttpClient +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.terst.listen.data.DownloadState +import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.FakeEpisodeDao +import org.terst.listen.data.FakeMidrollDao +import org.terst.listen.data.FakePlaybackStateDao +import org.terst.listen.data.FakeShowFingerprintDao +import org.terst.listen.data.FakeSubscriptionDao +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.data.SubscriptionKind +import org.terst.listen.server.ServerSkips +import org.terst.listen.server.SkipServer +import org.terst.listen.server.SkipServerConfig +import org.terst.listen.server.SkipTransport + +/** + * The real downloader over the real [ServerSkips] and [SkipServer], with only the HTTP faked. + * + * The unit tests above fake [ServerAudio] and so cannot see the distinction that matters most: + * "no server configured" and "server unreachable" both come out of the client as null. These + * run the actual client so a regression there -- the app waiting forever for a server nobody + * set up -- is caught where it would really happen. + */ +class WaitForServerWiringTest { + + @get:Rule val temp = TemporaryFolder() + + private lateinit var publisher: MockWebServer + private lateinit var dao: FakeEpisodeDao + private lateinit var subscriptions: FakeSubscriptionDao + + private class Transport(private val body: String?) : SkipTransport { + var gets = 0 + override suspend fun get(url: String, token: String): String? { + gets++ + return body + } + + override suspend fun post(url: String, token: String, body: String) = true + } + + @Before + fun setUp() { + publisher = MockWebServer().apply { start() } + dao = FakeEpisodeDao() + subscriptions = FakeSubscriptionDao().apply { + rows += SubscriptionEntity(id = "s1", feedUrl = "https://feed", title = "Show", addedAtMs = 0) + } + } + + @After + fun tearDown() = publisher.shutdown() + + private fun episode(subscriptionId: String = "s1") = EpisodeEntity( + id = "e1", subscriptionId = subscriptionId, guid = "g", title = "T", + mediaUrl = publisher.url("/orig.mp3").toString(), mediaType = "audio/mpeg", discoveredAtMs = 0, + ).also { dao.rows += it } + + private fun downloader(transport: Transport, configured: Boolean): EpisodeDownloader { + val skipServer = SkipServer( + { if (configured) SkipServerConfig("https://listen.example", "tok") else null }, + transport, + ) + val fingerprints = FakeShowFingerprintDao().apply { episodeRows = dao.rows } + val serverSkips = ServerSkips(skipServer, subscriptions, fingerprints, FakePlaybackStateDao(dao.rows), FakeMidrollDao()) + return EpisodeDownloader(OkHttpClient(), dao, temp.newFolder(), null, serverSkips) { 1L } + } + + @Test + fun `no server configured downloads from the publisher immediately`() = runTest { + publisher.enqueue(MockResponse().setBody("publisher-bytes")) + val transport = Transport(null) + + val result = downloader(transport, configured = false).download(episode(), SubscriptionKind.PODCAST) + + assertTrue(result.isSuccess) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single().downloadState) + assertEquals("/orig.mp3", publisher.takeRequest().path) + assertEquals("an unconfigured server must not even be asked", 0, transport.gets) + } + + @Test + fun `a missing feed url downloads from the publisher, there being nothing to ask`() = runTest { + publisher.enqueue(MockResponse().setBody("publisher-bytes")) + + val result = downloader(Transport("""{"status":"pending"}"""), configured = true) + .download(episode(subscriptionId = "gone"), SubscriptionKind.PODCAST) + + assertTrue(result.isSuccess) + assertEquals(DownloadState.DOWNLOADED, dao.rows.single().downloadState) + } + + private fun assertWaitsWithoutPublisher(body: String?) = runTest { + val transport = Transport(body) + + val result = downloader(transport, configured = true).download(episode(), SubscriptionKind.PODCAST) + + assertTrue(result.isFailure) + assertEquals(DownloadState.WAITING_FOR_SERVER, dao.rows.single().downloadState) + assertEquals(1, transport.gets) + assertEquals("the publisher must not be contacted", 0, publisher.requestCount) + } + + @Test fun `an unreachable server waits`() = assertWaitsWithoutPublisher(null) + + @Test fun `a response that will not parse waits`() = assertWaitsWithoutPublisher("<html>502 bad gateway") + + @Test fun `a pending episode waits`() = assertWaitsWithoutPublisher("""{"status":"pending","detected":false}""") + + @Test fun `a ready status with no audio url waits`() = assertWaitsWithoutPublisher("""{"status":"ready"}""") +} diff --git a/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt b/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt new file mode 100644 index 0000000..465af46 --- /dev/null +++ b/app/src/test/java/org/terst/listen/download/WaitingDownloadsTest.kt @@ -0,0 +1,198 @@ +package org.terst.listen.download + +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runCurrent +import kotlinx.coroutines.test.runTest +import okhttp3.OkHttpClient +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.assertTrue +import org.junit.Assert.fail +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.terst.listen.data.DownloadState +import org.terst.listen.data.EpisodeEntity +import org.terst.listen.data.FakeEpisodeDao +import org.terst.listen.data.FakeSubscriptionDao +import org.terst.listen.data.SubscriptionEntity +import org.terst.listen.data.SubscriptionKind +import org.terst.listen.server.ServerEpisode +import java.io.File + +/** + * Retrying the episodes that are waiting for the skip server's copy, on a feed refresh. + * + * Bounded and isolated for the same reasons as `SkipRefresher`: a library can accumulate many + * waiting episodes, each a real download, and one bad one must not stop the rest. + */ +class WaitingDownloadsTest { + + @get:Rule val temp = TemporaryFolder() + + private lateinit var publisher: MockWebServer + private lateinit var episodes: FakeEpisodeDao + private lateinit var subscriptions: FakeSubscriptionDao + + @Before + fun setUp() { + publisher = MockWebServer().apply { start() } + episodes = FakeEpisodeDao() + subscriptions = FakeSubscriptionDao().apply { + rows += SubscriptionEntity(id = "s1", feedUrl = "https://feed", title = "Show", addedAtMs = 0) + } + } + + @After + fun tearDown() = publisher.shutdown() + + private fun waiting(id: String, state: DownloadState = DownloadState.WAITING_FOR_SERVER) = EpisodeEntity( + id = id, + subscriptionId = "s1", + guid = id, + title = "Episode $id", + mediaUrl = publisher.url("/$id.mp3").toString(), + mediaType = "audio/mpeg", + discoveredAtMs = 0, + downloadState = state, + ) + + private class ScriptedServer(private val answers: MutableMap<String, ServerEpisode?>) : ServerAudio { + val recorded = mutableListOf<String>() + override suspend fun canAsk(episode: EpisodeEntity) = true + override suspend fun resolve(episode: EpisodeEntity) = answers[episode.id] + override suspend fun record(episode: EpisodeEntity, info: ServerEpisode) { + recorded += episode.id + } + } + + private fun readyAt(path: String) = ServerEpisode( + ready = true, audioUrl = publisher.url(path).toString(), + durationMs = null, introMs = 1, outroMs = 2, detected = true, + ) + + private fun realDownloader(audio: ServerAudio) = + EpisodeDownloader(OkHttpClient(), episodes, temp.newFolder("lib"), null, audio) { 5L } + + private fun retrier(downloader: EpisodeDownloader) = + WaitingDownloads(episodes, subscriptions) { episode, kind -> downloader.download(episode, kind) } + + @Test + fun `a waiting episode succeeds on retry once the server has it`() = runTest { + episodes.rows += waiting("e1") + publisher.enqueue(MockResponse().setBody("server-bytes")) + val audio = ScriptedServer(mutableMapOf("e1" to readyAt("/audio/e1"))) + + val done = retrier(realDownloader(audio)).retryAll() + + assertEquals(1, done) + val stored = episodes.rows.single() + assertEquals(DownloadState.DOWNLOADED, stored.downloadState) + assertTrue("the retry fetched the server's copy", stored.audioFromServer) + assertEquals(listOf("e1"), audio.recorded) + assertEquals("/audio/e1", publisher.takeRequest().path) + } + + @Test + fun `a still-pending episode stays waiting, is not failed, and the publisher is not contacted`() = runTest { + episodes.rows += waiting("e1") + val audio = ScriptedServer(mutableMapOf("e1" to null)) + + val done = retrier(realDownloader(audio)).retryAll() + + assertEquals(0, done) + assertEquals(DownloadState.WAITING_FOR_SERVER, episodes.rows.single().downloadState) + assertEquals(0, publisher.requestCount) + } + + @Test + fun `only waiting episodes are retried`() = runTest { + episodes.rows += waiting("w") + episodes.rows += waiting("none", DownloadState.NONE) + episodes.rows += waiting("failed", DownloadState.FAILED) + episodes.rows += waiting("done", DownloadState.DOWNLOADED) + val attempted = mutableListOf<String>() + + WaitingDownloads(episodes, subscriptions) { e, _ -> attempted += e.id; Result.success(File("x")) }.retryAll() + + assertEquals(listOf("w"), attempted) + } + + @Test + fun `retries each episode with its show's kind`() = runTest { + episodes.rows += waiting("w") + var kind: SubscriptionKind? = null + + WaitingDownloads(episodes, subscriptions) { _, k -> kind = k; Result.success(File("x")) }.retryAll() + + assertEquals(SubscriptionKind.PODCAST, kind) + } + + @Test + fun `an episode whose show is gone is skipped`() = runTest { + episodes.rows += waiting("w").copy(subscriptionId = "gone") + val attempted = mutableListOf<String>() + + WaitingDownloads(episodes, subscriptions) { e, _ -> attempted += e.id; Result.success(File("x")) }.retryAll() + + assertTrue(attempted.isEmpty()) + } + + @Test + fun `one episode's failure does not stop the others`() = runTest { + listOf("a", "bad", "c", "boom", "e").forEach { episodes.rows += waiting(it) } + val done = mutableListOf<String>() + + val count = WaitingDownloads(episodes, subscriptions) { e, _ -> + when (e.id) { + "bad" -> Result.failure(java.io.IOException("offline")) + "boom" -> error("unexpected") + else -> { done += e.id; Result.success(File("x")) } + } + }.retryAll() + + assertEquals(setOf("a", "c", "e"), done.toSet()) + assertEquals(3, count) + } + + @Test + fun `cancellation is not swallowed`() = runTest { + episodes.rows += waiting("a") + try { + WaitingDownloads(episodes, subscriptions) { _, _ -> throw CancellationException("stop") }.retryAll() + fail("a cancelled retry must propagate the cancellation") + } catch (_: CancellationException) { + // expected + } + } + + @OptIn(ExperimentalCoroutinesApi::class) + @Test + fun `a retry is bounded in both batch size and concurrency`() = runTest { + (1..30).forEach { episodes.rows += waiting("e$it") } + val gate = CompletableDeferred<Unit>() + val started = mutableListOf<String>() + + val job = launch { + WaitingDownloads(episodes, subscriptions) { e, _ -> + started += e.id + gate.await() + Result.success(File("x")) + }.retryAll() + } + runCurrent() + assertEquals("only a couple of downloads at once", WaitingDownloads.CONCURRENT, started.size) + + gate.complete(Unit) + job.join() + assertEquals("a single refresh retries a bounded batch", WaitingDownloads.MAX_PER_REFRESH, started.size) + assertFalse(started.isEmpty()) + } +} diff --git a/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt b/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt new file mode 100644 index 0000000..ed06106 --- /dev/null +++ b/app/src/test/java/org/terst/listen/download/WaitingForServerTextTest.kt @@ -0,0 +1,35 @@ +package org.terst.listen.download + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.terst.listen.data.DownloadState +import org.terst.listen.data.EpisodeEntity + +class WaitingForServerTextTest { + + private fun waitingEpisode() = EpisodeEntity( + id = "e1", subscriptionId = "s1", guid = "g", title = "One", mediaUrl = "https://cdn/1.mp3", + mediaType = "audio/mpeg", discoveredAtMs = 0, downloadState = DownloadState.WAITING_FOR_SERVER, + ) + + @Test + fun theWaitingWordingPromisesARetry() { + assertEquals("Waiting for the server's copy; will retry on the next refresh", WaitingForServerText.STATUS) + assertEquals("waiting for server", WaitingForServerText.ROW_LABEL) + } + + @Test + fun thePublisherChoiceStatesTheTradeAndIsOfferedOnlyWhileWaiting() { + val offer = WaitingForServerText.publisherOffer(waitingEpisode())!! + + assertEquals("Download from publisher instead", offer.label) + assertTrue(offer.warning, offer.warning.contains("can differ")) + assertTrue(offer.warning, offer.warning.contains("will not apply")) + + DownloadState.entries.filter { it != DownloadState.WAITING_FOR_SERVER }.forEach { + assertNull("no offer in $it", WaitingForServerText.publisherOffer(waitingEpisode().copy(downloadState = it))) + } + } +} 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 38541a7..d550c69 100644 --- a/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt +++ b/app/src/test/java/org/terst/listen/fingerprint/SkipSummaryTest.kt @@ -1,7 +1,9 @@ package org.terst.listen.fingerprint +import org.junit.Assert.assertTrue import org.junit.Assert.assertEquals import org.junit.Test +import org.terst.listen.download.WaitingForServerText import org.terst.listen.data.DownloadState import org.terst.listen.data.EpisodeEntity import org.terst.listen.data.EpisodeMidrollEntity @@ -469,4 +471,18 @@ class SkipSummaryTest { assertEquals("intro", got[0].kind) assertEquals("ad", got[1].kind) } + + // ---- waiting for the server's copy --------------------------------------------------- + + @Test + fun aWaitingEpisodeSaysItIsWaitingRatherThanNotCheckedYet() { + val waiting = episodeRow().copy(downloadState = DownloadState.WAITING_FOR_SERVER) + + val skips = SkipSummary.episode(waiting, showRow(), ranges = emptyList()) + + assertEquals( + listOf(SkipSummary.BreakLine("Breaks", WaitingForServerText.STATUS)), + skips.breaks, + ) + } } diff --git a/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt b/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt index ae933dc..5d069df 100644 --- a/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt +++ b/app/src/test/java/org/terst/listen/server/ServerSkipsTest.kt @@ -252,4 +252,26 @@ class ServerSkipsTest { assertEquals("intro", ranges[0].kind) assertEquals("cue", ranges[1].kind) } + + @Test + fun `can ask when a server is configured and the show is known`() = runTest { + assertTrue(subject(StubTransport(null)).canAsk(episode)) + } + + @Test + fun `cannot ask when the show's feed is unknown`() = runTest { + assertEquals(false, subject(StubTransport(null), subscriptions = FakeSubscriptionDao()).canAsk(episode)) + } + + @Test + fun `cannot ask when no server is configured`() = runTest { + val unconfigured = ServerSkips( + SkipServer({ null }, StubTransport(null)), + FakeSubscriptionDao().apply { rows += show }, + FakeShowFingerprintDao(), + FakePlaybackStateDao(FakeEpisodeDao().rows), + FakeMidrollDao(), + ) + assertEquals(false, unconfigured.canAsk(episode)) + } } diff --git a/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt b/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt index 1858120..ebeec3f 100644 --- a/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt +++ b/app/src/test/java/org/terst/listen/server/SkipRefresherTest.kt @@ -65,6 +65,8 @@ class SkipRefresherTest { val resolved = mutableListOf<String>() val recorded = mutableListOf<Pair<String, ServerEpisode>>() + override suspend fun canAsk(episode: EpisodeEntity) = true + override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { resolved += episode.id check(episode.id !in failing) { "server unreachable: ${episode.id}" } @@ -168,6 +170,8 @@ class SkipRefresherTest { val stuck = CompletableDeferred<ServerEpisode?>() val started = mutableListOf<String>() val audio = object : ServerAudio { + override suspend fun canAsk(episode: EpisodeEntity) = true + override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { started += episode.id return stuck.await() @@ -203,6 +207,8 @@ class SkipRefresherTest { // propagating it. val dao = FakeEpisodeDao().apply { rows += episode("e1") } val audio = object : ServerAudio { + override suspend fun canAsk(episode: EpisodeEntity) = true + override suspend fun resolve(episode: EpisodeEntity): ServerEpisode? { throw CancellationException("cancelled mid-query") } diff --git a/app/src/test/java/org/terst/listen/server/SkipServerTest.kt b/app/src/test/java/org/terst/listen/server/SkipServerTest.kt index 4c48600..32d13e0 100644 --- a/app/src/test/java/org/terst/listen/server/SkipServerTest.kt +++ b/app/src/test/java/org/terst/listen/server/SkipServerTest.kt @@ -302,4 +302,13 @@ class SkipServerTest { assertEquals("ad", got?.breaks?.single()?.kind) } + + @Test + fun `knows whether a server is configured`() = runTest { + val transport = FakeTransport() + assertTrue(server(transport).isConfigured()) + assertEquals(false, server(transport, baseUrl = null).isConfigured()) + assertEquals(false, server(transport, token = null).isConfigured()) + assertTrue("asking must not touch the network", transport.gets.isEmpty()) + } } |