music: improve indexer state management

Improve Indexer's state management by splitting up the current loading state and the last response. This is intended to resolve a bug where if the UI task and IndexerService are both killed, the Indexer state would become indeterminate and the library would not show. Resolve this by keeping track of whatever the last completed state was and falling back to it whenever the loading process is canceled.
2022-06-04 10:12:31 -06:00 · 2022-06-04 10:12:31 -06:00 · 09392ef381
commit 09392ef381
parent 08caa01dca
10 changed files with 165 additions and 88 deletions
--- 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
--- 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<FragmentHomeBinding>(), 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,25 +322,28 @@ class HomeFragment : ViewBindingFragment<FragmentHomeBinding>(), 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
-            }
-        } else {
+        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
+                }
+            }
+        }
    }

    private fun handleNavigation(item: Music?) {
--- 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<Callback>()

    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<Song>
    }

--- 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"
    }
--- 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

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

 /**
--- a/app/src/main/java/org/oxycblt/auxio/music/backend/IndexerUtil.kt
+++ b/app/src/main/java/org/oxycblt/auxio/music/backend/IndexerUtil.kt
@ -15,7 +15,7 @@
 * along with this program.  If not, see <https://www.gnu.org/licenses/>.
 */
 
-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
-        }
-
-        if (startsWith('(') && endsWith(')')) {
+        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(')') -> {

                    // 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)
                    }
-                ?: this
+                }
+                // Current name is fine
+                else -> null
            }

-        // Current name is fine.
-        return this
+        return newName ?: this
    }

 /**
--- 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<Future<TrackGroupArray>?> = 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<Song> {
        // 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)
--- 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<Song> {
        // 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
--- 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
 */