From 940746f24839b9e11b956aae5e67f4eef36be259 Mon Sep 17 00:00:00 2001
From: OxygenCobalt <oxycblt@abstractr.net>
Date: Thu, 24 Dec 2020 11:30:23 -0700
Subject: [PATCH] Make state restore use names instead of ids

Make it so that PlaybackStateDatabase stores the item data for PlaybackState/QueueItems as strings instead of ids, as ids are too volitaile if the music library changes.
---
 .github/CONTRIBUTING.md                       |  2 +-
 .github/ISSUE_TEMPLATE/bug-crash-report.md    |  3 +-
 app/build.gradle                              |  8 +--
 .../oxycblt/auxio/database/PlaybackState.kt   | 12 ++---
 .../auxio/database/PlaybackStateDatabase.kt   | 53 ++++++++++++-------
 .../org/oxycblt/auxio/database/QueueItem.kt   | 12 ++---
 .../auxio/music/processing/MusicLoader.kt     |  1 +
 .../playback/state/PlaybackStateManager.kt    | 31 +++++------
 .../recycler/viewholders/BaseViewHolder.kt    |  6 +--
 info/ADDITIONS.md                             |  2 +-
 10 files changed, 75 insertions(+), 55 deletions(-)

diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md
index a309fe00b..d00833a39 100644
--- a/.github/CONTRIBUTING.md
+++ b/.github/CONTRIBUTING.md
@@ -26,8 +26,8 @@ Please keep in mind when requesting a feature:
 - **Will it be accepted?** Read the [Accepted Additions and Requests](../info/ADDITIONS.md) in order to see the likelihood that your request will be implemented.
 
 If you do make a request, provide the following:
-- What kind of addition is this? (A Full Feature? A new customization option? A UI Change?)
 - What is it that you want?
