From 53870cd31b4b1a0798739da6c95c43311a411893 Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Mon, 1 Jan 2024 14:36:26 -0700 Subject: [PATCH] music: fix reloads not cancelling prior ones Caused by a dumb mistake in the cancellation code. --- .../oxycblt/auxio/music/MusicRepository.kt | 27 ++++++++++++------- .../auxio/music/system/IndexerService.kt | 6 ++--- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt index 026054e43..17a8ca671 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicRepository.kt @@ -29,8 +29,10 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.async import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeout import kotlinx.coroutines.yield import org.oxycblt.auxio.music.cache.CacheRepository import org.oxycblt.auxio.music.device.DeviceLibrary @@ -341,11 +343,11 @@ constructor( } override fun index(worker: MusicRepository.IndexingWorker, withCache: Boolean) = - worker.scope.launch { indexWrapper(worker, withCache) } + worker.scope.launch { indexWrapper(worker.context, this, withCache) } - private suspend fun indexWrapper(worker: MusicRepository.IndexingWorker, withCache: Boolean) { + private suspend fun indexWrapper(context: Context, scope: CoroutineScope, withCache: Boolean) { try { - indexImpl(worker, withCache) + indexImpl(context, scope, withCache) } catch (e: CancellationException) { // Got cancelled, propagate upwards to top-level co-routine. logD("Loading routine was cancelled") @@ -359,13 +361,13 @@ constructor( } } - private suspend fun indexImpl(worker: MusicRepository.IndexingWorker, withCache: Boolean) { + private suspend fun indexImpl(context: Context, scope: CoroutineScope, withCache: Boolean) { // TODO: Find a way to break up this monster of a method, preferably as another class. val start = System.currentTimeMillis() // Make sure we have permissions before going forward. Theoretically this would be better // done at the UI level, but that intertwines logic and display too much. - if (ContextCompat.checkSelfPermission(worker.context, PERMISSION_READ_AUDIO) == + if (ContextCompat.checkSelfPermission(context, PERMISSION_READ_AUDIO) == PackageManager.PERMISSION_DENIED) { logE("Permissions were not granted") throw NoAudioPermissionException() @@ -390,7 +392,7 @@ constructor( emitIndexingProgress(IndexingProgress.Indeterminate) val mediaStoreQueryJob = - worker.scope.async { + scope.async { val query = try { mediaStoreExtractor.query(constraints) @@ -428,7 +430,7 @@ constructor( // does not exist. In the latter situation, it also applies it's own (inferior) metadata. logD("Starting MediaStore discovery") val mediaStoreJob = - worker.scope.async { + scope.async { try { mediaStoreExtractor.consume(query, cache, incompleteSongs, completeSongs) } catch (e: Exception) { @@ -446,7 +448,7 @@ constructor( // metadata for them, and then forwards it to DeviceLibrary. logD("Starting tag extraction") val tagJob = - worker.scope.async { + scope.async { try { tagExtractor.consume(incompleteSongs, completeSongs) } catch (e: Exception) { @@ -461,7 +463,7 @@ constructor( // and then forwards them to the primary loading loop. logD("Starting DeviceLibrary creation") val deviceLibraryJob = - worker.scope.async(Dispatchers.Default) { + scope.async(Dispatchers.Default) { val deviceLibrary = try { deviceLibraryFactory.create( @@ -488,6 +490,11 @@ constructor( emitIndexingProgress(IndexingProgress.Songs(rawSongs.size, query.projectedTotal)) } + withTimeout(10000) { + mediaStoreJob.await() + tagJob.await() + } + // Deliberately done after the involved initialization step to make it less likely // that the short-circuit occurs so quickly as to break the UI. // TODO: Do not error, instead just wipe the entire library. @@ -510,7 +517,7 @@ constructor( // working on parent information. logD("Starting UserLibrary query") val userLibraryQueryJob = - worker.scope.async { + scope.async { val rawPlaylists = try { userLibraryFactory.query() diff --git a/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt index 37b32f8ef..d3cad75c6 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/system/IndexerService.kt @@ -32,7 +32,6 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job -import kotlinx.coroutines.launch import org.oxycblt.auxio.BuildConfig import org.oxycblt.auxio.music.IndexingProgress import org.oxycblt.auxio.music.IndexingState @@ -124,12 +123,11 @@ class IndexerService : // --- CONTROLLER CALLBACKS --- override fun requestIndex(withCache: Boolean) { - logD("Starting new indexing job") + logD("Starting new indexing job (previous=${currentIndexJob?.hashCode()})") // Cancel the previous music loading job. currentIndexJob?.cancel() // Start a new music loading job on a co-routine. - currentIndexJob = - indexScope.launch { musicRepository.index(this@IndexerService, withCache) } + currentIndexJob = musicRepository.index(this@IndexerService, withCache) } override val context = this