From a57ca922508645bbd2286ded31c60667bccdf2a5 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Sun, 27 Sep 2026 13:19:10 +0000 Subject: [PATCH] cancel a list in one go, and round the hero's touch feedback - Cancelling a big playlist did per-row work: a binder cancel, two queries and a delete each, thousands of times, under the lock. The platform takes a list of ids in one call, so the cancel is now one binder call, one delete per chunk of rows and one sweep, which is both the several seconds and the stale button. - Partial files are covered by the same sweep, which already deletes any file in a source directory that no row names; a test now proves a stray .part goes with the cancel. - The hero's control was a plain clickable Box after the long-press work, so its splash was square; it clips to a circle the way an IconButton does. --- .../com/subtracks/data/db/DownloadDao.kt | 6 +++ .../subtracks/data/download/DownloadEngine.kt | 2 +- .../data/download/SystemDownloadEngine.kt | 4 +- .../subtracks/data/repo/DownloadRepository.kt | 41 ++++++++++++------- .../ui/components/HeroDetailScaffold.kt | 3 ++ .../data/download/FakeDownloadEngine.kt | 6 ++- .../data/repo/DownloadRepositoryTest.kt | 29 +++++++++++++ 7 files changed, 71 insertions(+), 20 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 36ce5e22..81eac212 100644 --- a/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt +++ b/app/src/main/kotlin/com/subtracks/data/db/DownloadDao.kt @@ -83,6 +83,12 @@ interface DownloadDao { songId: String, ) + @Query("DELETE FROM song_downloads WHERE sourceId = :sourceId AND songId IN (:songIds)") + suspend fun deleteSongs( + sourceId: Long, + songIds: Collection, + ) + @Query("DELETE FROM song_downloads WHERE sourceId = :sourceId") suspend fun deleteSource(sourceId: Long) } 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 cf66938e..c55c6993 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/DownloadEngine.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/DownloadEngine.kt @@ -20,5 +20,5 @@ interface DownloadEngine { fun download(id: Long): EngineDownload? - fun cancel(id: Long) + fun cancel(ids: List) } 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 26419e34..4392d544 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt @@ -40,8 +40,8 @@ class SystemDownloadEngine( } } - override fun cancel(id: Long) { - manager().remove(id) + override fun cancel(ids: List) { + manager().remove(*ids.toLongArray()) } private fun manager(): DownloadManager = context.getSystemService(DownloadManager::class.java) 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 17830c85..53e4f882 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -119,7 +119,7 @@ class DownloadRepository( ): Boolean { if (alreadyDownloadingOrDownloaded(sourceId, songId)) return false val existing = db.downloadDao().find(sourceId, songId) - existing?.engineId?.let(engine::cancel) + existing?.engineId?.let { engine.cancel(listOf(it)) } dir(sourceId).mkdirs() file(sourceId, songId).delete() db.downloadDao().upsert(SongDownload(sourceId = sourceId, songId = songId, status = DownloadStatus.Queued)) @@ -251,10 +251,7 @@ class DownloadRepository( list: DownloadList, refId: String, ) = withContext(dispatcher) { - mutex.withLock { - val removed = rowsFor(sourceId, list, refId).filter { it.status.isActive }.map { removeRow(sourceId, it.songId) } - if (removed.any { it }) sweep(db.downloadDao().all()) - } + mutex.withLock { removeListRows(sourceId, list, refId) { it.status.isActive } } } suspend fun deleteAll( @@ -262,13 +259,25 @@ class DownloadRepository( list: DownloadList, refId: String, ) = withContext(dispatcher) { - mutex.withLock { - val removed = - rowsFor(sourceId, list, refId) - .filter { it.status == DownloadStatus.Completed } - .map { removeRow(sourceId, it.songId) } - if (removed.any { it }) sweep(db.downloadDao().all()) - } + mutex.withLock { removeListRows(sourceId, list, refId) { it.status == DownloadStatus.Completed } } + } + + /** + * One binder call for the platform, one delete per chunk and one sweep, rather than a round of + * each per row: a playlist of thousands took seconds to cancel and left the button stale. + */ + private suspend fun removeListRows( + sourceId: Long, + list: DownloadList, + refId: String, + selecting: (SongDownload) -> Boolean, + ) { + val rows = rowsFor(sourceId, list, refId).filter(selecting) + if (rows.isEmpty()) return + engine.cancel(rows.mapNotNull { it.engineId }) + rows.forEach { file(sourceId, it.songId).delete() } + rows.map { it.songId }.chunked(DELETE_CHUNK).forEach { db.downloadDao().deleteSongs(sourceId, it) } + sweep(db.downloadDao().all()) } suspend fun removeSource(sourceId: Long) = @@ -278,7 +287,8 @@ class DownloadRepository( .downloadDao() .all() .filter { it.sourceId == sourceId } - .forEach { row -> row.engineId?.let(engine::cancel) } + .mapNotNull { it.engineId } + .let(engine::cancel) db.downloadDao().deleteSource(sourceId) dir(sourceId).deleteRecursively() } @@ -407,7 +417,7 @@ class DownloadRepository( songId: String, ): Boolean { val row = db.downloadDao().find(sourceId, songId) ?: return false - row.engineId?.let(engine::cancel) + row.engineId?.let { engine.cancel(listOf(it)) } file(sourceId, songId).delete() db.downloadDao().delete(sourceId, songId) return true @@ -478,7 +488,7 @@ class DownloadRepository( row: SongDownload, message: String, ) { - row.engineId?.let(engine::cancel) + row.engineId?.let { engine.cancel(listOf(it)) } file(row.sourceId, row.songId).delete() db.downloadDao().upsert(row.copy(status = DownloadStatus.Failed, engineId = null, error = message)) } @@ -542,6 +552,7 @@ class DownloadRepository( const val NO_MEDIA = ".nomedia" const val POLL_MS = 1_000L const val IN_FLIGHT_LIMIT = 8 + const val DELETE_CHUNK = 900 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" diff --git a/app/src/main/kotlin/com/subtracks/ui/components/HeroDetailScaffold.kt b/app/src/main/kotlin/com/subtracks/ui/components/HeroDetailScaffold.kt index 626a1015..f9a39632 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/HeroDetailScaffold.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/HeroDetailScaffold.kt @@ -25,6 +25,7 @@ import androidx.compose.foundation.layout.width import androidx.compose.foundation.lazy.LazyColumn import androidx.compose.foundation.lazy.LazyListScope import androidx.compose.foundation.lazy.rememberLazyListState +import androidx.compose.foundation.shape.CircleShape import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.rounded.ArrowBack @@ -53,6 +54,7 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.clip import androidx.compose.ui.draw.drawBehind import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.graphicsLayer @@ -299,6 +301,7 @@ fun HeroHeader( modifier = Modifier .size(48.dp) + .clip(CircleShape) .combinedClickable( enabled = hasSongs, onClick = { onDownloadAction(downloadStatus.action()) }, diff --git a/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt b/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt index cef35dbc..44a979af 100644 --- a/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt +++ b/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt @@ -6,6 +6,7 @@ class FakeDownloadEngine : DownloadEngine { val requests = mutableListOf>() val cancelled = mutableListOf() + var cancelCalls = 0 var enqueueDelayMs = 0L var failEnqueueFor: String? = null @@ -21,8 +22,9 @@ class FakeDownloadEngine : DownloadEngine { override fun download(id: Long): EngineDownload? = downloads[id] - override fun cancel(id: Long) { - cancelled += id + override fun cancel(ids: List) { + cancelCalls++ + cancelled += ids } fun running( 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 d8d1e043..ae8ed134 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -670,6 +670,35 @@ class DownloadRepositoryTest { assertEquals(5L, repository.downloadedBytes(1, DownloadList.Artist, "ar1")) } + @Test + fun cancellingAListRemovesPartialFilesToo() = + runTest { + seedLibrary() + runBlocking { repository.downloadAll(1, DownloadList.Album, "al1") } + writeFile(1, "s1", "half") + writeFile(1, "s1.part", "half") + writeFile(1, "s2", "half") + + runBlocking { repository.cancelAll(1, DownloadList.Album, "al1") } + + assertFalse(file(1, "s1").exists()) + assertFalse(File(file(1, "s1").parentFile, "s1.part").exists()) + assertFalse(file(1, "s2").exists()) + } + + @Test + fun cancellingAListCancelsThePlatformOnce() = + runTest { + seedLibrary() + seedSongs(3..40) + runBlocking { repository.downloadAll(1, DownloadList.Album, "al1") } + + runBlocking { repository.cancelAll(1, DownloadList.Album, "al1") } + + assertEquals(1, engine.cancelCalls) + assertTrue(allRows().isEmpty()) + } + private fun requestedSongIds(): List = engine.requests.map { it.second.path.substringAfterLast('/') } private fun seedSongs(tracks: IntRange) {