music: add finalization routine

Combine validation into a new finalization routine that now
individually validates and in the future may finalize certain
fields that require linking to be properly created.

CHANGELOG.md

@@ -3,12 +3,14 @@
 ## dev
 
 #### What's New
+- Added support for songs with multiple genres
 - Reworked music hashing to be even more reliable (Will wipe playback state)
 
 #### What's Improved
 - Sorting now takes accented characters into account
 - Added support for compilation sub-release-types like (DJ) Mix
 - Album dates now start from the earliest date instead of latest date
+- Reshuffling the queue will no longer drop any songs you have added/removed
 
 #### What's Fixed
 - Fixed issue where the scroll popup would not display correctly in landscape mode [#230]

app/src/main/java/org/oxycblt/auxio/music/Music.kt

@@ -41,8 +41,6 @@ import java.util.UUID
 import kotlin.math.max
 import kotlin.math.min
 
-// TODO: Make empty parents a hard error
-
 // --- MUSIC MODELS ---
 
 /** [Item] variant that represents a music item. */
@@ -55,20 +53,31 @@ sealed class Music : Item {
     /** The raw sorting name of this item. Null if not present. */
     abstract val rawSortName: String?
 
+    /**
+     * A key used by the sorting system that takes into account the sort tags of this item,
+     * any (english) articles that prefix the names, and collation rules.
+     */
+    abstract val collationKey: CollationKey?
+
     /**
      * Resolve a name from it's raw form to a form suitable to be shown in a UI.
      * Null values will be resolved into their string form with this function.
      */
     abstract fun resolveName(context: Context): String
 
-    /**
+    // Equality is based on UIDs, as some items (Especially artists) can have identical
-     * A key used by the sorting system that takes into account the sort tags of this item,
+    // properties (Name) yet non-identical UIDs due to MusicBrainz tags
-     * any (english) articles that prefix the names, and collation rules.
-     */
-    val collationKey: CollationKey? by lazy {
-        // Ideally, we would generate this on creation, but this is an abstract class, which
-        // requires us to generate it lazily instead.
 
+    override fun hashCode() = uid.hashCode()
+
+    override fun equals(other: Any?) =
+        other is Music && javaClass == other.javaClass && uid == other.uid
+
+    /**
+     * Workaround to allow for easy collation key generation in the initializer without
+     * base-class initialization issues or slow lazy initialization.
+     */
+    protected fun makeCollationKeyImpl(): CollationKey? {
         val sortName = (rawSortName ?: rawName)?.run {
             when {
                 length > 5 && startsWith("the ", ignoreCase = true) -> substring(4)
@@ -78,16 +87,15 @@ sealed class Music : Item {
             }
         }
 
-        COLLATOR.getCollationKey(sortName)
+        return COLLATOR.getCollationKey(sortName)
     }
 
-    // Equality is based on UIDs, as some items (Especially artists) can have identical
+    /**
-    // properties (Name) yet non-identical UIDs due to MusicBrainz tags
+     * Called when the library has been linked and validation/construction steps dependent
-
+     * on linked items should run. It's also used to do last-step initialization of fields
-    override fun hashCode() = uid.hashCode()
+     * that require any parent values that would not be present during startup.
-
+     */
-    override fun equals(other: Any?) =
+    abstract fun _finalize()
-        other is Music && javaClass == other.javaClass && uid == other.uid
 
     /**
      * A unique identifier for a piece of music.
@@ -192,6 +200,10 @@ sealed class Music : Item {
 sealed class MusicParent : Music() {
     /** The songs that this parent owns. */
     abstract val songs: List<Song>
+
+    override fun _finalize() {
+        check(songs.isNotEmpty()) { "Invalid parent: No songs" }
+    }
 }
 
 /**
@@ -218,6 +230,8 @@ class Song constructor(raw: Raw, settings: Settings) : Music() {
 
     override val rawSortName = raw.sortName
 
+    override val collationKey = makeCollationKeyImpl()
+
     override fun resolveName(context: Context) = rawName
 
     /** The track number of this song in it's album.. */
@@ -279,15 +293,6 @@ class Song constructor(raw: Raw, settings: Settings) : Music() {
     private val albumArtistSortName = raw.albumArtistSortNames.parseMultiValue(settings)
         .joinToString().ifEmpty { null }
 
-    /**
-     * The raw artist name for this song in particular. First uses the artist tag, and then falls
-     * back to the album artist tag (i.e parent artist name). Null if name is unknown.
-     */
-    val individualArtistRawName: String?
-        // Note: This is a getter since it relies on a parent value that will not be initialized
-        // yet on creation.
-        get() = artistName ?: album.artist.rawName
-
     /**
      * Resolve the artist name for this song in particular. First uses the artist tag, and then
      * falls back to the album artist tag (i.e parent artist name)
@@ -295,6 +300,14 @@ class Song constructor(raw: Raw, settings: Settings) : Music() {
     fun resolveIndividualArtistName(context: Context) =
         artistName ?: album.artist.resolveName(context)
 
+    fun areArtistContentsTheSame(other: Song): Boolean {
+        if (other.artistName != null && artistName != null) {
+            return other.artistName == artistName
+        }
+
+        return album.artist.rawName == other.album.artist.rawName
+    }
+
     private val _genres: MutableList<Genre> = mutableListOf()
 
     /**
@@ -331,9 +344,9 @@ class Song constructor(raw: Raw, settings: Settings) : Music() {
         _genres.add(genre)
     }
 
-    fun _validate() {
+    override fun _finalize() {
-        (checkNotNull(_album) { "Malformed song: album is null" })._validate()
+        (checkNotNull(_album) { "Malformed song: Album is null" })
-        check(_genres.isNotEmpty()) { "Malformed song: genres are empty" }
+        check(_genres.isNotEmpty()) { "Malformed song: No genres" }
     }
 
     class Raw
@@ -381,6 +394,8 @@ class Album constructor(raw: Raw, override val songs: List<Song>) : MusicParent(
 
     override val rawSortName = raw.sortName
 
+    override val collationKey = makeCollationKeyImpl()
+
     override fun resolveName(context: Context) = rawName
 
     /** The earliest date this album was released. */
@@ -415,7 +430,8 @@ class Album constructor(raw: Raw, override val songs: List<Song>) : MusicParent(
         _artist = artist
     }
 
-    fun _validate() {
+    override fun _finalize() {
+        super._finalize()
         checkNotNull(_artist) { "Invalid album: Artist is null " }
     }
 
@@ -478,6 +494,8 @@ constructor(
 
     override val rawSortName = raw.sortName
 
+    override val collationKey = makeCollationKeyImpl()
+
     override fun resolveName(context: Context) = rawName ?: context.getString(R.string.def_artist)
 
     private val _songs = mutableListOf<Song>()
@@ -525,6 +543,8 @@ class Genre constructor(raw: Raw, override val songs: List<Song>) : MusicParent(
     // Sort tags don't make sense on genres
     override val rawSortName = rawName
 
+    override val collationKey = makeCollationKeyImpl()
+
     override fun resolveName(context: Context) = rawName ?: context.getString(R.string.def_genre)
 
     /** The total duration of the songs in this genre, in millis. */

app/src/main/java/org/oxycblt/auxio/music/MusicStore.kt

@@ -34,7 +34,7 @@ import org.oxycblt.auxio.util.contentResolverSafe
  *
  * The only other, memory-efficient option is to create our own hybrid database that leverages both
  * a typical DB and a mem-cache, like Vinyl. But why would we do that when I've encountered no real
- * issues with the current system.
+ * issues with the current system?
  *
  * [Library] may not be available at all times, so leveraging [Callback] is recommended. Consumers
  * should also be aware that [Library] may change while they are running, and design their work

app/src/main/java/org/oxycblt/auxio/music/extractor/MediaStoreExtractor.kt

@@ -100,7 +100,7 @@ import java.io.File
  * music loading process.
  * @author OxygenCobalt
  */
-abstract class MediaStoreLayer(private val context: Context, private val cacheLayer: CacheDatabase) {
+abstract class MediaStoreExtractor(private val context: Context, private val cacheDatabase: CacheDatabase) {
     private var cursor: Cursor? = null
 
     private var idIndex = -1
@@ -129,7 +129,7 @@ abstract class MediaStoreLayer(private val context: Context, private val cacheLa
         logD("Initializing")
         val start = System.currentTimeMillis()
 
-        cacheLayer.init()
+        cacheDatabase.init()
 
         val storageManager = context.getSystemServiceCompat(StorageManager::class)
         _volumes.addAll(storageManager.storageVolumesCompat)
@@ -238,7 +238,7 @@ abstract class MediaStoreLayer(private val context: Context, private val cacheLa
         cursor?.close()
         cursor = null
 
-        cacheLayer.finalize(rawSongs)
+        cacheDatabase.finalize(rawSongs)
     }
 
     /**
@@ -259,7 +259,7 @@ abstract class MediaStoreLayer(private val context: Context, private val cacheLa
         raw.dateAdded = cursor.getLong(dateAddedIndex)
         raw.dateModified = cursor.getLong(dateAddedIndex)
 
-        if (cacheLayer.maybePopulateCachedRaw(raw)) {
+        if (cacheDatabase.maybePopulateCachedRaw(raw)) {
             // We found a valid cache entry, no need to extract metadata.
             logD("Found cached raw: ${raw.name}")
             return true
@@ -368,12 +368,12 @@ abstract class MediaStoreLayer(private val context: Context, private val cacheLa
 // speed, we only want to add redundancy on known issues, not with possible issues.
 
 /**
- * A [MediaStoreLayer] that completes the music loading process in a way compatible from
+ * A [MediaStoreExtractor] that completes the music loading process in a way compatible from
  * API 21 onwards to API 29.
  * @author OxygenCobalt
  */
-class Api21MediaStoreLayer(context: Context, cacheLayer: CacheDatabase) :
+class Api21MediaStoreExtractor(context: Context, cacheDatabase: CacheDatabase) :
-    MediaStoreLayer(context, cacheLayer) {
+    MediaStoreExtractor(context, cacheDatabase) {
     private var trackIndex = -1
     private var dataIndex = -1
 
@@ -433,13 +433,13 @@ class Api21MediaStoreLayer(context: Context, cacheLayer: CacheDatabase) :
 }
 
 /**
- * A [MediaStoreLayer] that selects directories and builds paths using the modern volume fields
+ * A [MediaStoreExtractor] that selects directories and builds paths using the modern volume fields
  * available from API 29 onwards.
  * @author OxygenCobalt
  */
 @RequiresApi(Build.VERSION_CODES.Q)
-open class BaseApi29MediaStoreLayer(context: Context, cacheLayer: CacheDatabase) :
+open class BaseApi29MediaStoreExtractor(context: Context, cacheDatabase: CacheDatabase) :
-    MediaStoreLayer(context, cacheLayer) {
+    MediaStoreExtractor(context, cacheDatabase) {
     private var volumeIndex = -1
     private var relativePathIndex = -1
 
@@ -489,13 +489,13 @@ open class BaseApi29MediaStoreLayer(context: Context, cacheLayer: CacheDatabase)
 }
 
 /**
- * A [MediaStoreLayer] that completes the music loading process in a way compatible with at least
+ * A [MediaStoreExtractor] that completes the music loading process in a way compatible with at least
  * API 29.
  * @author OxygenCobalt
  */
 @RequiresApi(Build.VERSION_CODES.Q)
-open class Api29MediaStoreLayer(context: Context, cacheLayer: CacheDatabase) :
+open class Api29MediaStoreExtractor(context: Context, cacheDatabase: CacheDatabase) :
-    BaseApi29MediaStoreLayer(context, cacheLayer) {
+    BaseApi29MediaStoreExtractor(context, cacheDatabase) {
     private var trackIndex = -1
 
     override fun init(): Cursor {
@@ -521,13 +521,13 @@ open class Api29MediaStoreLayer(context: Context, cacheLayer: CacheDatabase) :
 }
 
 /**
- * A [MediaStoreLayer] that completes the music loading process in a way compatible with at least
+ * A [MediaStoreExtractor] that completes the music loading process in a way compatible with at least
  * API 30.
  * @author OxygenCobalt
  */
 @RequiresApi(Build.VERSION_CODES.R)
-class Api30MediaStoreLayer(context: Context, cacheLayer: CacheDatabase) :
+class Api30MediaStoreExtractor(context: Context, cacheDatabase: CacheDatabase) :
-    BaseApi29MediaStoreLayer(context, cacheLayer) {
+    BaseApi29MediaStoreExtractor(context, cacheDatabase) {
     private var trackIndex: Int = -1
     private var discIndex: Int = -1
 

app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataExtractor.kt

@@ -43,19 +43,19 @@ import org.oxycblt.auxio.util.logW
  *
  * @author OxygenCobalt
  */
-class MetadataLayer(private val context: Context, private val mediaStoreLayer: MediaStoreLayer) {
+class MetadataExtractor(private val context: Context, private val mediaStoreExtractor: MediaStoreExtractor) {
     private val taskPool: Array<Task?> = arrayOfNulls(TASK_CAPACITY)
 
     /** Initialize the sub-layers that this layer relies on. */
-    fun init() = mediaStoreLayer.init().count
+    fun init() = mediaStoreExtractor.init().count
 
     /** Finalize the sub-layers that this layer relies on. */
-    fun finalize(rawSongs: List<Song.Raw>) = mediaStoreLayer.finalize(rawSongs)
+    fun finalize(rawSongs: List<Song.Raw>) = mediaStoreExtractor.finalize(rawSongs)
 
     suspend fun parse(emit: suspend (Song.Raw) -> Unit) {
         while (true) {
             val raw = Song.Raw()
-            if (mediaStoreLayer.populateRawSong(raw) ?: break) {
+            if (mediaStoreExtractor.populateRawSong(raw) ?: break) {
                 // No need to extract metadata that was successfully restored from the cache
                 emit(raw)
                 continue

app/src/main/java/org/oxycblt/auxio/music/system/Indexer.kt

@@ -33,11 +33,11 @@ import org.oxycblt.auxio.music.Genre
 import org.oxycblt.auxio.music.MusicStore
 import org.oxycblt.auxio.music.Song
 import org.oxycblt.auxio.music.Sort
-import org.oxycblt.auxio.music.extractor.Api21MediaStoreLayer
+import org.oxycblt.auxio.music.extractor.Api21MediaStoreExtractor
-import org.oxycblt.auxio.music.extractor.Api29MediaStoreLayer
+import org.oxycblt.auxio.music.extractor.Api29MediaStoreExtractor
-import org.oxycblt.auxio.music.extractor.Api30MediaStoreLayer
+import org.oxycblt.auxio.music.extractor.Api30MediaStoreExtractor
 import org.oxycblt.auxio.music.extractor.CacheDatabase
-import org.oxycblt.auxio.music.extractor.MetadataLayer
+import org.oxycblt.auxio.music.extractor.MetadataExtractor
 import org.oxycblt.auxio.settings.Settings
 import org.oxycblt.auxio.util.logD
 import org.oxycblt.auxio.util.logE
@@ -197,25 +197,25 @@ class Indexer {
     private suspend fun indexImpl(context: Context): MusicStore.Library? {
         emitIndexing(Indexing.Indeterminate)
 
-        // Create the chain of layers. Each layer builds on the previous layer and
+        // Create the chain of extractors. Each extractor builds on the previous and
         // enables version-specific features in order to create the best possible music
         // experience. This is technically dependency injection. Except it doesn't increase
         // your compile times by 3x. Isn't that nice.
 
-        val cacheLayer = CacheDatabase()
+        val cacheDatabase = CacheDatabase()
 
-        val mediaStoreLayer =
+        val mediaStoreExtractor =
             when {
                 Build.VERSION.SDK_INT >= Build.VERSION_CODES.R ->
-                    Api30MediaStoreLayer(context, cacheLayer)
+                    Api30MediaStoreExtractor(context, cacheDatabase)
                 Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q ->
-                    Api29MediaStoreLayer(context, cacheLayer)
+                    Api29MediaStoreExtractor(context, cacheDatabase)
-                else -> Api21MediaStoreLayer(context, cacheLayer)
+                else -> Api21MediaStoreExtractor(context, cacheDatabase)
             }
 
-        val metadataLayer = MetadataLayer(context, mediaStoreLayer)
+        val metadataExtractor = MetadataExtractor(context, mediaStoreExtractor)
 
-        val songs = buildSongs(metadataLayer, Settings(context))
+        val songs = buildSongs(metadataExtractor, Settings(context))
         if (songs.isEmpty()) {
             return null
         }
@@ -226,9 +226,21 @@ class Indexer {
         val artists = buildArtists(albums)
         val genres = buildGenres(songs)
 
-        // Sanity check: Ensure that all songs are linked up to albums/artists/genres.
+        // Make sure we finalize all the items now that they are fully built.
         for (song in songs) {
-            song._validate()
+            song._finalize()
+        }
+
+        for (album in albums) {
+            album._finalize()
+        }
+
+        for (artist in artists) {
+            artist._finalize()
+        }
+
+        for (genre in genres) {
+            genre._finalize()
         }
 
         logD("Successfully built library in ${System.currentTimeMillis() - buildStart}ms")
@@ -237,19 +249,19 @@ class Indexer {
     }
 
     /**
-     * Does the initial query over the song database using [metadataLayer]. The songs returned by
+     * Does the initial query over the song database using [metadataExtractor]. The songs returned by
      * this function are **not** well-formed. The companion [buildAlbums], [buildArtists], and
      * [buildGenres] functions must be called with the returned list so that all songs are properly
      * linked up.
      */
-    private suspend fun buildSongs(metadataLayer: MetadataLayer, settings: Settings): List<Song> {
+    private suspend fun buildSongs(metadataExtractor: MetadataExtractor, settings: Settings): List<Song> {
         logD("Starting indexing process")
 
         val start = System.currentTimeMillis()
 
         // Initialize the extractor chain. This also nets us the projected total
         // that we can show when loading.
-        val total = metadataLayer.init()
+        val total = metadataExtractor.init()
         yield()
 
         // Note: We use a set here so we can eliminate effective duplicates of
@@ -257,7 +269,7 @@ class Indexer {
         val songs = mutableSetOf<Song>()
         val rawSongs = mutableListOf<Song.Raw>()
 
-        metadataLayer.parse { rawSong ->
+        metadataExtractor.parse { rawSong ->
             songs.add(Song(rawSong, settings))
             rawSongs.add(rawSong)
 
@@ -266,7 +278,7 @@ class Indexer {
             emitIndexing(Indexing.Songs(songs.size, total))
         }
 
-        metadataLayer.finalize(rawSongs)
+        metadataExtractor.finalize(rawSongs)
 
         val sorted = Sort(Sort.Mode.ByName, true).songs(songs)
 

app/src/main/java/org/oxycblt/auxio/search/SearchViewModel.kt

@@ -144,7 +144,6 @@ class SearchViewModel(application: Application) :
 
     private fun List<Song>.filterSongsBy(value: String) =
         baseFilterBy(value) {
-            logD(it.rawSortName)
             it.rawSortName?.contains(value, ignoreCase = true) == true ||
                 it.path.name.contains(value)
         }

app/src/main/java/org/oxycblt/auxio/ui/recycler/ViewHolders.kt

@@ -64,7 +64,7 @@ class SongViewHolder private constructor(private val binding: ItemSongBinding) :
             object : SimpleItemCallback<Song>() {
                 override fun areContentsTheSame(oldItem: Song, newItem: Song) =
                     oldItem.rawName == newItem.rawName &&
-                        oldItem.individualArtistRawName == oldItem.individualArtistRawName
+                        oldItem.areArtistContentsTheSame(newItem)
             }
     }
 }