From 31c1fec322fe31ac6a2ebd9573601d7cc5f9a8fc Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 06:12:04 +0000 Subject: [PATCH] fix downloaded playback and tighten the download state machine Review findings on the first downloads block: - ExoPlayer could not open a downloaded file at all: the service's data source factory was OkHttp only, so a file:// URI went to the HTTP stack and failed with a malformed URL. Wrap it in DefaultDataSource so the file, asset and content schemes are resolved before the stream chain, and cover it with a test that reads a local file through the production factory. - Reconcile no longer restarts a failed download, or a running one whose engine entry vanished: only a queued row is started again, and the rest become failed so the explicit Retry in the UI means something. - song_downloads also references songs with CASCADE now, so a song pruned from the mirror takes its download row with it and the sweep can reclaim the file. - The mutating methods run on the injected dispatcher rather than the caller's, which was the main thread for every call site, and the repository exposes one shared state flow instead of a second map plus five subscriptions. - Playback stops when the active source disappears, including deleting the last source while a downloaded track is playing. - Dropped the unused request mime type, the queued-at column nothing read, the metered constructor parameter nobody varied, and a dead index guard. - The running indicator is asserted positively, the engine's status and reason mapping has a test, and the failure paths use the fake's failure and forget helpers. --- .../18.json | 26 +-- .../com/subtracks/data/db/Migrations.kt | 3 +- .../subtracks/data/download/DownloadEngine.kt | 1 - .../data/download/SystemDownloadEngine.kt | 44 ++--- .../com/subtracks/data/model/Download.kt | 7 +- .../subtracks/data/repo/DownloadRepository.kt | 164 ++++++++++-------- .../subtracks/playback/PlaybackController.kt | 6 +- .../com/subtracks/playback/PlaybackService.kt | 21 ++- .../ui/library/AlbumDetailViewModel.kt | 5 +- .../ui/library/PlaylistDetailViewModel.kt | 4 +- .../com/subtracks/ui/library/SongsContent.kt | 13 +- .../subtracks/ui/playback/NowPlayingScreen.kt | 2 +- .../com/subtracks/ui/playback/QueueScreen.kt | 2 +- .../data/download/SystemDownloadEngineTest.kt | 31 ++++ .../data/repo/DownloadRepositoryTest.kt | 56 ++++++ .../subtracks/playback/MediaDataSourceTest.kt | 32 ++++ .../ui/library/SongRowDownloadTest.kt | 3 +- 17 files changed, 285 insertions(+), 135 deletions(-) create mode 100644 app/src/test/kotlin/com/subtracks/data/download/SystemDownloadEngineTest.kt create mode 100644 app/src/test/kotlin/com/subtracks/playback/MediaDataSourceTest.kt diff --git a/app/schemas/com.subtracks.data.db.SubtracksDatabase/18.json b/app/schemas/com.subtracks.data.db.SubtracksDatabase/18.json index 793cbf84..1276b6d3 100644 --- a/app/schemas/com.subtracks.data.db.SubtracksDatabase/18.json +++ b/app/schemas/com.subtracks.data.db.SubtracksDatabase/18.json @@ -2,7 +2,7 @@ "formatVersion": 1, "database": { "version": 18, - "identityHash": "5af02d01d5774106a81b2d031b6bd215", + "identityHash": "982f1044d9532868990a4e14017ad3d1", "entities": [ { "tableName": "sources", @@ -1105,7 +1105,7 @@ }, { "tableName": "song_downloads", - "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`sourceId` INTEGER NOT NULL, `songId` TEXT NOT NULL, `status` TEXT NOT NULL, `engineId` INTEGER, `bytes` INTEGER NOT NULL DEFAULT 0, `total` INTEGER NOT NULL DEFAULT 0, `error` TEXT, `queuedAt` INTEGER NOT NULL DEFAULT 0, PRIMARY KEY(`sourceId`, `songId`), FOREIGN KEY(`sourceId`) REFERENCES `sources`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`sourceId` INTEGER NOT NULL, `songId` TEXT NOT NULL, `status` TEXT NOT NULL, `engineId` INTEGER, `bytes` INTEGER NOT NULL DEFAULT 0, `total` INTEGER NOT NULL DEFAULT 0, `error` TEXT, PRIMARY KEY(`sourceId`, `songId`), FOREIGN KEY(`sourceId`) REFERENCES `sources`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE , FOREIGN KEY(`sourceId`, `songId`) REFERENCES `songs`(`sourceId`, `id`) ON UPDATE NO ACTION ON DELETE CASCADE )", "fields": [ { "fieldPath": "sourceId", @@ -1148,13 +1148,6 @@ "fieldPath": "error", "columnName": "error", "affinity": "TEXT" - }, - { - "fieldPath": "queuedAt", - "columnName": "queuedAt", - "affinity": "INTEGER", - "notNull": true, - "defaultValue": "0" } ], "primaryKey": { @@ -1195,13 +1188,26 @@ "referencedColumns": [ "id" ] + }, + { + "table": "songs", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "sourceId", + "songId" + ], + "referencedColumns": [ + "sourceId", + "id" + ] } ] } ], "setupQueries": [ "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", - "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '5af02d01d5774106a81b2d031b6bd215')" + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, '982f1044d9532868990a4e14017ad3d1')" ] } } \ No newline at end of file diff --git a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt index 7a0b18b3..bb700fe9 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/Migrations.kt @@ -249,8 +249,9 @@ val MIGRATION_17_18 = connection.execSQL( "CREATE TABLE IF NOT EXISTS `song_downloads` (`sourceId` INTEGER NOT NULL, `songId` TEXT NOT NULL, " + "`status` TEXT NOT NULL, `engineId` INTEGER, `bytes` INTEGER NOT NULL DEFAULT 0, " + - "`total` INTEGER NOT NULL DEFAULT 0, `error` TEXT, `queuedAt` INTEGER NOT NULL DEFAULT 0, " + + "`total` INTEGER NOT NULL DEFAULT 0, `error` TEXT, " + "PRIMARY KEY(`sourceId`, `songId`), FOREIGN KEY(`sourceId`) REFERENCES `sources`(`id`) " + + "ON UPDATE NO ACTION ON DELETE CASCADE, FOREIGN KEY(`sourceId`, `songId`) REFERENCES `songs`(`sourceId`, `id`) " + "ON UPDATE NO ACTION ON DELETE CASCADE )", ) connection.execSQL("CREATE INDEX IF NOT EXISTS `index_song_downloads_sourceId` ON `song_downloads` (`sourceId`)") diff --git a/app/src/main/kotlin/com/subtracks/data/download/DownloadEngine.kt b/app/src/main/kotlin/com/subtracks/data/download/DownloadEngine.kt index 0ab65275..32d59e2b 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/DownloadEngine.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/DownloadEngine.kt @@ -4,7 +4,6 @@ data class EngineRequest( val uri: String, val path: String, val title: String, - val mimeType: String? = null, val allowMetered: Boolean = false, ) diff --git a/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt b/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt index 809090b3..9915d002 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt @@ -9,15 +9,14 @@ class SystemDownloadEngine( private val context: Context, ) : DownloadEngine { override fun enqueue(request: EngineRequest): Long { - val builder = + val download = DownloadManager .Request(Uri.parse(request.uri)) .setTitle(request.title) .setDestinationInExternalFilesDir(context, Environment.DIRECTORY_MUSIC, request.path) .setNotificationVisibility(DownloadManager.Request.VISIBILITY_VISIBLE) .setAllowedOverMetered(request.allowMetered) - request.mimeType?.let(builder::setMimeType) - return manager().enqueue(builder) + return manager().enqueue(download) } override fun download(id: Long): EngineDownload? { @@ -31,15 +30,8 @@ class SystemDownloadEngine( cursor .getInt(cursor.getColumnIndexOrThrow(DownloadManager.COLUMN_REASON)) .takeIf { status == DownloadManager.STATUS_FAILED } - val mapped = - when (status) { - DownloadManager.STATUS_SUCCESSFUL -> EngineStatus.Completed - DownloadManager.STATUS_FAILED -> EngineStatus.Failed - DownloadManager.STATUS_RUNNING -> EngineStatus.Running - else -> EngineStatus.Pending - } return EngineDownload( - status = mapped, + status = engineStatus(status), bytes = bytes.coerceAtLeast(0), total = total.coerceAtLeast(0), error = reason?.let(::failureMessage), @@ -52,15 +44,23 @@ class SystemDownloadEngine( } private fun manager(): DownloadManager = context.getSystemService(DownloadManager::class.java) - - private fun failureMessage(reason: Int): String = - when (reason) { - DownloadManager.ERROR_INSUFFICIENT_SPACE -> "Not enough space to download" - DownloadManager.ERROR_DEVICE_NOT_FOUND -> "Storage is unavailable" - DownloadManager.ERROR_FILE_ERROR, DownloadManager.ERROR_FILE_ALREADY_EXISTS -> "Could not write the download" - DownloadManager.ERROR_HTTP_DATA_ERROR, DownloadManager.ERROR_UNHANDLED_HTTP_CODE -> "The server refused to send this track" - DownloadManager.ERROR_TOO_MANY_REDIRECTS -> "The server redirected too many times" - DownloadManager.ERROR_CANNOT_RESUME -> "The download could not be resumed" - else -> "Download failed" - } } + +internal fun engineStatus(status: Int): EngineStatus = + when (status) { + DownloadManager.STATUS_SUCCESSFUL -> EngineStatus.Completed + DownloadManager.STATUS_FAILED -> EngineStatus.Failed + DownloadManager.STATUS_RUNNING -> EngineStatus.Running + else -> EngineStatus.Pending + } + +internal fun failureMessage(reason: Int): String = + when (reason) { + DownloadManager.ERROR_INSUFFICIENT_SPACE -> "Not enough space to download" + DownloadManager.ERROR_DEVICE_NOT_FOUND -> "Storage is unavailable" + DownloadManager.ERROR_FILE_ERROR, DownloadManager.ERROR_FILE_ALREADY_EXISTS -> "Could not write the download" + DownloadManager.ERROR_HTTP_DATA_ERROR, DownloadManager.ERROR_UNHANDLED_HTTP_CODE -> "The server refused to send this track" + DownloadManager.ERROR_TOO_MANY_REDIRECTS -> "The server redirected too many times" + DownloadManager.ERROR_CANNOT_RESUME -> "The download could not be resumed" + else -> "Download failed" + } diff --git a/app/src/main/kotlin/com/subtracks/data/model/Download.kt b/app/src/main/kotlin/com/subtracks/data/model/Download.kt index 799586c9..d9cb3eba 100644 --- a/app/src/main/kotlin/com/subtracks/data/model/Download.kt +++ b/app/src/main/kotlin/com/subtracks/data/model/Download.kt @@ -18,6 +18,12 @@ enum class DownloadStatus { Queued, Running, Completed, Failed } childColumns = ["sourceId"], onDelete = ForeignKey.CASCADE, ), + ForeignKey( + entity = Song::class, + parentColumns = ["sourceId", "id"], + childColumns = ["sourceId", "songId"], + onDelete = ForeignKey.CASCADE, + ), ], indices = [Index("sourceId"), Index("engineId")], ) @@ -29,7 +35,6 @@ data class SongDownload( @ColumnInfo(defaultValue = "0") val bytes: Long = 0, @ColumnInfo(defaultValue = "0") val total: Long = 0, val error: String? = null, - @ColumnInfo(defaultValue = "0") val queuedAt: Long = 0, ) { val progress: Float? get() = total.takeIf { it > 0 }?.let { (bytes.toFloat() / it).coerceIn(0f, 1f) } diff --git a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt index e8208013..6f6d50ba 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -17,14 +17,18 @@ import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.withContext import java.io.File class DownloadRepository( @@ -32,9 +36,8 @@ class DownloadRepository( private val sourceRepository: SourceRepository, private val engine: DownloadEngine, private val downloadsDir: File, - private val allowMetered: Boolean = false, private val showMessage: (String) -> Unit = {}, - dispatcher: CoroutineDispatcher = Dispatchers.IO, + private val dispatcher: CoroutineDispatcher = Dispatchers.IO, ) { private val scope = CoroutineScope(SupervisorJob() + dispatcher) private val mutex = Mutex() @@ -44,94 +47,106 @@ class DownloadRepository( @Volatile private var started = false - private val completedFiles = MutableStateFlow>(emptyMap()) + private val sharedStates: StateFlow> by lazy { + downloadStates() + .onEach { loaded.complete(Unit) } + .stateIn(scope, SharingStarted.Eagerly, emptyMap()) + } fun start() { if (started) return started = true downloadsDir.mkdirs() File(downloadsDir, NO_MEDIA).createNewFile() + sharedStates scope.launch { sourceRepository.activeSourceId().distinctUntilChanged().collect { reconcile() } } - scope.launch { - states().collect { rows -> - completedFiles.value = - rows - .filterValues { it.status == DownloadStatus.Completed } - .mapValues { (_, row) -> file(row.sourceId, row.songId) } - .filterValues { it.exists() } - loaded.complete(Unit) - } - } } fun close() { scope.cancel() } - fun states(): Flow> = - sourceRepository - .activeSourceId() - .flatMapLatest { sourceId -> if (sourceId == null) flowOf(emptyList()) else db.downloadDao().downloads(sourceId) } - .map { rows -> rows.associateBy { it.songId } } + fun states(): StateFlow> = sharedStates suspend fun awaitLoaded() { - if (started) loaded.await() + if (!started) return + states() + loaded.await() } - fun localUri(songId: String): String? = completedFiles.value[songId]?.takeIf { it.exists() }?.let { Uri.fromFile(it).toString() } + fun localUri(songId: String): String? = + sharedStates.value[songId] + ?.takeIf { it.status == DownloadStatus.Completed } + ?.let { file(it.sourceId, it.songId) } + ?.takeIf { it.exists() } + ?.let { Uri.fromFile(it).toString() } suspend fun download( sourceId: Long, songId: String, - ) = mutex.withLock { - if (sourceRepository.activeSourceIdOnce() != sourceId) { - showMessage("Can't download from a source that isn't active") - return@withLock + ) = withContext(dispatcher) { + mutex.withLock { + if (sourceRepository.activeSourceIdOnce() != sourceId) { + showMessage("Can't download from a source that isn't active") + return@withLock + } + val existing = db.downloadDao().find(sourceId, songId) + if (existing != null && existing.status != DownloadStatus.Failed) return@withLock + existing?.engineId?.let(engine::cancel) + val url = sourceRepository.downloadUri(songId) + if (url == null) { + showMessage("Can't download: the server address is unavailable") + return@withLock + } + enqueue(sourceId, songId, url) } - val existing = db.downloadDao().find(sourceId, songId) - if (existing != null && existing.status != DownloadStatus.Failed) return@withLock - existing?.engineId?.let(engine::cancel) - val url = sourceRepository.downloadUri(songId) - if (url == null) { - showMessage("Can't download: the server address is unavailable") - return@withLock - } - enqueue(sourceId, songId, url) } suspend fun remove( sourceId: Long, songId: String, - ) = mutex.withLock { - val row = db.downloadDao().find(sourceId, songId) ?: return@withLock - row.engineId?.let(engine::cancel) - file(sourceId, songId).delete() - db.downloadDao().delete(sourceId, songId) + ) = withContext(dispatcher) { + mutex.withLock { + val row = db.downloadDao().find(sourceId, songId) ?: return@withLock + row.engineId?.let(engine::cancel) + file(sourceId, songId).delete() + db.downloadDao().delete(sourceId, songId) + } } suspend fun removeSource(sourceId: Long) = - mutex.withLock { - db - .downloadDao() - .all() - .filter { it.sourceId == sourceId } - .forEach { row -> row.engineId?.let(engine::cancel) } - db.downloadDao().deleteSource(sourceId) - dir(sourceId).deleteRecursively() + withContext(dispatcher) { + mutex.withLock { + db + .downloadDao() + .all() + .filter { it.sourceId == sourceId } + .forEach { row -> row.engineId?.let(engine::cancel) } + db.downloadDao().deleteSource(sourceId) + dir(sourceId).deleteRecursively() + } } suspend fun reconcile() = - mutex.withLock { - db.downloadDao().all().forEach { row -> - val state = row.engineId?.let(engine::download) - if (state == null) recover(row) else apply(row, state) + withContext(dispatcher) { + mutex.withLock { + db.downloadDao().all().forEach { row -> + val state = row.engineId?.let(engine::download) + if (state == null) recover(row) else apply(row, state) + } + sweep(db.downloadDao().all()) + ensurePolling() } - sweep(db.downloadDao().all()) - ensurePolling() } + private fun downloadStates(): Flow> = + sourceRepository + .activeSourceId() + .flatMapLatest { sourceId -> if (sourceId == null) flowOf(emptyList()) else db.downloadDao().downloads(sourceId) } + .map { rows -> rows.associateBy { it.songId } } + private suspend fun enqueue( sourceId: Long, songId: String, @@ -142,40 +157,32 @@ class DownloadRepository( val title = db.libraryDao().songOnce(sourceId, songId)?.title ?: songId val engineId = engine.enqueue( - EngineRequest(uri = url, path = enginePath(sourceId, songId), title = title, allowMetered = allowMetered), + EngineRequest(uri = url, path = enginePath(sourceId, songId), title = title, allowMetered = false), ) db.downloadDao().upsert( - SongDownload( - sourceId = sourceId, - songId = songId, - status = DownloadStatus.Queued, - engineId = engineId, - queuedAt = System.currentTimeMillis(), - ), + SongDownload(sourceId = sourceId, songId = songId, status = DownloadStatus.Queued, engineId = engineId), ) ensurePolling() } private suspend fun recover(row: SongDownload) { - when { - row.status == DownloadStatus.Completed && file(row.sourceId, row.songId).exists() -> { - Unit + when (row.status) { + DownloadStatus.Completed -> { + if (!file(row.sourceId, row.songId).exists()) markFailed(row, MISSING_FILE) } - row.status == DownloadStatus.Completed -> { - markFailed(row, "The downloaded file is missing") - } - - sourceRepository.activeSourceIdOnce() != row.sourceId -> { - Unit - } - - else -> { + DownloadStatus.Queued -> { + if (sourceRepository.activeSourceIdOnce() != row.sourceId) return val url = sourceRepository.downloadUri(row.songId) - when (url) { - null -> markFailed(row, "Can't download: the server address is unavailable") - else -> enqueue(row.sourceId, row.songId, url) - } + if (url == null) markFailed(row, NO_ADDRESS) else enqueue(row.sourceId, row.songId, url) + } + + DownloadStatus.Running -> { + markFailed(row, STOPPED) + } + + DownloadStatus.Failed -> { + Unit } } } @@ -257,5 +264,8 @@ class DownloadRepository( const val ENGINE_DIR = "downloads" const val NO_MEDIA = ".nomedia" const val POLL_MS = 1_000L + const val MISSING_FILE = "The downloaded file is missing" + const val NO_ADDRESS = "Can't download: the server address is unavailable" + const val STOPPED = "The download stopped unexpectedly" } } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index e528432b..8daf7ad0 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -144,7 +144,7 @@ class PlaybackController( init { scope.launch { sourceRepository.activeSourceId().collect { sourceId -> - if (queueSourceId != null && sourceId != null && sourceId != queueSourceId) stop() + if (queueSourceId != null && sourceId != queueSourceId) stop() } } scope.launch { @@ -965,10 +965,8 @@ class PlaybackController( val snapshot = snapshot ?: return if (snapshot.size == 0L) return if (player.isEnded) return - val index = player.currentIndex - val position = (windowStart + index).coerceIn(0, snapshot.size - 1) + val position = (windowStart + player.currentIndex).coerceIn(0, snapshot.size - 1) val positionMs = player.currentPositionMs - if (player.currentIndex != index) return loadWindow(position, autoplay = player.playWhenReady, startPositionMs = positionMs) } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt index f25a6f94..1cd8fa73 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackService.kt @@ -1,6 +1,7 @@ package com.subtracks.playback import android.app.PendingIntent +import android.content.Context import android.content.Intent import android.os.SystemClock import android.util.Log @@ -9,6 +10,8 @@ import androidx.media3.common.AudioAttributes import androidx.media3.common.C import androidx.media3.common.util.ExperimentalApi import androidx.media3.common.util.UnstableApi +import androidx.media3.datasource.DataSource +import androidx.media3.datasource.DefaultDataSource import androidx.media3.datasource.okhttp.OkHttpDataSource import androidx.media3.exoplayer.ExoPlayer import androidx.media3.exoplayer.source.DefaultMediaSourceFactory @@ -41,9 +44,7 @@ class PlaybackService : MediaSessionService() { .setMediaSourceFactory( DefaultMediaSourceFactory(this) .setEnableClippingInMediaPeriod(true) - .setDataSourceFactory( - KnownLengthDataSourceFactory(OkHttpDataSource.Factory(streamingClient())), - ), + .setDataSourceFactory(mediaDataSourceFactory(this, OkHttpDataSource.Factory(streamingClient()))), ).build() val loader = CoverArtBitmapLoader( @@ -88,10 +89,6 @@ class PlaybackService : MediaSessionService() { response }.build() - private companion object { - const val STREAM_TAG = "SubtracksPlayback" - } - override fun onDestroy() { artworkLoader?.shutdown() artworkLoader = null @@ -102,4 +99,14 @@ class PlaybackService : MediaSessionService() { session = null super.onDestroy() } + + private companion object { + const val STREAM_TAG = "SubtracksPlayback" + } } + +@OptIn(UnstableApi::class) +internal fun mediaDataSourceFactory( + context: Context, + upstream: DataSource.Factory, +): DataSource.Factory = DefaultDataSource.Factory(context, KnownLengthDataSourceFactory(upstream)) diff --git a/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailViewModel.kt b/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailViewModel.kt index 9934a784..a73150c5 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailViewModel.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailViewModel.kt @@ -13,12 +13,10 @@ import com.subtracks.data.repo.SourceRepository import com.subtracks.playback.PlaybackController import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapLatest -import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch @OptIn(ExperimentalCoroutinesApi::class) @@ -37,8 +35,7 @@ class AlbumDetailViewModel( val discs: Flow> = sourceId.flatMapLatest { libraryRepository.albumDiscs(it, albumId) } - val downloads: StateFlow> = - downloadRepository.states().stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyMap()) + val downloads: StateFlow> = downloadRepository.states() fun coverArt( coverArt: String?, diff --git a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailViewModel.kt b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailViewModel.kt index e8441400..0da0253f 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailViewModel.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailViewModel.kt @@ -15,12 +15,10 @@ import com.subtracks.data.repo.SourceRepository import com.subtracks.playback.PlaybackController import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.filterNotNull import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapLatest -import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch @OptIn(ExperimentalCoroutinesApi::class) @@ -41,7 +39,7 @@ class PlaylistDetailViewModel( .cachedIn(viewModelScope) val downloads: StateFlow> = - downloadRepository.states().stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyMap()) + downloadRepository.states() fun download(song: Song) { viewModelScope.launch { downloadRepository.download(song.sourceId, song.id) } diff --git a/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt b/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt index bb984354..854b78c1 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/SongsContent.kt @@ -20,6 +20,8 @@ import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip import androidx.compose.ui.graphics.Color +import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.semantics.semantics import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp @@ -117,9 +119,16 @@ private fun DownloadBadge(download: SongDownload?) { DownloadStatus.Queued, DownloadStatus.Running -> { val progress = download.progress if (progress == null) { - CircularProgressIndicator(modifier = Modifier.size(14.dp), strokeWidth = 2.dp) + CircularProgressIndicator( + modifier = Modifier.size(14.dp).semantics { contentDescription = "Downloading" }, + strokeWidth = 2.dp, + ) } else { - CircularProgressIndicator(progress = { progress }, modifier = Modifier.size(14.dp), strokeWidth = 2.dp) + CircularProgressIndicator( + progress = { progress }, + modifier = Modifier.size(14.dp).semantics { contentDescription = "Downloading" }, + strokeWidth = 2.dp, + ) } } diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt index b6c39bba..26c0510a 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/NowPlayingScreen.kt @@ -104,7 +104,7 @@ fun NowPlayingRoute( val positionMs by controller.positionMs.collectAsStateWithLifecycle() val sourceId = state.context?.sourceId val songId = state.item?.id - val downloads by downloadRepository.states().collectAsStateWithLifecycle(initialValue = emptyMap()) + val downloads by downloadRepository.states().collectAsStateWithLifecycle() val download = songId?.let { downloads[it] } val scope = rememberCoroutineScope() val song by diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt index ce0afa09..d5e5f6c6 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -279,7 +279,7 @@ fun QueueRoute( downloadRepository: DownloadRepository = koinInject(), ) { val playback by controller.state.collectAsStateWithLifecycle() - val downloads by downloadRepository.states().collectAsStateWithLifecycle(initialValue = emptyMap()) + val downloads by downloadRepository.states().collectAsStateWithLifecycle() val queueContext = playback.context // The item's album keeps the header stable while the async name lookup runs, so it does not // flash the fallback when tapping between context and manually queued tracks. diff --git a/app/src/test/kotlin/com/subtracks/data/download/SystemDownloadEngineTest.kt b/app/src/test/kotlin/com/subtracks/data/download/SystemDownloadEngineTest.kt new file mode 100644 index 00000000..bf478e81 --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/data/download/SystemDownloadEngineTest.kt @@ -0,0 +1,31 @@ +package com.subtracks.data.download + +import android.app.DownloadManager +import androidx.test.ext.junit.runners.AndroidJUnit4 +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith + +@RunWith(AndroidJUnit4::class) +class SystemDownloadEngineTest { + @Test + fun successfulRunningAndFailedStatusesAreMapped() { + assertEquals(EngineStatus.Completed, engineStatus(DownloadManager.STATUS_SUCCESSFUL)) + assertEquals(EngineStatus.Running, engineStatus(DownloadManager.STATUS_RUNNING)) + assertEquals(EngineStatus.Failed, engineStatus(DownloadManager.STATUS_FAILED)) + } + + @Test + fun aWaitingDownloadIsPending() { + assertEquals(EngineStatus.Pending, engineStatus(DownloadManager.STATUS_PENDING)) + assertEquals(EngineStatus.Pending, engineStatus(DownloadManager.STATUS_PAUSED)) + assertEquals(EngineStatus.Pending, engineStatus(Int.MIN_VALUE)) + } + + @Test + fun failureReasonsReadAsSentences() { + assertEquals("Not enough space to download", failureMessage(DownloadManager.ERROR_INSUFFICIENT_SPACE)) + assertEquals("Storage is unavailable", failureMessage(DownloadManager.ERROR_DEVICE_NOT_FOUND)) + assertEquals("Download failed", failureMessage(DownloadManager.ERROR_UNKNOWN)) + } +} diff --git a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt index 31fee6b8..d859cd95 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -173,6 +173,62 @@ class DownloadRepositoryTest { assertEquals(DownloadStatus.Queued, row(1, "s1")?.status) } + @Test + fun aFailedDownloadIsNotRestartedByReconcile() = + runTest { + seedLibrary() + db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Failed, engineId = null, error = "boom")) + + runBlocking { repository.reconcile() } + runBlocking { repository.reconcile() } + + assertTrue(engine.requests.isEmpty()) + assertEquals(DownloadStatus.Failed, row(1, "s1")?.status) + assertEquals("boom", row(1, "s1")?.error) + } + + @Test + fun anEngineFailureIsMirroredWithItsMessage() = + runTest { + seedLibrary() + runBlocking { repository.download(1, "s1") } + val id = engine.requests.single().first + + engine.fail(id) + runBlocking { repository.reconcile() } + + assertEquals(DownloadStatus.Failed, row(1, "s1")?.status) + assertEquals("Download failed", row(1, "s1")?.error) + } + + @Test + fun aRunningDownloadWhoseEngineEntryVanishesIsFailedRatherThanRestarted() = + runTest { + seedLibrary() + repository.start() + runBlocking { repository.download(1, "s1") } + val id = engine.requests.single().first + engine.running(id, bytes = 40, total = 100) + await { row(1, "s1")?.status == DownloadStatus.Running } + + engine.forget(id) + runBlocking { repository.reconcile() } + + assertEquals(DownloadStatus.Failed, row(1, "s1")?.status) + assertEquals(1, engine.requests.size) + } + + @Test + fun aSongRemovedFromTheServerTakesItsDownloadRowWithIt() = + runTest { + seedLibrary() + runBlocking { repository.download(1, "s1") } + + runBlocking { db.libraryDao().deleteSongs(1, listOf("s1")) } + + assertNull(row(1, "s1")) + } + @Test fun downloadingFromASourceThatIsNotActiveIsRefused() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/playback/MediaDataSourceTest.kt b/app/src/test/kotlin/com/subtracks/playback/MediaDataSourceTest.kt new file mode 100644 index 00000000..250b149c --- /dev/null +++ b/app/src/test/kotlin/com/subtracks/playback/MediaDataSourceTest.kt @@ -0,0 +1,32 @@ +package com.subtracks.playback + +import android.content.Context +import android.net.Uri +import androidx.media3.datasource.DataSpec +import androidx.media3.datasource.okhttp.OkHttpDataSource +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import okhttp3.OkHttpClient +import org.junit.Assert.assertEquals +import org.junit.Test +import org.junit.runner.RunWith +import java.io.File + +@RunWith(AndroidJUnit4::class) +class MediaDataSourceTest { + @Test + fun aDownloadedFileIsReadThroughTheStreamingChain() { + val context = ApplicationProvider.getApplicationContext() + val file = File(context.cacheDir, "downloaded-${System.nanoTime()}").apply { writeBytes(byteArrayOf(1, 2, 3, 4)) } + val dataSource = mediaDataSourceFactory(context, OkHttpDataSource.Factory(OkHttpClient())).createDataSource() + + val length = dataSource.open(DataSpec(Uri.fromFile(file))) + val buffer = ByteArray(4) + val read = dataSource.read(buffer, 0, buffer.size) + dataSource.close() + + assertEquals(4L, length) + assertEquals(4, read) + assertEquals(listOf(1, 2, 3, 4), buffer.toList()) + } +} diff --git a/app/src/test/kotlin/com/subtracks/ui/library/SongRowDownloadTest.kt b/app/src/test/kotlin/com/subtracks/ui/library/SongRowDownloadTest.kt index d90482e5..cfb9eed0 100644 --- a/app/src/test/kotlin/com/subtracks/ui/library/SongRowDownloadTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/library/SongRowDownloadTest.kt @@ -45,10 +45,11 @@ class SongRowDownloadTest { } @Test - fun aRunningDownloadShowsNoDownloadedIndicator() { + fun aRunningDownloadShowsProgressRatherThanTheDownloadedIndicator() { render(download(DownloadStatus.Running)) composeRule.onNodeWithContentDescription("Downloaded").assertDoesNotExist() + composeRule.onNodeWithContentDescription("Downloading").assertIsDisplayed() } @Test