diff --git a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt index 3f407e55..e3690fb6 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -15,6 +15,9 @@ interface DownloadDao { @Query("SELECT * FROM song_downloads WHERE sourceId = :sourceId") fun downloads(sourceId: Long): Flow> + @Query("SELECT * FROM song_downloads WHERE status IN ('Queued', 'Running')") + fun activeDownloads(): Flow> + @Query("SELECT * FROM song_downloads ORDER BY rowid") suspend fun all(): List @@ -67,7 +70,8 @@ interface DownloadDao { "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 IS NOT NULL " + - "GROUP BY songs.albumId", + "GROUP BY songs.albumId " + + "HAVING downloaded > 0 OR downloading > 0", ) fun albumStatuses(sourceId: Long): Flow> @@ -80,7 +84,8 @@ interface DownloadDao { "JOIN albums ON albums.sourceId = songs.sourceId AND albums.id = songs.albumId " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE songs.sourceId = :sourceId AND albums.artistId IS NOT NULL " + - "GROUP BY albums.artistId", + "GROUP BY albums.artistId " + + "HAVING downloaded > 0 OR downloading > 0", ) fun artistStatuses(sourceId: Long): Flow> @@ -93,7 +98,8 @@ interface DownloadDao { "JOIN songs ON songs.sourceId = playlist_songs.sourceId AND songs.id = playlist_songs.songId " + "LEFT JOIN song_downloads ON song_downloads.sourceId = songs.sourceId AND song_downloads.songId = songs.id " + "WHERE playlist_songs.sourceId = :sourceId " + - "GROUP BY playlist_songs.playlistId", + "GROUP BY playlist_songs.playlistId " + + "HAVING downloaded > 0 OR downloading > 0", ) fun playlistStatuses(sourceId: Long): Flow> diff --git a/app/src/main/kotlin/com/subtracks/data/download/DownloadCancelReceiver.kt b/app/src/main/kotlin/com/subtracks/data/download/DownloadCancelReceiver.kt index 55370ffd..d9bef4f3 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/DownloadCancelReceiver.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/DownloadCancelReceiver.kt @@ -20,6 +20,8 @@ class DownloadCancelReceiver : context: Context, intent: Intent, ) { + // The broadcast budget is short, and cancelActive waits for the repository mutex, which a + // large downloadAll holds for the whole insert pass; a huge queue could outlast it. val pending = goAsync() CoroutineScope(SupervisorJob() + Dispatchers.IO).launch { try { diff --git a/app/src/main/kotlin/com/subtracks/data/download/DownloadNotifier.kt b/app/src/main/kotlin/com/subtracks/data/download/DownloadNotifier.kt index d6f1a131..fb5d8aa2 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/DownloadNotifier.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/DownloadNotifier.kt @@ -31,8 +31,8 @@ class DownloadNotifier( fun start() { createChannel() scope.launch { - repository.states().collect { rows -> - val content = downloadNotification(rows.values) + repository.activeDownloads().collect { rows -> + val content = downloadNotification(rows) if (content == null) { manager.cancel(ID) } else if (manager.areNotificationsEnabled()) { 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 c67212a6..73f85f44 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -82,6 +82,8 @@ class DownloadRepository( fun states(): StateFlow> = statesFlow + fun activeDownloads(): Flow> = db.downloadDao().activeDownloads() + suspend fun awaitLoaded() { if (started) loaded.await() } @@ -539,8 +541,10 @@ class DownloadRepository( delay(POLL_MS) val promoted = mutex.withLock { - val sourceId = sourceRepository.activeSourceIdOnce() - val active = db.downloadDao().all().filter { it.status.isActive && it.sourceId == sourceId } + // Reconcile every source's in-flight rows, not just the active one: the + // engine reports status without a source, and a source switched away + // from keeps downloading, so its rows must not freeze as Running. + val active = db.downloadDao().all().filter { it.status.isActive } if (active.isEmpty()) return@launch reconcileRows(active) promote() diff --git a/app/src/main/kotlin/com/subtracks/ui/components/DownloadIndicator.kt b/app/src/main/kotlin/com/subtracks/ui/components/DownloadIndicator.kt index 7728fed4..366ea481 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/DownloadIndicator.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/DownloadIndicator.kt @@ -20,10 +20,7 @@ import com.subtracks.data.model.ListDownloadStatus const val DOWNLOADING_INDICATOR_TAG = "Downloading" -enum class DownloadIndicator { Hidden, InProgress } - -fun ListDownloadStatus.indicator(): DownloadIndicator = - if (total > 0 && downloading > 0) DownloadIndicator.InProgress else DownloadIndicator.Hidden +internal fun ListDownloadStatus.isDownloading(): Boolean = total > 0 && downloading > 0 @Composable fun ListDownloadIndicator( @@ -32,7 +29,7 @@ fun ListDownloadIndicator( scrim: Boolean = false, ) { val current = status ?: return - if (current.indicator() == DownloadIndicator.Hidden) return + if (!current.isDownloading()) return val progress = { current.downloaded.toFloat() / current.total } if (scrim) { Surface( diff --git a/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt b/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt index 06328b80..2949e22d 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/ItemContextMenu.kt @@ -126,14 +126,6 @@ fun MenuTarget.bulkRef(): Pair? = is MenuTarget.Song -> null } -fun MenuTarget.listDownload(): ListDownloadStatus? = - when (this) { - is MenuTarget.Album -> download - is MenuTarget.Playlist -> download - is MenuTarget.Artist -> download - is MenuTarget.Song -> null - } - class ItemActions( val playNext: ((QueueRef) -> Unit)? = null, val addToQueue: ((QueueRef) -> Unit)? = null, @@ -337,7 +329,14 @@ private fun BulkDownloadItem( dismiss: (() -> Unit) -> Unit, ) { val bulk = actions.bulkDownload ?: return - for (action in (target.listDownload() ?: ListDownloadStatus()).actions()) { + val status = + when (target) { + is MenuTarget.Album -> target.download + is MenuTarget.Playlist -> target.download + is MenuTarget.Artist -> target.download + is MenuTarget.Song -> null + } + for (action in (status ?: ListDownloadStatus()).actions()) { when (action) { BulkDownloadAction.Delete -> MenuItem(Icons.Rounded.Delete, "Delete downloads") { dismiss { bulk(target, action) } } BulkDownloadAction.Cancel -> MenuItem(Icons.Rounded.Cancel, "Cancel downloads") { dismiss { bulk(target, action) } } 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 6669e1e5..b35ea1eb 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -851,6 +851,40 @@ class DownloadRepositoryTest { assertEquals(setOf("ar1" to "Artist"), songs.map { it.artistId to it.artistName }.toSet()) } + @Test + fun activeDownloadsCoverEverySourceNotJustTheActiveOne() = + runTest { + seedLibrary() + runBlocking { + db.sourcesDao().upsertSource(Source(2, "other", "http://other/", isActive = false, createdAt = 0)) + db.libraryDao().upsertSongs( + listOf( + Song( + sourceId = 2, + id = "o1", + albumId = "al2", + artistId = "ar2", + title = "Other", + album = "Other", + artist = "Other", + duration = 100, + track = 1, + disc = 1, + starred = null, + genre = null, + ), + ), + ) + db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Running, engineId = 1)) + db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Completed)) + db.downloadDao().upsert(SongDownload(2, "o1", DownloadStatus.Queued)) + } + + val rows = runBlocking { repository.activeDownloads().first() } + + assertEquals(setOf(1L to "s1", 2L to "o1"), rows.map { it.sourceId to it.songId }.toSet()) + } + private fun requestedSongIds(): List = engine.requests.map { it.second.path.substringAfterLast('/') } private fun seedSongs(tracks: IntRange) { diff --git a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt index 69334c8f..41db43ae 100644 --- a/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/AppScreenshotTest.kt @@ -11,7 +11,9 @@ import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.runtime.remember import androidx.compose.ui.Modifier +import androidx.compose.ui.test.assertCountEquals import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule +import androidx.compose.ui.test.onAllNodesWithContentDescription import androidx.compose.ui.test.onAllNodesWithTag import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onRoot @@ -41,6 +43,7 @@ import com.subtracks.data.prefs.StreamQuality import com.subtracks.playback.PlaybackState import com.subtracks.playback.QueueItem import com.subtracks.playback.RepeatMode +import com.subtracks.ui.components.DOWNLOADING_INDICATOR_TAG import com.subtracks.ui.library.ALBUM_COVER_TAG import com.subtracks.ui.library.ARTIST_NAME_SCRIM_TAG import com.subtracks.ui.library.AlbumDetailScreen @@ -154,6 +157,7 @@ class AppScreenshotTest { } } awaitTag(ALBUM_COVER_TAG) + composeRule.onAllNodesWithContentDescription(DOWNLOADING_INDICATOR_TAG).assertCountEquals(1) composeRule.onRoot().captureRoboImage("src/test/screenshots/library_albums_downloads.png") } diff --git a/app/src/test/kotlin/com/subtracks/ui/components/DownloadIndicatorTest.kt b/app/src/test/kotlin/com/subtracks/ui/components/DownloadIndicatorTest.kt index b9b682e5..abe7b874 100644 --- a/app/src/test/kotlin/com/subtracks/ui/components/DownloadIndicatorTest.kt +++ b/app/src/test/kotlin/com/subtracks/ui/components/DownloadIndicatorTest.kt @@ -1,37 +1,38 @@ package com.subtracks.ui.components import com.subtracks.data.model.ListDownloadStatus -import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test class DownloadIndicatorTest { @Test - fun anUntouchedListShowsNothing() { - assertEquals(DownloadIndicator.Hidden, ListDownloadStatus().indicator()) + fun anUntouchedListIsNotDownloading() { + assertFalse(ListDownloadStatus().isDownloading()) } @Test - fun aPopulatedButUnstartedListShowsNothing() { - assertEquals(DownloadIndicator.Hidden, ListDownloadStatus(total = 4).indicator()) + fun aPopulatedButUnstartedListIsNotDownloading() { + assertFalse(ListDownloadStatus(total = 4).isDownloading()) } @Test - fun aQueuedButNotYetDownloadedListShowsProgress() { - assertEquals(DownloadIndicator.InProgress, ListDownloadStatus(total = 4, downloading = 4).indicator()) + fun aQueuedButNotYetDownloadedListIsDownloading() { + assertTrue(ListDownloadStatus(total = 4, downloading = 4).isDownloading()) } @Test - fun aPartlyDownloadedIdleListShowsNothing() { - assertEquals(DownloadIndicator.Hidden, ListDownloadStatus(total = 4, downloaded = 2).indicator()) + fun aPartlyDownloadedIdleListIsNotDownloading() { + assertFalse(ListDownloadStatus(total = 4, downloaded = 2).isDownloading()) } @Test - fun aPartlyDownloadedListWithSomethingInFlightShowsProgress() { - assertEquals(DownloadIndicator.InProgress, ListDownloadStatus(total = 4, downloaded = 2, downloading = 1).indicator()) + fun aPartlyDownloadedListWithSomethingInFlightIsDownloading() { + assertTrue(ListDownloadStatus(total = 4, downloaded = 2, downloading = 1).isDownloading()) } @Test - fun aFullyDownloadedListShowsNothing() { - assertEquals(DownloadIndicator.Hidden, ListDownloadStatus(total = 4, downloaded = 4).indicator()) + fun aFullyDownloadedListIsNotDownloading() { + assertFalse(ListDownloadStatus(total = 4, downloaded = 4).isDownloading()) } }