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) {