+- Is it related to some problem? If so, describe why.
 - Why do you think it will benefit everyone's usage of the app?
 
 If you have the knowledge, you can also implement the feature yourself and create a [Pull Request](https://github.com/OxygenCobalt/Auxio/pulls), but its recommended that **you create an issue beforehand to give me a heads up.**
diff --git a/.github/ISSUE_TEMPLATE/bug-crash-report.md b/.github/ISSUE_TEMPLATE/bug-crash-report.md
index 0cb8be30f..1bc8fbea3 100644
--- a/.github/ISSUE_TEMPLATE/bug-crash-report.md
+++ b/.github/ISSUE_TEMPLATE/bug-crash-report.md
@@ -7,8 +7,9 @@ assignees: ''
 
 ---
 
-#### Describe the bug/crash
+#### Describe the bug/crash:
 <!-- A clear and concise description of what the bug or crash is. -->
+
 #### Steps To Reproduce the bug/crash:
 <!--
 1. Go to X
diff --git a/app/build.gradle b/app/build.gradle
index fef33fd8b..2e69f1b8d 100644
--- a/app/build.gradle
+++ b/app/build.gradle
@@ -63,8 +63,8 @@ dependencies {
 
     // General
     implementation 'androidx.core:core-ktx:1.3.2'
-    implementation 'androidx.activity:activity-ktx:1.2.0-beta02'
-    implementation 'androidx.fragment:fragment-ktx:1.3.0-beta02'
+    implementation 'androidx.activity:activity-ktx:1.2.0-rc01'
+    implementation 'androidx.fragment:fragment-ktx:1.3.0-rc01'
 
     // Layout
     implementation 'androidx.constraintlayout:constraintlayout:2.0.4'
@@ -100,7 +100,7 @@ dependencies {
     implementation 'io.coil-kt:coil:0.13.0'
 
     // Material
-    implementation 'com.google.android.material:material:1.3.0-alpha04'
+    implementation 'com.google.android.material:material:1.3.0-beta01'
 
     // Fast-Scroll
     implementation 'com.reddit:indicator-fast-scroll:1.3.0'
@@ -111,7 +111,7 @@ dependencies {
     // --- DEV ---
 
     // Lint
-    ktlint "com.pinterest:ktlint:0.37.2"
+    ktlint "com.pinterest:ktlint:0.40.0"
 
     // Memory Leak checking
     debugImplementation 'com.squareup.leakcanary:leakcanary-android:2.5'
diff --git a/app/src/main/java/org/oxycblt/auxio/database/PlaybackState.kt b/app/src/main/java/org/oxycblt/auxio/database/PlaybackState.kt
index dc65094bb..9685580a9 100644
--- a/app/src/main/java/org/oxycblt/auxio/database/PlaybackState.kt
+++ b/app/src/main/java/org/oxycblt/auxio/database/PlaybackState.kt
@@ -3,8 +3,8 @@ package org.oxycblt.auxio.database
 /**
  * A database entity that stores a compressed variant of the current playback state.
  * @property id - The database key for this state
- * @property songId - The song that is currently playing
- * @property parentId - The parent that is being played from [-1 if none]
+ * @property songName - The song that is currently playing
+ * @property parentName - The parent that is being played from [-1 if none]
  * @property index - The current index in the queue.
  * @property mode - The integer form of the current [org.oxycblt.auxio.playback.state.PlaybackMode]
  * @property isShuffling - A bool for if the queue was shuffled
@@ -14,9 +14,9 @@ package org.oxycblt.auxio.database
  */
 data class PlaybackState(
     val id: Long = 0L,
-    val songId: Long = -1L,
+    val songName: String = "",
     val position: Long,
-    val parentId: Long = -1L,
+    val parentName: String = "",
     val index: Int,
     val mode: Int,
     val isShuffling: Boolean,
@@ -25,9 +25,9 @@ data class PlaybackState(
 ) {
     companion object {
         const val COLUMN_ID = "state_id"
-        const val COLUMN_SONG_ID = "song_id"
+        const val COLUMN_SONG_NAME = "song_name"
         const val COLUMN_POSITION = "position"
-        const val COLUMN_PARENT_ID = "parent_id"
+        const val COLUMN_PARENT_NAME = "parent_name"
         const val COLUMN_INDEX = "state_index"
         const val COLUMN_MODE = "mode"
         const val COLUMN_IS_SHUFFLING = "is_shuffling"
diff --git a/app/src/main/java/org/oxycblt/auxio/database/PlaybackStateDatabase.kt b/app/src/main/java/org/oxycblt/auxio/database/PlaybackStateDatabase.kt
index 4e178350d..0ae16091f 100644
--- a/app/src/main/java/org/oxycblt/auxio/database/PlaybackStateDatabase.kt
+++ b/app/src/main/java/org/oxycblt/auxio/database/PlaybackStateDatabase.kt
@@ -5,12 +5,15 @@ import android.content.Context
 import android.database.Cursor
 import android.database.sqlite.SQLiteDatabase
 import android.database.sqlite.SQLiteOpenHelper
+import android.os.Looper
+import androidx.core.database.getStringOrNull
 import org.oxycblt.auxio.logD
 
 /**
  * A SQLite database for managing the persistent playback state and queue.
  * Yes, I know androidx has Room which supposedly makes database creation easier, but it also
  * has a crippling bug where it will endlessly allocate rows even if you clear the entire db, so...
+ * TODO: Turn queue loading info flow
  * @author OxygenCobalt
  */
 class PlaybackStateDatabase(context: Context) :
@@ -53,9 +56,9 @@ class PlaybackStateDatabase(context: Context) :
      */
     private fun constructStateTable(command: StringBuilder): StringBuilder {
         command.append("${PlaybackState.COLUMN_ID} LONG PRIMARY KEY,")
-        command.append("${PlaybackState.COLUMN_SONG_ID} LONG NOT NULL,")
+        command.append("${PlaybackState.COLUMN_SONG_NAME} STRING NOT NULL,")
         command.append("${PlaybackState.COLUMN_POSITION} LONG NOT NULL,")
-        command.append("${PlaybackState.COLUMN_PARENT_ID} LONG NOT NULL,")
+        command.append("${PlaybackState.COLUMN_PARENT_NAME} STRING NOT NULL,")
         command.append("${PlaybackState.COLUMN_INDEX} INTEGER NOT NULL,")
         command.append("${PlaybackState.COLUMN_MODE} INTEGER NOT NULL,")
         command.append("${PlaybackState.COLUMN_IS_SHUFFLING} BOOLEAN NOT NULL,")
@@ -70,8 +73,8 @@ class PlaybackStateDatabase(context: Context) :
      */
     private fun constructQueueTable(command: StringBuilder): StringBuilder {
         command.append("${QueueItem.COLUMN_ID} LONG PRIMARY KEY,")
-        command.append("${QueueItem.COLUMN_SONG_ID} LONG NOT NULL,")
-        command.append("${QueueItem.COLUMN_ALBUM_ID} LONG NOT NULL,")
+        command.append("${QueueItem.COLUMN_SONG_NAME} LONG NOT NULL,")
+        command.append("${QueueItem.COLUMN_ALBUM_NAME} LONG NOT NULL,")
         command.append("${QueueItem.COLUMN_IS_USER_QUEUE} BOOLEAN NOT NULL)")
 
         return command
@@ -83,6 +86,8 @@ class PlaybackStateDatabase(context: Context) :
      * Clear the previously written [PlaybackState] and write a new one.
      */
     fun writeState(state: PlaybackState) {
+        assertBackgroundThread()
+
         val database = writableDatabase
         database.beginTransaction()
 
@@ -101,9 +106,9 @@ class PlaybackStateDatabase(context: Context) :
             val stateData = ContentValues(9)
 
             stateData.put(PlaybackState.COLUMN_ID, state.id)
-            stateData.put(PlaybackState.COLUMN_SONG_ID, state.songId)
+            stateData.put(PlaybackState.COLUMN_SONG_NAME, state.songName)
             stateData.put(PlaybackState.COLUMN_POSITION, state.position)
-            stateData.put(PlaybackState.COLUMN_PARENT_ID, state.parentId)
+            stateData.put(PlaybackState.COLUMN_PARENT_NAME, state.parentName)
             stateData.put(PlaybackState.COLUMN_INDEX, state.index)
             stateData.put(PlaybackState.COLUMN_MODE, state.mode)
             stateData.put(PlaybackState.COLUMN_IS_SHUFFLING, state.isShuffling)
@@ -124,6 +129,8 @@ class PlaybackStateDatabase(context: Context) :
      * @return The stored [PlaybackState], null if there isn't one.
      */
     fun readState(): PlaybackState? {
+        assertBackgroundThread()
+
         val database = writableDatabase
 
         var state: PlaybackState? = null
@@ -140,9 +147,9 @@ class PlaybackStateDatabase(context: Context) :
                 // Don't bother if the cursor [and therefore database] has nothing in it.
                 if (cursor.count == 0) return@use
 
-                val songIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_SONG_ID)
+                val songIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_SONG_NAME)
                 val positionIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_POSITION)
-                val parentIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_PARENT_ID)
+                val parentIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_PARENT_NAME)
                 val indexIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_INDEX)
                 val modeIndex = cursor.getColumnIndexOrThrow(PlaybackState.COLUMN_MODE)
                 val isShufflingIndex =
@@ -155,9 +162,9 @@ class PlaybackStateDatabase(context: Context) :
                 cursor.moveToFirst()
 
                 state = PlaybackState(
-                    songId = cursor.getLong(songIndex),
+                    songName = cursor.getStringOrNull(songIndex) ?: "",
                     position = cursor.getLong(positionIndex),
-                    parentId = cursor.getLong(parentIndex),
+                    parentName = cursor.getStringOrNull(parentIndex) ?: "",
                     index = cursor.getInt(indexIndex),
                     mode = cursor.getInt(modeIndex),
                     isShuffling = cursor.getInt(isShufflingIndex) == 1,
@@ -175,6 +182,8 @@ class PlaybackStateDatabase(context: Context) :
      * @param queue The list of [QueueItem]s to be written.
      */
     fun writeQueue(queue: List<QueueItem>) {
+        assertBackgroundThread()
+
         val database = readableDatabase
         database.beginTransaction()
 
@@ -204,8 +213,8 @@ class PlaybackStateDatabase(context: Context) :
                     i++
 
                     itemData.put(QueueItem.COLUMN_ID, item.id)
-                    itemData.put(QueueItem.COLUMN_SONG_ID, item.songId)
-                    itemData.put(QueueItem.COLUMN_ALBUM_ID, item.albumId)
+                    itemData.put(QueueItem.COLUMN_SONG_NAME, item.songName)
+                    itemData.put(QueueItem.COLUMN_ALBUM_NAME, item.albumName)
                     itemData.put(QueueItem.COLUMN_IS_USER_QUEUE, item.isUserQueue)
 
                     database.insert(TABLE_NAME_QUEUE, null, itemData)
@@ -229,6 +238,8 @@ class PlaybackStateDatabase(context: Context) :
      * @return A list of any stored [QueueItem]s.
      */
     fun readQueue(): List<QueueItem> {
+        assertBackgroundThread()
+
         val database = readableDatabase
 
         val queueItems = mutableListOf<QueueItem>()
@@ -244,18 +255,18 @@ class PlaybackStateDatabase(context: Context) :
                 if (cursor.count == 0) return@use
 
                 val idIndex = cursor.getColumnIndexOrThrow(QueueItem.COLUMN_ID)
-                val songIdIndex = cursor.getColumnIndexOrThrow(QueueItem.COLUMN_SONG_ID)
-                val albumIdIndex = cursor.getColumnIndexOrThrow(QueueItem.COLUMN_ALBUM_ID)
+                val songIdIndex = cursor.getColumnIndexOrThrow(QueueItem.COLUMN_SONG_NAME)
+                val albumIdIndex = cursor.getColumnIndexOrThrow(QueueItem.COLUMN_ALBUM_NAME)
                 val isUserQueueIndex = cursor.getColumnIndexOrThrow(QueueItem.COLUMN_IS_USER_QUEUE)
 
                 while (cursor.moveToNext()) {
                     val id = cursor.getLong(idIndex)
-                    val songId = cursor.getLong(songIdIndex)
-                    val albumId = cursor.getLong(albumIdIndex)
+                    val songName = cursor.getStringOrNull(songIdIndex) ?: ""
+                    val albumName = cursor.getStringOrNull(albumIdIndex) ?: ""
                     val isUserQueue = cursor.getInt(isUserQueueIndex) == 1
 
                     queueItems.add(
-                        QueueItem(id, songId, albumId, isUserQueue)
+                        QueueItem(id, songName, albumName, isUserQueue)
                     )
                 }
             }
@@ -264,8 +275,14 @@ class PlaybackStateDatabase(context: Context) :
         }
     }
 
+    private fun assertBackgroundThread() {
+        if (Looper.myLooper() == Looper.getMainLooper()) {
+            error("Not on a background thread.")
+        }
+    }
+
     companion object {
-        const val DB_VERSION = 1
+        const val DB_VERSION = 2
         const val DB_NAME = "auxio_state_database"
 
         const val TABLE_NAME_STATE = "playback_state_table"
diff --git a/app/src/main/java/org/oxycblt/auxio/database/QueueItem.kt b/app/src/main/java/org/oxycblt/auxio/database/QueueItem.kt
index c394a3988..53d9d6a84 100644
--- a/app/src/main/java/org/oxycblt/auxio/database/QueueItem.kt
+++ b/app/src/main/java/org/oxycblt/auxio/database/QueueItem.kt
@@ -3,21 +3,21 @@ package org.oxycblt.auxio.database
 /**
  * A database entity that stores a simplified representation of a song in a queue.
  * @property id The database entity's id
- * @property songId The song id for this queue item
- * @property albumId The album id for this queue item, used to make searching quicker
+ * @property songName The song name for this queue item
+ * @property albumName The album name for this queue item, used to make searching quicker
  * @property isUserQueue A bool for if this queue item is a user queue item or not
  * @author OxygenCobalt
  */
 data class QueueItem(
     var id: Long = 0L,
-    val songId: Long = Long.MIN_VALUE,
-    val albumId: Long = Long.MIN_VALUE,
+    val songName: String = "",
+    val albumName: String = "",
     val isUserQueue: Boolean = false
 ) {
     companion object {
         const val COLUMN_ID = "id"
-        const val COLUMN_SONG_ID = "song_id"
-        const val COLUMN_ALBUM_ID = "album_id"
+        const val COLUMN_SONG_NAME = "song_name"
+        const val COLUMN_ALBUM_NAME = "album_name"
         const val COLUMN_IS_USER_QUEUE = "is_user_queue"
     }
 }
diff --git a/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt b/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt
index e86ed43e6..ee28a3f68 100644
--- a/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt
+++ b/app/src/main/java/org/oxycblt/auxio/music/processing/MusicLoader.kt
@@ -123,6 +123,7 @@ class MusicLoader(private val app: Application) {
                 val year = cursor.getInt(yearIndex)
                 val coverUri = id.toAlbumArtURI()
 
+                // Correct any artist names to a nicer "Unknown Artist" label
                 if (artistName == MediaStore.UNKNOWN_STRING) {
                     artistName = artistPlaceholder
                 }
diff --git a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt
index daf50c23f..a389cb28c 100644
--- a/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt
+++ b/app/src/main/java/org/oxycblt/auxio/playback/state/PlaybackStateManager.kt
@@ -667,15 +667,15 @@ class PlaybackStateManager private constructor() {
      * @return A [PlaybackState] reflecting the current state.
      */
     private fun packToPlaybackState(): PlaybackState {
-        val songId = mSong?.id ?: -1L
-        val parentId = mParent?.id ?: -1L
+        val songName = mSong?.name ?: ""
+        val parentName = mParent?.name ?: ""
         val intMode = mMode.toInt()
         val intLoopMode = mLoopMode.toInt()
 
         return PlaybackState(
-            songId = songId,
+            songName = songName,
             position = mPosition,
-            parentId = parentId,
+            parentName = parentName,
             index = mIndex,
             mode = intMode,
             isShuffling = mIsShuffling,
@@ -694,12 +694,12 @@ class PlaybackStateManager private constructor() {
         var queueItemId = 0L
 
         mUserQueue.forEach {
-            unified.add(QueueItem(queueItemId, it.id, it.albumId, true))
+            unified.add(QueueItem(queueItemId, it.name, it.album.name, true))
             queueItemId++
         }
 
         mQueue.forEach {
-            unified.add(QueueItem(queueItemId, it.id, it.albumId, false))
+            unified.add(QueueItem(queueItemId, it.name, it.album.name, false))
             queueItemId++
         }
 
@@ -714,9 +714,9 @@ class PlaybackStateManager private constructor() {
         val musicStore = MusicStore.getInstance()
 
         // Turn the simplified information from PlaybackState into values that can be used
-        mSong = musicStore.songs.find { it.id == playbackState.songId }
+        mSong = musicStore.songs.find { it.name == playbackState.songName }
         mPosition = playbackState.position
-        mParent = musicStore.parents.find { it.id == playbackState.parentId }
+        mParent = musicStore.parents.find { it.name == playbackState.parentName }
         mMode = PlaybackMode.fromInt(playbackState.mode) ?: PlaybackMode.ALL_SONGS
         mLoopMode = LoopMode.fromInt(playbackState.loopMode) ?: LoopMode.NONE
         mIsShuffling = playbackState.isShuffling
@@ -739,18 +739,19 @@ class PlaybackStateManager private constructor() {
 
         queueItems.forEach { item ->
             // Traverse albums and then album songs instead of just the songs, as its faster.
-            musicStore.albums.find { it.id == item.albumId }
-                ?.songs?.find { it.id == item.songId }?.let {
-                if (item.isUserQueue) {
-                    mUserQueue.add(it)
-                } else {
-                    mQueue.add(it)
+            musicStore.albums.find { it.name == item.albumName }
+                ?.songs?.find { it.name == item.songName }?.let {
+                    if (item.isUserQueue) {
+                        mUserQueue.add(it)
+                    } else {
+                        mQueue.add(it)
+                    }
                 }
-            }
         }
 
         // When done, get a more accurate index to prevent issues with queue songs that were saved
         // to the db but are now deleted when the restore occurred.
+        // Not done if in user queue because that could result in a bad index being created.
         if (!mIsInUserQueue) {
             mSong?.let {
                 val index = mQueue.indexOf(it)
diff --git a/app/src/main/java/org/oxycblt/auxio/recycler/viewholders/BaseViewHolder.kt b/app/src/main/java/org/oxycblt/auxio/recycler/viewholders/BaseViewHolder.kt
index cf98a88bf..34b77fd92 100644
--- a/app/src/main/java/org/oxycblt/auxio/recycler/viewholders/BaseViewHolder.kt
+++ b/app/src/main/java/org/oxycblt/auxio/recycler/viewholders/BaseViewHolder.kt
@@ -8,9 +8,9 @@ import org.oxycblt.auxio.music.BaseModel
 /**
  * A [RecyclerView.ViewHolder] that streamlines a lot of the common things across all viewholders.
  * @param T The datatype, inheriting [BaseModel] for this ViewHolder.
- * @property baseBinding Basic [ViewDataBinding] required to set up click listeners & sizing.
- * @property doOnClick Function that specifies what to do when an item is clicked. Specify null if you want no action to occur.
- * @property doOnLongClick Function that specifies what to do when an item is long clicked. Specify null if you want no action to occur.
+ * @param baseBinding Basic [ViewDataBinding] required to set up click listeners & sizing.
+ * @param doOnClick Function that specifies what to do when an item is clicked. Specify null if you want no action to occur.
+ * @param doOnLongClick Function that specifies what to do when an item is long clicked. Specify null if you want no action to occur.
  * @author OxygenCobalt
  */
 abstract class BaseViewHolder<T : BaseModel>(
diff --git a/info/ADDITIONS.md b/info/ADDITIONS.md
index 729f6a6c8..ccf046731 100644
--- a/info/ADDITIONS.md
+++ b/info/ADDITIONS.md
@@ -20,7 +20,7 @@ Overall I tend to accept these however if I see the benefits of adding this UI/B
 
 ## Feature Additions and UI Changes
 
-These are far less likely to be accepted/added. As I said, I want to avoid Auxio from becoming overly bloated with features I do not use, and therefore **I will only accept features/UI changes that directly benefit my own usage.** If they do not, then I will reject/ignore them. This does not rule out all additions of this kind, but I am generally less likely to accept these kinds of requests/PRs.
+These are far less likely to be accepted/added. As I said, I want to avoid Auxio from becoming overly bloated with features I do not use, and therefore **I only tend to accept features/UI changes that directly benefit my own usage.** If they do not, then I will reject them. This does not rule out all additions of this kind, but I am generally less likely to accept these kinds of requests/PRs.
 
 Feel free to fork Auxio to add your own features however.