diff --git a/CHANGELOG.md b/CHANGELOG.md index 460fe5e95..f27c5b614 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## dev [v2.3.2, v2.4.0, or v3.0.0] +#### Dev/Meta +- Moved music loading to a foreground service + ## v2.3.1 #### What's Improved diff --git a/app/src/main/java/org/oxycblt/auxio/home/HomeFragment.kt b/app/src/main/java/org/oxycblt/auxio/home/HomeFragment.kt index 07939303e..5b864b315 100644 --- a/app/src/main/java/org/oxycblt/auxio/home/HomeFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/home/HomeFragment.kt @@ -57,6 +57,7 @@ import org.oxycblt.auxio.util.launch import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.logE import org.oxycblt.auxio.util.logTraceOrThrow +import org.oxycblt.auxio.util.logW import org.oxycblt.auxio.util.systemBarInsetsCompat import org.oxycblt.auxio.util.textSafe import org.oxycblt.auxio.util.unlikelyToBeNull @@ -260,10 +261,12 @@ class HomeFragment : ViewBindingFragment(), Toolbar.OnMenuI private fun handleIndexerState(state: Indexer.State?) { val binding = requireBinding() - if (state is Indexer.State.Complete) { - handleLoaderResponse(binding, state.response) - } else { - handleLoadingState(binding, state) + when (state) { + is Indexer.State.Complete -> handleLoaderResponse(binding, state.response) + is Indexer.State.Loading -> handleLoadingState(binding, state.loading) + null -> { + logW("Loading is in indeterminate state, doing nothing") + } } } @@ -319,24 +322,27 @@ class HomeFragment : ViewBindingFragment(), Toolbar.OnMenuI } } - private fun handleLoadingState(binding: FragmentHomeBinding, event: Indexer.State?) { + private fun handleLoadingState(binding: FragmentHomeBinding, loading: Indexer.Loading) { binding.homeFab.hide() binding.homePager.visibility = View.INVISIBLE binding.homeLoadingContainer.visibility = View.VISIBLE binding.homeLoadingProgress.visibility = View.VISIBLE binding.homeLoadingAction.visibility = View.INVISIBLE - if (event is Indexer.State.Loading) { - binding.homeLoadingStatus.textSafe = - getString(R.string.fmt_indexing, event.current, event.total) - binding.homeLoadingProgress.apply { - isIndeterminate = false - max = event.total - progress = event.current + when (loading) { + is Indexer.Loading.Indeterminate -> { + binding.homeLoadingStatus.textSafe = getString(R.string.lbl_loading) + binding.homeLoadingProgress.isIndeterminate = true + } + is Indexer.Loading.Songs -> { + binding.homeLoadingStatus.textSafe = + getString(R.string.fmt_indexing, loading.current, loading.total) + binding.homeLoadingProgress.apply { + isIndeterminate = false + max = loading.total + progress = loading.current + } } - } else { - binding.homeLoadingStatus.textSafe = getString(R.string.lbl_loading) - binding.homeLoadingProgress.isIndeterminate = true } } diff --git a/app/src/main/java/org/oxycblt/auxio/music/Indexer.kt b/app/src/main/java/org/oxycblt/auxio/music/Indexer.kt index 1c94f1194..4cf8d6119 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/Indexer.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/Indexer.kt @@ -54,12 +54,17 @@ import org.oxycblt.auxio.util.logE * @author OxygenCobalt */ class Indexer { - private var state: State? = null + private var lastResponse: Response? = null + private var loadingState: Loading? = null + private var currentGeneration: Long = 0 private val callbacks = mutableListOf() fun addCallback(callback: Callback) { - callback.onIndexerStateChanged(state) + val currentState = + loadingState?.let { State.Loading(it) } ?: lastResponse?.let { State.Complete(it) } + + callback.onIndexerStateChanged(currentState) callbacks.add(callback) } @@ -75,7 +80,7 @@ class Indexer { PackageManager.PERMISSION_DENIED if (notGranted) { - emitState(State.Complete(Response.NoPerms), generation) + emitCompletion(Response.NoPerms, generation) return } @@ -98,9 +103,13 @@ class Indexer { Response.Err(e) } - emitState(State.Complete(response), generation) + emitCompletion(response, generation) } + /** + * Request that re-indexing should be done. This should be used by components that do not manage + * the indexing process to re-index music. + */ fun requestReindex() { for (callback in callbacks) { callback.onRequestReindex() @@ -116,17 +125,41 @@ class Indexer { fun cancelLast() { synchronized(this) { currentGeneration++ - emitState(null, currentGeneration) + emitLoading(null, currentGeneration) } } - private fun emitState(newState: State?, generation: Long) { + private fun emitLoading(loading: Loading?, generation: Long) { synchronized(this) { - if (currentGeneration == generation) { - state = newState - for (callback in callbacks) { - callback.onIndexerStateChanged(newState) - } + if (currentGeneration != generation) { + return + } + + loadingState = loading + + // If we have canceled the loading process, we want to revert to a previous completion + // whenever possible to prevent state inconsistency. + val state = + loadingState?.let { State.Loading(it) } ?: lastResponse?.let { State.Complete(it) } + + for (callback in callbacks) { + callback.onIndexerStateChanged(state) + } + } + } + + private fun emitCompletion(response: Response, generation: Long) { + synchronized(this) { + if (currentGeneration != generation) { + return + } + + lastResponse = response + loadingState = null + + val state = State.Complete(response) + for (callback in callbacks) { + callback.onIndexerStateChanged(state) } } } @@ -136,7 +169,7 @@ class Indexer { * calling this function. */ private fun indexImpl(context: Context, generation: Long): MusicStore.Library? { - emitState(State.Query, generation) + emitLoading(Loading.Indeterminate, generation) // Establish the backend to use when initially loading songs. val mediaStoreBackend = @@ -188,9 +221,7 @@ class Indexer { "Successfully queried media database " + "in ${System.currentTimeMillis() - start}ms") - backend.loadSongs(context, cursor) { count, total -> - emitState(State.Loading(count, total), generation) - } + backend.loadSongs(context, cursor) { loading -> emitLoading(loading, generation) } } // Deduplicate songs to prevent (most) deformed music clones @@ -304,11 +335,15 @@ class Indexer { /** Represents the current indexer state. */ sealed class State { - object Query : State() - data class Loading(val current: Int, val total: Int) : State() + data class Loading(val loading: Indexer.Loading) : State() data class Complete(val response: Response) : State() } + sealed class Loading { + object Indeterminate : Loading() + class Songs(val current: Int, val total: Int) : Loading() + } + /** Represents the possible outcomes of a loading process. */ sealed class Response { data class Ok(val library: MusicStore.Library) : Response() @@ -318,6 +353,14 @@ class Indexer { } interface Callback { + /** + * Called when the current state of the Indexer changed. + * + * Notes: + * - Null means that no loading is going on, but no load has completed either. + * - [State.Complete] may represent a previous load, if the current loading process was + * canceled for one reason or another. + */ fun onIndexerStateChanged(state: State?) fun onRequestReindex() {} } @@ -331,7 +374,7 @@ class Indexer { fun loadSongs( context: Context, cursor: Cursor, - onAddSong: (count: Int, total: Int) -> Unit + emitLoading: (Loading) -> Unit ): Collection } diff --git a/app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt b/app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt index 746eb3999..b4545bd7e 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/IndexerService.kt @@ -78,19 +78,20 @@ class IndexerService : Service(), Indexer.Callback { override fun onDestroy() { super.onDestroy() - stopForeground(true) - isForeground = false - - indexer.removeCallback(this) + // cancelLast actually stops foreground for us as it updates the loading state to + // null or completed. indexer.cancelLast() + indexer.removeCallback(this) serviceJob.cancel() } override fun onIndexerStateChanged(state: Indexer.State?) { when (state) { is Indexer.State.Complete -> { - if (state.response is Indexer.Response.Ok) { - // Load was completed successfully, so apply the new library. + if (state.response is Indexer.Response.Ok && musicStore.library == null) { + // Load was completed successfully, so apply the new library if we + // have not already. + // TODO: Change null check for equality check [automatic rescanning] musicStore.library = state.response.library } @@ -104,12 +105,20 @@ class IndexerService : Service(), Indexer.Callback { // database. stopForegroundSession() } - is Indexer.State.Query, is Indexer.State.Loading -> { - // We are loading, so we want to the enter the foreground state so that android - // does not kill our app. Note that while we would prefer to display the current - // loading progress, updates tend to be too rapid-fire for it too work well. - startForegroundSession() + // When loading, we want to enter the foreground state so that android does + // not shut off the loading process. Note that while we will always post the + // notification when initially starting, we will not update the notification + // unless it indicates that we have changed it. + val changed = notification.updateLoadingState(state.loading) + if (!isForeground) { + logD("Starting foreground session") + startForeground(IntegerTable.INDEXER_NOTIFICATION_CODE, notification.build()) + isForeground = true + } else if (changed) { + logD("Notification changed, re-posting notification") + notification.renotify() + } } null -> { // Null is the indeterminate state that occurs on app startup or after @@ -124,14 +133,6 @@ class IndexerService : Service(), Indexer.Callback { indexScope.launch { indexer.index(this@IndexerService) } } - private fun startForegroundSession() { - if (!isForeground) { - logD("Starting foreground service") - startForeground(IntegerTable.INDEXER_NOTIFICATION_CODE, notification.build()) - isForeground = true - } - } - private fun stopForegroundSession() { if (isForeground) { stopForeground(true) @@ -140,7 +141,7 @@ class IndexerService : Service(), Indexer.Callback { } } -private class IndexerNotification(context: Context) : +private class IndexerNotification(private val context: Context) : NotificationCompat.Builder(context, CHANNEL_ID) { private val notificationManager = context.getSystemServiceSafe(NotificationManager::class) @@ -166,6 +167,31 @@ private class IndexerNotification(context: Context) : setProgress(0, 0, true) } + fun renotify() { + notificationManager.notify(IntegerTable.INDEXER_NOTIFICATION_CODE, build()) + } + + fun updateLoadingState(loading: Indexer.Loading): Boolean { + when (loading) { + is Indexer.Loading.Indeterminate -> { + setContentText(context.getString(R.string.lbl_loading)) + setProgress(0, 0, true) + return true + } + is Indexer.Loading.Songs -> { + // Only update the notification every 50 songs to prevent excessive updates. + if (loading.current % 50 == 0) { + setContentText( + context.getString(R.string.fmt_indexing, loading.current, loading.total)) + setProgress(loading.total, loading.current, false) + return true + } + } + } + + return false + } + companion object { const val CHANNEL_ID = BuildConfig.APPLICATION_ID + ".channel.INDEXER" } diff --git a/app/src/main/java/org/oxycblt/auxio/music/Music.kt b/app/src/main/java/org/oxycblt/auxio/music/Music.kt index 9e0ed3f66..964161d7d 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/Music.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/Music.kt @@ -23,8 +23,6 @@ import android.content.Context import android.net.Uri import android.provider.MediaStore import org.oxycblt.auxio.R -import org.oxycblt.auxio.music.backend.id3v2GenreName -import org.oxycblt.auxio.music.backend.withoutArticle import org.oxycblt.auxio.ui.Item import org.oxycblt.auxio.util.unlikelyToBeNull diff --git a/app/src/main/java/org/oxycblt/auxio/music/MusicStore.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicStore.kt index b249f6d16..8ce615090 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/MusicStore.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicStore.kt @@ -20,7 +20,6 @@ package org.oxycblt.auxio.music import android.content.Context import android.net.Uri import android.provider.OpenableColumns -import org.oxycblt.auxio.music.backend.useQuery import org.oxycblt.auxio.util.contentResolverSafe /** diff --git a/app/src/main/java/org/oxycblt/auxio/music/backend/IndexerUtil.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicUtil.kt similarity index 89% rename from app/src/main/java/org/oxycblt/auxio/music/backend/IndexerUtil.kt rename to app/src/main/java/org/oxycblt/auxio/music/MusicUtil.kt index 33a53524c..500ed2709 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/backend/IndexerUtil.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicUtil.kt @@ -15,7 +15,7 @@ * along with this program. If not, see . */ -package org.oxycblt.auxio.music.backend +package org.oxycblt.auxio.music import android.content.ContentResolver import android.content.ContentUris @@ -98,27 +98,27 @@ val String.withoutArticle: String */ val String.id3v2GenreName: String get() { - if (isDigitsOnly()) { - // ID3v1, just parse as an integer - return genreConstantTable.getOrNull(toInt()) ?: this - } + val newName = + when { + // ID3v1, should just be digits + isDigitsOnly() -> genreConstantTable.getOrNull(toInt()) + // ID3v2.3/ID3v2.4, parse out the parentheses and get the integer + // Any genres formatted as "(CHARS)" will be ignored. + startsWith('(') && endsWith(')') -> { - if (startsWith('(') && endsWith(')')) { - // ID3v2.3/ID3v2.4, parse out the parentheses and get the integer - // Any genres formatted as "(CHARS)" will be ignored. + // TODO: Technically, the spec for genres is far more complex here. Perhaps we + // should copy mutagen's implementation? + // https://github.com/quodlibet/mutagen/blob/master/mutagen/id3/_frames.py - // TODO: Technically, the spec for genres is far more complex here. Perhaps we - // should copy mutagen's implementation? - // https://github.com/quodlibet/mutagen/blob/master/mutagen/id3/_frames.py - - return substring(1 until lastIndex).toIntOrNull()?.run { - genreConstantTable.getOrNull(this) + substring(1 until lastIndex).toIntOrNull()?.let { + genreConstantTable.getOrNull(it) + } + } + // Current name is fine + else -> null } - ?: this - } - // Current name is fine. - return this + return newName ?: this } /** diff --git a/app/src/main/java/org/oxycblt/auxio/music/backend/ExoPlayerBackend.kt b/app/src/main/java/org/oxycblt/auxio/music/backend/ExoPlayerBackend.kt index 3d40d2afc..fc75301d3 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/backend/ExoPlayerBackend.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/backend/ExoPlayerBackend.kt @@ -33,10 +33,13 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.asExecutor import org.oxycblt.auxio.music.Indexer import org.oxycblt.auxio.music.Song +import org.oxycblt.auxio.music.audioUri +import org.oxycblt.auxio.music.iso8601year +import org.oxycblt.auxio.music.no import org.oxycblt.auxio.util.logW /** - * A [OldIndexer.Backend] that leverages ExoPlayer's metadata retrieval system to index metadata. + * A [Indexer.Backend] that leverages ExoPlayer's metadata retrieval system to index metadata. * * Normally, leveraging ExoPlayer's metadata system would be a terrible idea, as it is horrifically * slow. However, if we parallelize it, we can get similar throughput to other metadata extractors, @@ -46,12 +49,8 @@ import org.oxycblt.auxio.util.logW * pitfalls given ExoPlayer's cozy relationship with native code. However, this backend should do * enough to eliminate such issues. * - * TODO: This class is currently not used, as there are a number of technical improvements that must - * be made first before it can be integrated. - * * @author OxygenCobalt */ -@Suppress("UNUSED") class ExoPlayerBackend(private val inner: MediaStoreBackend) : Indexer.Backend { private val runningTasks: Array?> = arrayOfNulls(TASK_CAPACITY) @@ -62,7 +61,7 @@ class ExoPlayerBackend(private val inner: MediaStoreBackend) : Indexer.Backend { override fun loadSongs( context: Context, cursor: Cursor, - onAddSong: (count: Int, total: Int) -> Unit + emitLoading: (Indexer.Loading) -> Unit ): Collection { // Metadata retrieval with ExoPlayer is asynchronous, so a callback may at any point // add a completed song to the list. To prevent a crash in that case, we use the @@ -90,7 +89,7 @@ class ExoPlayerBackend(private val inner: MediaStoreBackend) : Indexer.Backend { AudioCallback(audio) { runningTasks[index] = null songs.add(it) - onAddSong(songs.size, cursor.count) + emitLoading(Indexer.Loading.Songs(songs.size, cursor.count)) }, // Normal JVM dispatcher will suffice here, as there is no IO work // going on (and there is no cost from switching contexts with executors) diff --git a/app/src/main/java/org/oxycblt/auxio/music/backend/MediaStoreBackend.kt b/app/src/main/java/org/oxycblt/auxio/music/backend/MediaStoreBackend.kt index 7c189d76d..4d356f38b 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/backend/MediaStoreBackend.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/backend/MediaStoreBackend.kt @@ -26,7 +26,12 @@ import androidx.core.database.getIntOrNull import androidx.core.database.getStringOrNull import org.oxycblt.auxio.music.Indexer import org.oxycblt.auxio.music.Song +import org.oxycblt.auxio.music.albumCoverUri +import org.oxycblt.auxio.music.audioUri import org.oxycblt.auxio.music.excluded.ExcludedDatabase +import org.oxycblt.auxio.music.no +import org.oxycblt.auxio.music.queryCursor +import org.oxycblt.auxio.music.useQuery import org.oxycblt.auxio.util.contentResolverSafe /* @@ -88,9 +93,8 @@ import org.oxycblt.auxio.util.contentResolverSafe */ /** - * Represents a [OldIndexer.Backend] that loads music from the media database ([MediaStore]). This - * is not a fully-featured class by itself, and it's API-specific derivatives should be used - * instead. + * Represents a [Indexer.Backend] that loads music from the media database ([MediaStore]). This is + * not a fully-featured class by itself, and it's API-specific derivatives should be used instead. * @author OxygenCobalt */ abstract class MediaStoreBackend : Indexer.Backend { @@ -130,7 +134,7 @@ abstract class MediaStoreBackend : Indexer.Backend { override fun loadSongs( context: Context, cursor: Cursor, - onAddSong: (count: Int, total: Int) -> Unit + emitLoading: (Indexer.Loading) -> Unit ): Collection { // Note: We do not actually update the callback with an Indexing state, this is because // loading music from MediaStore tends to be quite fast, with the only bottlenecks being diff --git a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt index 4182327cc..3cb83aaa7 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackViewModel.kt @@ -40,9 +40,8 @@ import org.oxycblt.auxio.util.unlikelyToBeNull /** * The ViewModel that provides a UI frontend for [PlaybackStateManager]. * - * **PLEASE Use this instead of [PlaybackStateManager], UIs are extremely volatile and this - * provides an interface that properly sanitizes input and abstracts functions unlike the master - * class.** + * **PLEASE Use this instead of [PlaybackStateManager], UIs are extremely volatile and this provides + * an interface that properly sanitizes input and abstracts functions unlike the master class.** * * @author OxygenCobalt */