address the bulk download review

- Cancelling a list while its download loop was still enqueueing missed every
  song queued after the cancel looked. The loop now holds the lock for the whole
  queue, so a cancel waits for it to finish and then removes all of it. That
  needed the enqueue helper split into a locked entry point and a body the loop
  can call while already holding the lock — calling the locked one from inside
  the lock deadlocked, which is what hung the tests.
- An empty resolved list now says so instead of doing nothing.
- One song failing to enqueue no longer aborts the rest of the batch.
- The artist actions resolve through the albums the artist page lists, matching
  what is on screen, rather than through songs.artistId, which for a guest on a
  compilation enqueued nothing.
- Bulk actions are one `applyAction` on the repository, so the three view models
  and both screens stop repeating the same dispatch.
- Tests: the playlist and artist resolve paths, a Running row counted as
  downloading, the action dispatch, and the action rule moved to a plain unit
  test.
This commit is contained in:
austinried
2026-10-04 00:30:02 +00:00
parent b2b92a388e
commit bf561075a2
15 changed files with 178 additions and 99 deletions
@@ -16,23 +16,18 @@ interface DownloadDao {
@Query("SELECT * FROM song_downloads")
suspend fun all(): List<SongDownload>
@Query("SELECT id FROM songs WHERE sourceId = :sourceId AND artistId = :artistId ORDER BY albumId, disc, track, id")
suspend fun artistSongIds(
sourceId: Long,
artistId: String,
): List<String>
@Query(
"SELECT COUNT(songs.id) AS total, " +
"COALESCE(SUM(CASE WHEN song_downloads.status = 'Completed' THEN 1 ELSE 0 END), 0) AS downloaded, " +
"COALESCE(SUM(CASE WHEN song_downloads.status IN ('Queued', 'Running') THEN 1 ELSE 0 END), 0) AS downloading " +
"FROM songs " +
"LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " +
"WHERE songs.sourceId = :sourceId AND songs.albumId = :albumId",
"WHERE songs.sourceId = :sourceId " +
"AND songs.albumId IN (SELECT id FROM albums WHERE sourceId = :sourceId AND artistId = :artistId)",
)
fun albumStatus(
fun artistStatus(
sourceId: Long,
albumId: String,
artistId: String,
): Flow<ListDownloadStatus>
@Query(
@@ -55,11 +50,11 @@ interface DownloadDao {
"COALESCE(SUM(CASE WHEN song_downloads.status IN ('Queued', 'Running') THEN 1 ELSE 0 END), 0) AS downloading " +
"FROM songs " +
"LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " +
"WHERE songs.sourceId = :sourceId AND songs.artistId = :artistId",
"WHERE songs.sourceId = :sourceId AND songs.albumId = :albumId",
)
fun artistStatus(
fun albumStatus(
sourceId: Long,
artistId: String,
albumId: String,
): Flow<ListDownloadStatus>
@Query(
@@ -162,6 +162,16 @@ interface QueueDao {
albumId: String,
): List<String>
@Query(
"SELECT id FROM songs WHERE sourceId = :sourceId " +
"AND albumId IN (SELECT id FROM albums WHERE sourceId = :sourceId AND artistId = :artistId) " +
"ORDER BY albumId, disc, track, id",
)
suspend fun artistSongIds(
sourceId: Long,
artistId: String,
): List<String>
@Query(
"SELECT songs.id FROM playlist_songs " +
"JOIN songs ON songs.sourceId = playlist_songs.sourceId AND songs.id = playlist_songs.songId " +
@@ -42,6 +42,8 @@ data class SongDownload(
enum class DownloadList { Album, Playlist, Artist }
enum class BulkDownloadAction { Download, Cancel, Delete }
data class ListDownloadStatus(
val total: Long = 0,
val downloaded: Long = 0,
@@ -8,6 +8,7 @@ import com.subtracks.data.download.DownloadEngine
import com.subtracks.data.download.EngineDownload
import com.subtracks.data.download.EngineRequest
import com.subtracks.data.download.EngineStatus
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.DownloadList
import com.subtracks.data.model.DownloadStatus
import com.subtracks.data.model.ListDownloadStatus
@@ -111,15 +112,19 @@ class DownloadRepository(
private suspend fun enqueueIfAbsent(
sourceId: Long,
songId: String,
): Boolean =
mutex.withLock {
if (alreadyDownloadingOrDownloaded(sourceId, songId)) return@withLock false
val existing = db.downloadDao().find(sourceId, songId)
existing?.engineId?.let(engine::cancel)
val url = sourceRepository.downloadUri(sourceId, songId) ?: return@withLock false
enqueue(sourceId, songId, url)
true
}
): Boolean = mutex.withLock { enqueueIfAbsentLocked(sourceId, songId) }
private suspend fun enqueueIfAbsentLocked(
sourceId: Long,
songId: String,
): Boolean {
if (alreadyDownloadingOrDownloaded(sourceId, songId)) return false
val existing = db.downloadDao().find(sourceId, songId)
existing?.engineId?.let(engine::cancel)
val url = sourceRepository.downloadUri(sourceId, songId) ?: return false
enqueue(sourceId, songId, url)
return true
}
suspend fun remove(
sourceId: Long,
@@ -152,15 +157,33 @@ class DownloadRepository(
return@withContext
}
val songIds = songIds(sourceId, list, refId)
if (songIds.isEmpty()) return@withContext
if (songIds.isEmpty()) {
showMessage(EMPTY_LIST)
return@withContext
}
if (sourceRepository.downloadUri(sourceId, songIds.first()) == null) {
showMessage(NO_ADDRESS)
return@withContext
}
songIds.forEach { enqueueIfAbsent(sourceId, it) }
// Under one lock, so a cancel that lands mid-loop waits for the queue to be complete and
// then removes all of it, rather than missing the songs enqueued after it looked.
mutex.withLock {
songIds.forEach { songId -> runCatching { enqueueIfAbsentLocked(sourceId, songId) } }
}
fetchArtworkSafely(sourceId, songIds)
}
suspend fun applyAction(
sourceId: Long,
list: DownloadList,
refId: String,
action: BulkDownloadAction,
) = when (action) {
BulkDownloadAction.Download -> downloadAll(sourceId, list, refId)
BulkDownloadAction.Cancel -> cancelAll(sourceId, list, refId)
BulkDownloadAction.Delete -> deleteAll(sourceId, list, refId)
}
suspend fun cancelAll(
sourceId: Long,
list: DownloadList,
@@ -367,7 +390,7 @@ class DownloadRepository(
when (list) {
DownloadList.Album -> db.queueDao().albumSongIds(sourceId, refId)
DownloadList.Playlist -> db.queueDao().playlistSongIds(sourceId, refId)
DownloadList.Artist -> db.downloadDao().artistSongIds(sourceId, refId)
DownloadList.Artist -> db.queueDao().artistSongIds(sourceId, refId)
}
private suspend fun fetchArtworkSafely(
@@ -471,6 +494,7 @@ class DownloadRepository(
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 EMPTY_LIST = "Nothing left to download from this list"
const val STOPPED = "The download stopped unexpectedly"
const val NOT_ACTIVE = "Can't download from a source that isn't active"
const val ART_TIMEOUT_MS = 60_000L
@@ -63,6 +63,7 @@ import androidx.compose.ui.text.style.TextAlign
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.unit.Dp
import androidx.compose.ui.unit.dp
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.ListDownloadStatus
import com.subtracks.ui.theme.ArtworkColors
@@ -208,8 +209,6 @@ private fun HeroSubtitle(
)
}
enum class BulkDownloadAction { Download, Cancel, Delete }
fun ListDownloadStatus.action(): BulkDownloadAction =
when {
complete -> BulkDownloadAction.Delete
@@ -45,6 +45,7 @@ import androidx.compose.ui.graphics.Color
import androidx.compose.ui.graphics.vector.ImageVector
import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.unit.dp
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.DownloadStatus
import com.subtracks.data.model.ListDownloadStatus
@@ -21,6 +21,7 @@ import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.subtracks.data.model.Album
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.Disc
import com.subtracks.data.model.ListDownloadStatus
@@ -29,7 +30,6 @@ import com.subtracks.data.model.Song
import com.subtracks.data.model.SongDownload
import com.subtracks.data.source.StarType
import com.subtracks.playback.PlaybackController
import com.subtracks.ui.components.BulkDownloadAction
import com.subtracks.ui.components.ContextMenuHost
import com.subtracks.ui.components.HeroDetailScaffold
import com.subtracks.ui.components.HeroHeader
@@ -84,13 +84,7 @@ fun AlbumDetailRoute(
artwork = rememberArtworkColors(shortcutArt ?: viewModel.coverArt(album?.coverArt, true), THEME_TRANSITION_MS),
downloads = downloads,
downloadStatus = downloadStatus,
onDownloadAction = { action ->
when (action) {
BulkDownloadAction.Download -> viewModel.downloadAll()
BulkDownloadAction.Cancel -> viewModel.cancelDownloads()
BulkDownloadAction.Delete -> viewModel.deleteDownloads()
}
},
onDownloadAction = viewModel::onDownloadAction,
onBack = onBack,
onSongClick = viewModel::play,
onSongLongClick = { contextMenuHost?.show(it, actions) },
@@ -3,6 +3,7 @@ package com.subtracks.ui.library
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.subtracks.data.model.Album
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.Disc
import com.subtracks.data.model.DownloadList
@@ -46,18 +47,10 @@ class AlbumDetailViewModel(
.flatMapLatest { downloadRepository.status(it, DownloadList.Album, albumId) }
.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), ListDownloadStatus())
fun downloadAll() {
viewModelScope.launch { downloadRepository.downloadAll(sourceId.first(), DownloadList.Album, albumId) }
}
fun cancelDownloads() {
viewModelScope.launch { downloadRepository.cancelAll(sourceId.first(), DownloadList.Album, albumId) }
}
fun deleteDownloads() {
fun onDownloadAction(action: BulkDownloadAction) {
viewModelScope.launch {
downloadRepository.deleteAll(sourceId.first(), DownloadList.Album, albumId)
playbackController.refreshMediaItems()
downloadRepository.applyAction(sourceId.first(), DownloadList.Album, albumId, action)
if (action == BulkDownloadAction.Delete) playbackController.refreshMediaItems()
}
}
@@ -74,6 +74,7 @@ import androidx.compose.ui.unit.dp
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.subtracks.data.model.Album
import com.subtracks.data.model.Artist
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.source.StarType
import com.subtracks.playback.PlaybackController
@@ -146,9 +147,9 @@ fun ArtistDetailRoute(
shuffleAlbum = { viewModel.shuffleAlbum(it.id) },
playNext = { playbackController.playNext(it.sourceId, it.kind, it.refId) },
addToQueue = { playbackController.addToQueue(it.sourceId, it.kind, it.refId) },
downloadArtist = { viewModel.downloadAll() },
cancelArtistDownload = { viewModel.cancelDownloads() },
deleteArtistDownload = { viewModel.deleteDownloads() },
downloadArtist = { viewModel.onDownloadAction(BulkDownloadAction.Download) },
cancelArtistDownload = { viewModel.onDownloadAction(BulkDownloadAction.Cancel) },
deleteArtistDownload = { viewModel.onDownloadAction(BulkDownloadAction.Delete) },
setStar = setStar,
viewAlbum = onViewAlbum,
)
@@ -4,6 +4,7 @@ import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.subtracks.data.model.Album
import com.subtracks.data.model.Artist
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.DownloadList
import com.subtracks.data.model.ListDownloadStatus
@@ -43,18 +44,10 @@ class ArtistDetailViewModel(
.flatMapLatest { downloadRepository.status(it, DownloadList.Artist, artistId) }
.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), ListDownloadStatus())
fun downloadAll() {
viewModelScope.launch { downloadRepository.downloadAll(sourceId.first(), DownloadList.Artist, artistId) }
}
fun cancelDownloads() {
viewModelScope.launch { downloadRepository.cancelAll(sourceId.first(), DownloadList.Artist, artistId) }
}
fun deleteDownloads() {
fun onDownloadAction(action: BulkDownloadAction) {
viewModelScope.launch {
downloadRepository.deleteAll(sourceId.first(), DownloadList.Artist, artistId)
playbackController.refreshMediaItems()
downloadRepository.applyAction(sourceId.first(), DownloadList.Artist, artistId, action)
if (action == BulkDownloadAction.Delete) playbackController.refreshMediaItems()
}
}
@@ -11,6 +11,7 @@ import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.paging.compose.LazyPagingItems
import androidx.paging.compose.collectAsLazyPagingItems
import androidx.paging.compose.itemKey
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.ListDownloadStatus
import com.subtracks.data.model.Playlist
@@ -19,7 +20,6 @@ import com.subtracks.data.model.QueueKind
import com.subtracks.data.model.SongDownload
import com.subtracks.data.source.StarType
import com.subtracks.playback.PlaybackController
import com.subtracks.ui.components.BulkDownloadAction
import com.subtracks.ui.components.ContextMenuHost
import com.subtracks.ui.components.HeroDetailScaffold
import com.subtracks.ui.components.HeroHeader
@@ -70,13 +70,7 @@ fun PlaylistDetailRoute(
artwork = rememberArtworkColors(viewModel.coverArt(playlist?.coverArt, true), THEME_TRANSITION_MS),
downloads = downloads,
downloadStatus = downloadStatus,
onDownloadAction = { action ->
when (action) {
BulkDownloadAction.Download -> viewModel.downloadAll()
BulkDownloadAction.Cancel -> viewModel.cancelDownloads()
BulkDownloadAction.Delete -> viewModel.deleteDownloads()
}
},
onDownloadAction = viewModel::onDownloadAction,
onBack = onBack,
onSongClick = viewModel::play,
onSongLongClick = { contextMenuHost?.show(it, actions) },
@@ -4,6 +4,7 @@ import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import androidx.paging.PagingData
import androidx.paging.cachedIn
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.CoverArtRef
import com.subtracks.data.model.DownloadList
import com.subtracks.data.model.ListDownloadStatus
@@ -50,18 +51,10 @@ class PlaylistDetailViewModel(
.flatMapLatest { downloadRepository.status(it, DownloadList.Playlist, playlistId) }
.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), ListDownloadStatus())
fun downloadAll() {
viewModelScope.launch { downloadRepository.downloadAll(sourceId.first(), DownloadList.Playlist, playlistId) }
}
fun cancelDownloads() {
viewModelScope.launch { downloadRepository.cancelAll(sourceId.first(), DownloadList.Playlist, playlistId) }
}
fun deleteDownloads() {
fun onDownloadAction(action: BulkDownloadAction) {
viewModelScope.launch {
downloadRepository.deleteAll(sourceId.first(), DownloadList.Playlist, playlistId)
playbackController.refreshMediaItems()
downloadRepository.applyAction(sourceId.first(), DownloadList.Playlist, playlistId, action)
if (action == BulkDownloadAction.Delete) playbackController.refreshMediaItems()
}
}
@@ -12,6 +12,7 @@ import com.subtracks.data.download.ArtworkStore
import com.subtracks.data.download.FakeDownloadEngine
import com.subtracks.data.model.Album
import com.subtracks.data.model.Artist
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.DownloadList
import com.subtracks.data.model.DownloadStatus
import com.subtracks.data.model.ListDownloadStatus
@@ -490,6 +491,10 @@ class DownloadRepositoryTest {
db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Queued, engineId = 22))
assertEquals(ListDownloadStatus(total = 2, downloaded = 1, downloading = 1), statusOf(DownloadList.Album, "al1"))
db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Running, engineId = 22))
assertEquals(ListDownloadStatus(total = 2, downloaded = 1, downloading = 1), statusOf(DownloadList.Album, "al1"))
}
@Test
@@ -514,6 +519,72 @@ class DownloadRepositoryTest {
refId: String,
): ListDownloadStatus = runBlocking { repository.status(1, list, refId).first() }
@Test
fun downloadingAPlaylistQueuesEverySong() =
runTest {
seedLibrary()
runBlocking {
db.libraryDao().upsertPlaylists(listOf(Playlist(1, "pl1", "Playlist", null, null, 2, 0)))
db.libraryDao().upsertPlaylistSongs(
listOf(
PlaylistSong(1, "pl1", "s1", 0),
PlaylistSong(1, "pl1", "s2", 1),
),
)
}
runBlocking { repository.downloadAll(1, DownloadList.Playlist, "pl1") }
assertEquals(listOf("s1", "s2"), requestedSongIds())
}
@Test
fun downloadingAnArtistQueuesTheSongsOfItsAlbums() =
runTest {
seedLibrary()
// s2 is a guest on the album, so its own artistId differs: the artist page lists the
// album, and the download follows what the page shows.
runBlocking {
db.libraryDao().upsertSongs(
listOf(
Song(
sourceId = 1,
id = "s2",
albumId = "al1",
artistId = "ar2",
title = "Song 2",
album = "Album",
artist = "Guest",
duration = 100,
track = 2,
disc = 1,
starred = null,
genre = null,
),
),
)
}
runBlocking { repository.downloadAll(1, DownloadList.Artist, "ar1") }
assertEquals(listOf("s1", "s2"), requestedSongIds())
}
@Test
fun applyActionRoutesToTheMatchingBulkAction() =
runTest {
seedLibrary()
runBlocking { repository.applyAction(1, DownloadList.Album, "al1", BulkDownloadAction.Download) }
assertEquals(2, engine.requests.size)
runBlocking { repository.applyAction(1, DownloadList.Album, "al1", BulkDownloadAction.Cancel) }
assertEquals(ListDownloadStatus(total = 2), statusOf(DownloadList.Album, "al1"))
}
private fun requestedSongIds(): List<String> = engine.requests.map { it.second.path.substringAfterLast('/') }
private fun album(coverArt: String?) =
Album(
sourceId = 1,
@@ -0,0 +1,29 @@
package com.subtracks.ui.components
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.ListDownloadStatus
import org.junit.Assert.assertEquals
import org.junit.Test
class BulkDownloadActionTest {
@Test
fun anUndownloadedListOffersDownload() {
assertEquals(BulkDownloadAction.Download, ListDownloadStatus(total = 3).action())
assertEquals(BulkDownloadAction.Download, ListDownloadStatus().action())
}
@Test
fun aPartiallyDownloadedListStillOffersDownload() {
assertEquals(BulkDownloadAction.Download, ListDownloadStatus(total = 3, downloaded = 1).action())
}
@Test
fun aListWithDownloadsInFlightOffersCancel() {
assertEquals(BulkDownloadAction.Cancel, ListDownloadStatus(total = 3, downloaded = 1, downloading = 1).action())
}
@Test
fun aFullyDownloadedListOffersDelete() {
assertEquals(BulkDownloadAction.Delete, ListDownloadStatus(total = 3, downloaded = 3).action())
}
}
@@ -7,6 +7,7 @@ import androidx.compose.ui.test.onNodeWithContentDescription
import androidx.compose.ui.test.performClick
import androidx.compose.ui.unit.dp
import androidx.test.ext.junit.runners.AndroidJUnit4
import com.subtracks.data.model.BulkDownloadAction
import com.subtracks.data.model.ListDownloadStatus
import com.subtracks.ui.theme.SubtracksTheme
import org.junit.Assert.assertEquals
@@ -21,27 +22,6 @@ class HeroDownloadTest {
@get:Rule
val composeRule = createAndroidComposeRule<ComponentActivity>()
@Test
fun anUndownloadedListOffersDownload() {
assertEquals(BulkDownloadAction.Download, ListDownloadStatus(total = 3).action())
assertEquals(BulkDownloadAction.Download, ListDownloadStatus().action())
}
@Test
fun aPartiallyDownloadedListStillOffersDownload() {
assertEquals(BulkDownloadAction.Download, ListDownloadStatus(total = 3, downloaded = 1).action())
}
@Test
fun aListWithDownloadsInFlightOffersCancel() {
assertEquals(BulkDownloadAction.Cancel, ListDownloadStatus(total = 3, downloaded = 1, downloading = 1).action())
}
@Test
fun aFullyDownloadedListOffersDelete() {
assertEquals(BulkDownloadAction.Delete, ListDownloadStatus(total = 3, downloaded = 3).action())
}
@Test
fun tappingTheButtonReportsTheActionItsStateOffers() {
val actions = mutableListOf<BulkDownloadAction>()