music: fix comparison issues

Fix a few problems with the current comparison algorithm:
1. It wasn't actually comparing the raw music information, only the
UIDs, which was redundant.
2. The comparison in the main music loading process occurred on the
main thread, which causes massive freeze-up issues.

Resolves #457.

CHANGELOG.md

@@ -18,6 +18,8 @@
 within it
 - Fixed blurry playing indicator in album/artist/genre/playlist items
 - Fixed incorrect songs being displayed when adding albums to the end of the queue
+- Fixed freezing occuring when scrolling through large music libraries
+- Fixed app not responding once music loading completes for large libraries
 
 #### What's Changed
 - Android Lollipop and Marshmallow support have been dropped

app/src/main/java/org/oxycblt/auxio/image/extractor/CoverExtractor.kt

@@ -118,6 +118,7 @@ constructor(
      *   the given [Album] in the given [Song] list.
      */
     fun computeCoverOrdering(songs: List<Song>): List<Album> {
+        // TODO: Start short-circuiting in more places
         if (songs.isEmpty()) return listOf()
         if (songs.size == 1) return listOf(songs.first().album)
 

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

@@ -32,7 +32,6 @@ import kotlinx.coroutines.Job
 import kotlinx.coroutines.async
 import kotlinx.coroutines.channels.Channel
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.withContext
 import kotlinx.coroutines.yield
 import org.oxycblt.auxio.music.cache.CacheRepository
 import org.oxycblt.auxio.music.device.DeviceLibrary
@@ -301,44 +300,35 @@ constructor(
         val userLibrary = synchronized(this) { userLibrary ?: return }
         logD("Creating playlist $name with ${songs.size} songs")
         userLibrary.createPlaylist(name, songs)
-        notifyUserLibraryChange()
+        emitLibraryChange(device = false, user = true)
     }
 
     override suspend fun renamePlaylist(playlist: Playlist, name: String) {
         val userLibrary = synchronized(this) { userLibrary ?: return }
         logD("Renaming $playlist to $name")
         userLibrary.renamePlaylist(playlist, name)
-        notifyUserLibraryChange()
+        emitLibraryChange(device = false, user = true)
     }
 
     override suspend fun deletePlaylist(playlist: Playlist) {
         val userLibrary = synchronized(this) { userLibrary ?: return }
         logD("Deleting $playlist")
         userLibrary.deletePlaylist(playlist)
-        notifyUserLibraryChange()
+        emitLibraryChange(device = false, user = true)
     }
 
     override suspend fun addToPlaylist(songs: List<Song>, playlist: Playlist) {
         val userLibrary = synchronized(this) { userLibrary ?: return }
         logD("Adding ${songs.size} songs to $playlist")
         userLibrary.addToPlaylist(playlist, songs)
-        notifyUserLibraryChange()
+        emitLibraryChange(device = false, user = true)
     }
 
     override suspend fun rewritePlaylist(playlist: Playlist, songs: List<Song>) {
         val userLibrary = synchronized(this) { userLibrary ?: return }
         logD("Rewriting $playlist with ${songs.size} songs")
         userLibrary.rewritePlaylist(playlist, songs)
-        notifyUserLibraryChange()
+        emitLibraryChange(device = false, user = true)
-    }
-
-    @Synchronized
-    private fun notifyUserLibraryChange() {
-        logD("Dispatching user library change")
-        for (listener in updateListeners) {
-            listener.onMusicChanges(
-                MusicRepository.Changes(deviceLibrary = false, userLibrary = true))
-        }
     }
 
     @Synchronized
@@ -435,7 +425,7 @@ constructor(
         val deviceLibraryChannel = Channel<DeviceLibrary>()
         logD("Starting DeviceLibrary creation")
         val deviceLibraryJob =
-            worker.scope.tryAsync(Dispatchers.Main) {
+            worker.scope.tryAsync(Dispatchers.Default) {
                 deviceLibraryFactory.create(rawSongs).also { deviceLibraryChannel.send(it) }
             }
         logD("Starting UserLibrary creation")
@@ -452,10 +442,22 @@ constructor(
         val userLibrary = userLibraryJob.await().getOrThrow()
 
         logD("Successfully indexed music library [device=$deviceLibrary user=$userLibrary]")
-        withContext(Dispatchers.Main) {
         emitComplete(null)
-            emitData(deviceLibrary, userLibrary)
+
+        // Comparing the library instances is obscenely expensive, do it within the library
+        val deviceLibraryChanged = this.deviceLibrary != deviceLibrary
+        val userLibraryChanged = this.userLibrary != userLibrary
+        if (!deviceLibraryChanged && !userLibraryChanged) {
+            logD("Library has not changed, skipping update")
+            return
         }
+
+        synchronized(this) {
+            this.deviceLibrary = deviceLibrary
+            this.userLibrary = userLibrary
+        }
+
+        emitLibraryChange(deviceLibraryChanged, userLibraryChanged)
     }
 
     /**
@@ -497,14 +499,8 @@ constructor(
     }
 
     @Synchronized
-    private fun emitData(deviceLibrary: DeviceLibrary, userLibrary: MutableUserLibrary) {
+    private fun emitLibraryChange(device: Boolean, user: Boolean) {
-        val deviceLibraryChanged = this.deviceLibrary != deviceLibrary
+        val changes = MusicRepository.Changes(device, user)
-        val userLibraryChanged = this.userLibrary != userLibrary
-        if (!deviceLibraryChanged && !userLibraryChanged) return
-
-        this.deviceLibrary = deviceLibrary
-        this.userLibrary = userLibrary
-        val changes = MusicRepository.Changes(deviceLibraryChanged, userLibraryChanged)
         logD("Dispatching library change [changes=$changes]")
         for (listener in updateListeners) {
             listener.onMusicChanges(changes)

app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt

@@ -193,8 +193,8 @@ private class DeviceLibraryImpl(rawSongs: List<RawSong>, settings: MusicSettings
     private fun buildAlbums(songs: List<SongImpl>, settings: MusicSettings): List<AlbumImpl> {
         // Group songs by their singular raw album, then map the raw instances and their
         // grouped songs to Album values. Album.Raw will handle the actual grouping rules.
-        val songsByAlbum = songs.groupBy { it.rawAlbum }
+        val songsByAlbum = songs.groupBy { it.rawAlbum.key }
-        val albums = songsByAlbum.map { AlbumImpl(it.key, settings, it.value) }
+        val albums = songsByAlbum.map { AlbumImpl(it.key.value, settings, it.value) }
         logD("Successfully built ${albums.size} albums")
         return albums
     }
@@ -221,22 +221,22 @@ private class DeviceLibraryImpl(rawSongs: List<RawSong>, settings: MusicSettings
     ): List<ArtistImpl> {
         // Add every raw artist credited to each Song/Album to the grouping. This way,
         // different multi-artist combinations are not treated as different artists.
-        val musicByArtist = mutableMapOf<RawArtist, MutableList<Music>>()
+        val musicByArtist = mutableMapOf<RawArtist.Key, MutableList<Music>>()
 
         for (song in songs) {
             for (rawArtist in song.rawArtists) {
-                musicByArtist.getOrPut(rawArtist) { mutableListOf() }.add(song)
+                musicByArtist.getOrPut(rawArtist.key) { mutableListOf() }.add(song)
             }
         }
 
         for (album in albums) {
             for (rawArtist in album.rawArtists) {
-                musicByArtist.getOrPut(rawArtist) { mutableListOf() }.add(album)
+                musicByArtist.getOrPut(rawArtist.key) { mutableListOf() }.add(album)
             }
         }
 
         // Convert the combined mapping into artist instances.
-        val artists = musicByArtist.map { ArtistImpl(it.key, settings, it.value) }
+        val artists = musicByArtist.map { ArtistImpl(it.key.value, settings, it.value) }
         logD("Successfully built ${artists.size} artists")
         return artists
     }
@@ -253,15 +253,15 @@ private class DeviceLibraryImpl(rawSongs: List<RawSong>, settings: MusicSettings
     private fun buildGenres(songs: List<SongImpl>, settings: MusicSettings): List<GenreImpl> {
         // Add every raw genre credited to each Song to the grouping. This way,
         // different multi-genre combinations are not treated as different genres.
-        val songsByGenre = mutableMapOf<RawGenre, MutableList<SongImpl>>()
+        val songsByGenre = mutableMapOf<RawGenre.Key, MutableList<SongImpl>>()
         for (song in songs) {
             for (rawGenre in song.rawGenres) {
-                songsByGenre.getOrPut(rawGenre) { mutableListOf() }.add(song)
+                songsByGenre.getOrPut(rawGenre.key) { mutableListOf() }.add(song)
             }
         }
 
         // Convert the mapping into genre instances.
-        val genres = songsByGenre.map { GenreImpl(it.key, settings, it.value) }
+        val genres = songsByGenre.map { GenreImpl(it.key.value, settings, it.value) }
         logD("Successfully built ${genres.size} genres")
         return genres
     }

app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt

@@ -407,7 +407,8 @@ class ArtistImpl(
      *   [RawArtist] will be within the list.
      * @return The index of the [Artist]'s [RawArtist] within the list.
      */
-    fun getOriginalPositionIn(rawArtists: List<RawArtist>) = rawArtists.indexOf(rawArtist)
+    fun getOriginalPositionIn(rawArtists: List<RawArtist>) =
+        rawArtists.indexOfFirst { it.key == rawArtist.key }
 
     /**
      * Perform final validation and organization on this instance.
@@ -481,7 +482,8 @@ class GenreImpl(
      *   [RawGenre] will be within the list.
      * @return The index of the [Genre]'s [RawGenre] within the list.
      */
-    fun getOriginalPositionIn(rawGenres: List<RawGenre>) = rawGenres.indexOf(rawGenre)
+    fun getOriginalPositionIn(rawGenres: List<RawGenre>) =
+        rawGenres.indexOfFirst { it.key == rawGenre.key }
 
     /**
      * Perform final validation and organization on this instance.

app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt

@@ -113,6 +113,10 @@ data class RawAlbum(
     /** @see RawArtist.name */
     val rawArtists: List<RawArtist>
 ) {
+    val key = Key(this)
+
+    /** Exposed information that denotes [RawAlbum] uniqueness. */
+    data class Key(val value: RawAlbum) {
         // Albums are grouped as follows:
         // - If we have a MusicBrainz ID, only group by it. This allows different Albums with the
         // same name to be differentiated, which is common in large libraries.
@@ -122,19 +126,22 @@ data class RawAlbum(
 
         // Cache the hash-code for HashMap efficiency.
         private val hashCode =
-        musicBrainzId?.hashCode() ?: (31 * name.lowercase().hashCode() + rawArtists.hashCode())
+            value.musicBrainzId?.hashCode()
+                ?: (31 * value.name.lowercase().hashCode() + value.rawArtists.hashCode())
 
         override fun hashCode() = hashCode
 
         override fun equals(other: Any?) =
-        other is RawAlbum &&
+            other is Key &&
                 when {
-                musicBrainzId != null && other.musicBrainzId != null ->
+                    value.musicBrainzId != null && other.value.musicBrainzId != null ->
-                    musicBrainzId == other.musicBrainzId
+                        value.musicBrainzId == other.value.musicBrainzId
-                musicBrainzId == null && other.musicBrainzId == null ->
+                    value.musicBrainzId == null && other.value.musicBrainzId == null ->
-                    name.equals(other.name, true) && rawArtists == other.rawArtists
+                        other.value.name.equals(other.value.name, true) &&
+                            other.value.rawArtists == other.value.rawArtists
                     else -> false
                 }
+    }
 }
 
 /**
@@ -151,6 +158,13 @@ data class RawArtist(
     /** @see Music.name */
     val sortName: String? = null
 ) {
+    val key = Key(this)
+
+    /**
+     * Allows [RawArtist]s to be compared by "fundamental" information that is unlikely to change on
+     * an item-by-item
+     */
+    data class Key(val value: RawArtist) {
         // Artists are grouped as follows:
         // - If we have a MusicBrainz ID, only group by it. This allows different Artists with the
         // same name to be differentiated, which is common in large libraries.
@@ -158,7 +172,7 @@ data class RawArtist(
         // grouping to be case-insensitive.
 
         // Cache the hashCode for HashMap efficiency.
-    private val hashCode = musicBrainzId?.hashCode() ?: name?.lowercase().hashCode()
+        private val hashCode = value.musicBrainzId?.hashCode() ?: value.name?.lowercase().hashCode()
 
         // Compare names and MusicBrainz IDs in order to differentiate artists with the
         // same name in large libraries.
@@ -166,18 +180,20 @@ data class RawArtist(
         override fun hashCode() = hashCode
 
         override fun equals(other: Any?) =
-        other is RawArtist &&
+            other is Key &&
                 when {
-                musicBrainzId != null && other.musicBrainzId != null ->
+                    value.musicBrainzId != null && other.value.musicBrainzId != null ->
-                    musicBrainzId == other.musicBrainzId
+                        value.musicBrainzId == other.value.musicBrainzId
-                musicBrainzId == null && other.musicBrainzId == null ->
+                    value.musicBrainzId == null && other.value.musicBrainzId == null ->
                         when {
-                        name != null && other.name != null -> name.equals(other.name, true)
+                            value.name != null && other.value.name != null ->
-                        name == null && other.name == null -> true
+                                value.name.equals(other.value.name, true)
+                            value.name == null && other.value.name == null -> true
                             else -> false
                         }
                     else -> false
                 }
+    }
 }
 
 /**
@@ -189,9 +205,11 @@ data class RawGenre(
     /** @see Music.name */
     val name: String? = null
 ) {
+    val key = Key(this)
 
+    data class Key(val value: RawGenre) {
         // Cache the hashCode for HashMap efficiency.
-    private val hashCode = name?.lowercase().hashCode()
+        private val hashCode = value.name?.lowercase().hashCode()
 
         // Only group by the lowercase genre name. This allows Genre grouping to be
         // case-insensitive, which may be helpful in some libraries with different ways of
@@ -199,10 +217,12 @@ data class RawGenre(
         override fun hashCode() = hashCode
 
         override fun equals(other: Any?) =
-        other is RawGenre &&
+            other is Key &&
                 when {
-                name != null && other.name != null -> name.equals(other.name, true)
+                    value.name != null && other.value.name != null ->
-                name == null && other.name == null -> true
+                        value.name.equals(other.value.name, true)
+                    value.name == null && other.value.name == null -> true
                     else -> false
                 }
+    }
 }

app/src/main/java/org/oxycblt/auxio/music/user/UserLibrary.kt

@@ -149,6 +149,8 @@ private class UserLibraryImpl(
     private val playlistMap: MutableMap<Music.UID, PlaylistImpl>,
     private val musicSettings: MusicSettings
 ) : MutableUserLibrary {
+    override fun hashCode() = playlistMap.hashCode()
+    override fun equals(other: Any?) = other is UserLibraryImpl && other.playlistMap == playlistMap
     override fun toString() = "UserLibrary(playlists=${playlists.size})"
 
     override val playlists: List<Playlist>

app/src/main/java/org/oxycblt/auxio/playback/system/PlaybackService.kt

@@ -107,8 +107,8 @@ class PlaybackService :
 
     // Coroutines
     private val serviceJob = Job()
-    private val restoreScope = CoroutineScope(serviceJob + Dispatchers.Main)
+    private val restoreScope = CoroutineScope(serviceJob + Dispatchers.IO)
-    private val saveScope = CoroutineScope(serviceJob + Dispatchers.Main)
+    private val saveScope = CoroutineScope(serviceJob + Dispatchers.IO)
 
     // --- SERVICE OVERRIDES ---