From f32c84bce7d850c0400ba17416e2c2d612fd26c3 Mon Sep 17 00:00:00 2001 From: austinried <4966622+austinried@users.noreply.github.com> Date: Tue, 29 Sep 2026 07:52:37 +0000 Subject: [PATCH] address the second offline review: cancel guard, bounded reconcile --- .../data/download/SystemDownloadEngine.kt | 3 ++ .../subtracks/data/repo/DownloadRepository.kt | 3 +- .../subtracks/data/repo/SourceRepository.kt | 5 +++- .../subtracks/playback/PlaybackController.kt | 22 +++++++------- .../com/subtracks/ui/components/Components.kt | 17 +++++++++++ .../subtracks/ui/library/AlbumDetailScreen.kt | 10 ++----- .../ui/library/ArtistDetailScreen.kt | 10 ++----- .../ui/library/PlaylistDetailScreen.kt | 10 ++----- .../com/subtracks/ui/playback/QueueScreen.kt | 29 +++++++++++-------- .../data/download/FakeDownloadEngine.kt | 2 ++ .../data/repo/DownloadRepositoryTest.kt | 17 +++++++++++ .../subtracks/data/sync/SyncManagerTest.kt | 1 + 12 files changed, 81 insertions(+), 48 deletions(-) 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 1201e9ed..dea352e4 100644 --- a/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt +++ b/app/src/main/kotlin/com/subtracks/data/download/SystemDownloadEngine.kt @@ -43,6 +43,9 @@ class SystemDownloadEngine( } override fun cancel(ids: List) { + // DownloadManager.remove throws on an empty id list, and queued rows waiting for a slot + // have no engine id yet, so an offline cancel can legitimately select none. + if (ids.isEmpty()) return manager().remove(*ids.toLongArray()) } 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 b715c9fa..a9631fce 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/DownloadRepository.kt @@ -327,7 +327,8 @@ class DownloadRepository( private suspend fun removeRows(rows: List) { if (rows.isEmpty()) return - engine.cancel(rows.mapNotNull { it.engineId }) + val engineIds = rows.mapNotNull { it.engineId } + if (engineIds.isNotEmpty()) engine.cancel(engineIds) rows.groupBy { it.sourceId }.forEach { (sourceId, group) -> group.forEach { file(sourceId, it.songId).delete() } group.map { it.songId }.chunked(DELETE_CHUNK).forEach { db.downloadDao().deleteSongs(sourceId, it) } diff --git a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt index 36fa2f33..5bc8ea29 100644 --- a/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt +++ b/app/src/main/kotlin/com/subtracks/data/repo/SourceRepository.kt @@ -75,6 +75,9 @@ class SourceRepository( prefs.offlineMode().collect { _offline.value = it } } scope.launch { + // Read the persisted flag before the source config can build a source or probe it, so a + // cold start with offline on never touches the network. + _offline.value = prefs.offlineMode().first() combine( db.sourcesDao().activeSubsonicConfig(), networkMode, @@ -88,7 +91,7 @@ class SourceRepository( _quality.value = quality val sourceChanged = config?.id != activeSourceId activeSourceId = config?.id - if (sourceChanged && config != null && config.useTokenAuth && !prefs.offlineMode().first()) { + if (sourceChanged && config != null && config.useTokenAuth && !_offline.value) { scope.launch { runCatching { config.toClient().check("ping") } } } } diff --git a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt index 3b75125e..c1706c77 100644 --- a/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt +++ b/app/src/main/kotlin/com/subtracks/playback/PlaybackController.kt @@ -24,6 +24,7 @@ import kotlinx.coroutines.flow.drop import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.withContext import kotlin.random.Random data class QueueItem( @@ -1266,17 +1267,18 @@ class PlaybackController( snapshot: QueueSnapshot, from: Long, step: Long, - ): Long? { - var position = from - var scanned = 0L - while (position in 0 until snapshot.size && scanned < OFFLINE_SCAN_LIMIT) { - val songId = queueRepository.itemAt(snapshot, position)?.song?.id - if (songId != null && downloads.localUri(songId) != null) return position - position += step - scanned++ + ): Long? = + withContext(Dispatchers.IO) { + var position = from + var scanned = 0L + while (position in 0 until snapshot.size && scanned < OFFLINE_SCAN_LIMIT) { + val songId = queueRepository.itemAt(snapshot, position)?.song?.id + if (songId != null && downloads.localUri(songId) != null) return@withContext position + position += step + scanned++ + } + null } - return null - } private suspend fun reloadForOffline() = startLock.withLock { diff --git a/app/src/main/kotlin/com/subtracks/ui/components/Components.kt b/app/src/main/kotlin/com/subtracks/ui/components/Components.kt index b0196298..ffe10456 100644 --- a/app/src/main/kotlin/com/subtracks/ui/components/Components.kt +++ b/app/src/main/kotlin/com/subtracks/ui/components/Components.kt @@ -22,6 +22,7 @@ import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember @@ -309,6 +310,22 @@ fun FilteredEmptyState( } } +@Composable +fun CloseWhenDownloadsGone( + downloaded: Long, + offline: Boolean, + onClose: () -> Unit, +) { + var hadDownloads by remember { mutableStateOf(false) } + LaunchedEffect(downloaded, offline) { + if (downloaded > 0) { + hadDownloads = true + } else if (hadDownloads && offline) { + onClose() + } + } +} + @Composable fun Modifier.statusBarScrim(): Modifier { val top = with(LocalDensity.current) { WindowInsets.statusBars.getTop(this).toDp() } diff --git a/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt index 778ad9c5..57041aca 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/AlbumDetailScreen.kt @@ -34,6 +34,7 @@ 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.CloseWhenDownloadsGone import com.subtracks.ui.components.ContextMenuHost import com.subtracks.ui.components.DeleteDownloadsDialog import com.subtracks.ui.components.HeroDetailScaffold @@ -67,14 +68,7 @@ fun AlbumDetailRoute( val downloads by viewModel.downloads.collectAsStateWithLifecycle() val downloadStatus by viewModel.downloadStatus.collectAsStateWithLifecycle() val offline by viewModel.offline.collectAsStateWithLifecycle() - var hadDownloads by remember { mutableStateOf(false) } - LaunchedEffect(downloadStatus.downloaded, offline) { - if (downloadStatus.downloaded > 0) { - hadDownloads = true - } else if (hadDownloads && offline) { - onBack() - } - } + CloseWhenDownloadsGone(downloadStatus.downloaded, offline, onBack) val scope = rememberCoroutineScope() var pendingDelete by remember { mutableStateOf(null) } val playback by playbackController.state.collectAsStateWithLifecycle() diff --git a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt index 49190085..f0fec1cb 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/ArtistDetailScreen.kt @@ -82,6 +82,7 @@ import com.subtracks.data.model.CoverArtRef import com.subtracks.data.model.ListDownloadStatus import com.subtracks.data.source.StarType import com.subtracks.playback.PlaybackController +import com.subtracks.ui.components.CloseWhenDownloadsGone import com.subtracks.ui.components.ContextMenuHost import com.subtracks.ui.components.CoverArt import com.subtracks.ui.components.DeleteDownloadsDialog @@ -150,14 +151,7 @@ fun ArtistDetailRoute( val artThumbnail by viewModel.artThumbnail.collectAsStateWithLifecycle() val downloadStatus by viewModel.downloadStatus.collectAsStateWithLifecycle() val offline by viewModel.offline.collectAsStateWithLifecycle() - var hadDownloads by remember { mutableStateOf(false) } - LaunchedEffect(downloadStatus.downloaded, offline) { - if (downloadStatus.downloaded > 0) { - hadDownloads = true - } else if (hadDownloads && offline) { - onBack() - } - } + CloseWhenDownloadsGone(downloadStatus.downloaded, offline, onBack) val albumDownloads by viewModel.albumDownloads.collectAsStateWithLifecycle() val scope = rememberCoroutineScope() var pendingDelete by remember { mutableStateOf(null) } diff --git a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt index 7b0eaa65..d45681b5 100644 --- a/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/library/PlaylistDetailScreen.kt @@ -22,6 +22,7 @@ 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.CloseWhenDownloadsGone import com.subtracks.ui.components.ContextMenuHost import com.subtracks.ui.components.DeleteDownloadsDialog import com.subtracks.ui.components.HeroDetailScaffold @@ -53,14 +54,7 @@ fun PlaylistDetailRoute( val downloads by viewModel.downloads.collectAsStateWithLifecycle() val downloadStatus by viewModel.downloadStatus.collectAsStateWithLifecycle() val offline by viewModel.offline.collectAsStateWithLifecycle() - var hadDownloads by remember { mutableStateOf(false) } - LaunchedEffect(downloadStatus.downloaded, offline) { - if (downloadStatus.downloaded > 0) { - hadDownloads = true - } else if (hadDownloads && offline) { - onBack() - } - } + CloseWhenDownloadsGone(downloadStatus.downloaded, offline, onBack) val scope = rememberCoroutineScope() var pendingDelete by remember { mutableStateOf(null) } val playback by playbackController.state.collectAsStateWithLifecycle() diff --git a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt index ef73ac1f..3fc276a5 100644 --- a/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt +++ b/app/src/main/kotlin/com/subtracks/ui/playback/QueueScreen.kt @@ -74,10 +74,9 @@ private const val POSITION_TIMEOUT_MS = 1_000L internal const val QUEUE_WINDOW_ROWS = QUEUE_CHUNK * 3 -// Offline the view hides non-downloaded rows, so resolving a contiguous position range can span far -// more than it shows. Cap the positions resolved per load (and per reconcile) so a sparse large -// playlist cannot make the view resolve the whole queue; online every position is shown, so the cap -// is never reached. +// Offline the view hides non-downloaded rows, so a displayed window can span far more positions than +// it shows. Reconcile resolves only this many positions from the head and keeps the rest on screen, +// so a sparse large playlist cannot make a layout change resolve the whole queue. private const val MAX_SCAN_POSITIONS = QUEUE_WINDOW_ROWS * 4 data class QueueRow( @@ -169,11 +168,21 @@ class QueueViewModel( val loaded = queueRepository.range(snapshot, start, end).filter(::visible) val reused = reusedIds() val updated = loaded.map { rowFor(it, reused) } - if (updated.size == rows.size) { - updated.forEachIndexed { index, row -> if (rows[index] != row) rows[index] = row } + if (end >= rawEnd) { + if (updated.size == rows.size) { + updated.forEachIndexed { index, row -> if (rows[index] != row) rows[index] = row } + } else { + rows.clear() + rows.addAll(updated) + } } else { + // Only the head of a sparse offline window was resolved; keep the tail on screen, + // dropping any tail row whose id was reused for a head row. + val usedIds = updated.mapTo(HashSet()) { it.id } + val tail = rows.filter { it.position > end && it.id !in usedIds } rows.clear() rows.addAll(updated) + rows.addAll(tail) } } @@ -187,11 +196,9 @@ class QueueViewModel( val snapshot = queueRepository.snapshot() val collected = mutableListOf() var end = first - 1 - var scanned = 0L - while (end >= 0L && collected.size < QUEUE_CHUNK && scanned < MAX_SCAN_POSITIONS) { + while (end >= 0L && collected.size < QUEUE_CHUNK) { val start = (end - QUEUE_CHUNK + 1).coerceAtLeast(0) collected.addAll(0, queueRepository.range(snapshot, start, end).filter(::visible)) - scanned += end - start + 1 end = start - 1 } if (collected.isEmpty()) return@withLock @@ -210,11 +217,9 @@ class QueueViewModel( if (last >= snapshot.size - 1) return@withLock val collected = mutableListOf() var start = last + 1 - var scanned = 0L - while (start < snapshot.size && collected.size < QUEUE_CHUNK && scanned < MAX_SCAN_POSITIONS) { + while (start < snapshot.size && collected.size < QUEUE_CHUNK) { val to = (start + QUEUE_CHUNK - 1).coerceAtMost(snapshot.size - 1) collected.addAll(queueRepository.range(snapshot, start, to).filter(::visible)) - scanned += to - start + 1 start = to + 1 } if (collected.isEmpty()) return@withLock 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 44a979af..0c115f24 100644 --- a/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt +++ b/app/src/test/kotlin/com/subtracks/data/download/FakeDownloadEngine.kt @@ -9,6 +9,7 @@ class FakeDownloadEngine : DownloadEngine { var cancelCalls = 0 var enqueueDelayMs = 0L var failEnqueueFor: String? = null + var failCancelOnEmpty = false override fun enqueue(request: EngineRequest): Long { if (enqueueDelayMs > 0) Thread.sleep(enqueueDelayMs) @@ -23,6 +24,7 @@ class FakeDownloadEngine : DownloadEngine { override fun download(id: Long): EngineDownload? = downloads[id] override fun cancel(ids: List) { + if (ids.isEmpty() && failCancelOnEmpty) throw IllegalArgumentException("no ids") cancelCalls++ cancelled += ids } 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 63806bfc..0c97abfc 100644 --- a/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/repo/DownloadRepositoryTest.kt @@ -362,6 +362,23 @@ class DownloadRepositoryTest { assertTrue(engine.cancelled.isNotEmpty()) } + @Test + fun cancellingQueuedRowsWithoutEngineIdsLeavesTheEngineAlone() = + runTest { + seedLibrary() + runBlocking { + db.downloadDao().upsert(SongDownload(1, "s1", DownloadStatus.Queued)) + db.downloadDao().upsert(SongDownload(1, "s2", DownloadStatus.Queued)) + } + // DownloadManager.remove throws on an empty id list; a queue of only waiting rows has + // no engine ids, so cancelling must not call the engine at all. + engine.failCancelOnEmpty = true + + runBlocking { repository.cancelActive() } + + assertTrue(allRows().isEmpty()) + } + @Test fun theDownloadStartsBeforeItsArtworkIsFetched() = runTest { diff --git a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt index abb6e1e6..d01b15e6 100644 --- a/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt +++ b/app/src/test/kotlin/com/subtracks/data/sync/SyncManagerTest.kt @@ -224,6 +224,7 @@ class SyncManagerTest { "" + "" } + @Test fun aSyncRequestWhileOfflineIsRefusedWithoutTouchingTheNetwork() = runBlocking {