From 7db6d1365fccd9830100ff0ab6f8e692dcd8dbff Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Mon, 28 Sep 2026 07:19:32 +0000 Subject: [PATCH] reconcile and notify downloads outside the active source A source switched away from kept downloading on the platform while our poller ignored it, so its rows froze as Running, the notification vanished and the only Cancel went with it. The poll now reconciles every source's in-flight rows (the engine reports status without a source) and the notification reads all of them, which is also what cancelActive already acted on. The grouped tab queries drop entities with nothing downloaded, since the UI treats a missing entry and an all-zero one the same. The indicator's two-value enum was used as a boolean, so it is one; and the library screenshot now asserts the ring is drawn, rather than capturing an image that would stay green if it disappeared. --- .../com/subtracks/data/db/DownloadDao.kt | 12 +++++-- .../data/download/DownloadCancelReceiver.kt | 2 ++ .../data/download/DownloadNotifier.kt | 4 +-- .../subtracks/data/repo/DownloadRepository.kt | 8 +++-- .../ui/components/DownloadIndicator.kt | 7 ++-- .../ui/components/ItemContextMenu.kt | 17 +++++----- .../data/repo/DownloadRepositoryTest.kt | 34 +++++++++++++++++++ .../com/subtracks/ui/AppScreenshotTest.kt | 4 +++ .../ui/components/DownloadIndicatorTest.kt | 27 ++++++++------- 9 files changed, 81 insertions(+), 34 deletions(-) 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()) } }