From 6e00fd11290189d2c41b24ff4ea5fe8f11a8da3e Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Thu, 3 Feb 2022 20:05:20 -0700 Subject: [PATCH] music: merge duplicate albums [#66] Move all duplicate checking to the album creation stage, adding a new check for duplicate albums. Album names can be similarly duplicated as artist names, most often when one has an album consisting of multiple differing file formats. This commit fixes that by grouping albums up by their ID as usual, but then merging together albums that have the same (lowercase) album name and (lowercase) artist name. --- .../java/org/oxycblt/auxio/music/Models.kt | 7 +- .../org/oxycblt/auxio/music/MusicLoader.kt | 97 ++++++++++++------- 2 files changed, 66 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/org/oxycblt/auxio/music/Models.kt b/app/src/main/java/org/oxycblt/auxio/music/Models.kt index 1626ba7fe..e30d0addc 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/Models.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/Models.kt @@ -143,10 +143,8 @@ data class Album( } /** - * The data object for an *album* artist. Inherits [MusicParent]. This differs from the actual - * performers. - * @property albums The list of all [Album]s in this artist - * @property songs The list of all [Song]s in this artist + * The [MusicParent] for an *album* artist. This reflects a group of songs with the same(ish) + * album artist or artist field, not the individual performers of an artist. */ data class Artist( override val id: Long, @@ -166,7 +164,6 @@ data class Artist( /** * The data object for a genre. Inherits [MusicParent] - * @property songs The list of all [Song]s in this genre. */ data class Genre( override val id: Long, diff --git a/app/src/main/java/org/oxycblt/auxio/music/MusicLoader.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicLoader.kt index 6960f73c7..29f2d7902 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/MusicLoader.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicLoader.kt @@ -142,6 +142,8 @@ class MusicLoader { val album = cursor.getString(albumIndex) val albumId = cursor.getLong(albumIdIndex) + // If the artist field is , make it null. This makes handling the + // insanity of the artist field easier later on. val artist = cursor.getString(artistIndex).let { if (it != MediaStore.UNKNOWN_STRING) it else null } @@ -154,8 +156,16 @@ class MusicLoader { songs.add( Song( - id, title, fileName, album, albumId, artist, - albumArtist, year, track, duration + id, + title, + fileName, + album, + albumId, + artist, + albumArtist, + year, + track, + duration ) ) } @@ -170,26 +180,56 @@ class MusicLoader { private fun buildAlbums(songs: List): List { // Group up songs by their album ids and then link them with their albums - // TODO: Figure out how to group up songs by album in a way that does not accidentally - // split songs by album. val albums = mutableListOf() val songsByAlbum = songs.groupBy { it.albumId } - songsByAlbum.forEach { entry -> + for (entry in songsByAlbum) { + val albumId = entry.key + val albumSongs = entry.value + // Use the song with the latest year as our metadata song. // This allows us to replicate the LAST_YEAR field, which is useful as it means that // weird years like "0" wont show up if there are alternatives. - val song = requireNotNull(entry.value.maxByOrNull { it.year }) + val templateSong = requireNotNull(albumSongs.maxByOrNull { it.year }) + val albumName = templateSong.albumName - albums.add( - Album( - // When assigning an artist to an album, use the album artist first, then the - // normal artist, and then the internal representation of an unknown artist name. - entry.key, song.albumName, - song.albumArtistName ?: song.artistName ?: MediaStore.UNKNOWN_STRING, - song.year, entry.value + // When assigning an artist to an album, use the album artist first, then the + // normal artist, and then the internal representation of an unknown artist name. + val artistName = templateSong.albumArtistName + ?: templateSong.artistName ?: MediaStore.UNKNOWN_STRING + + val albumYear = templateSong.year + + // Search for duplicate albums first. This serves two purposes: + // 1. It collapses differently styled artists [ex. Rammstein vs. RAMMSTEIN] into + // a single grouped artist + // 2. It also unifies albums that exist across several file formats [excluding + // when the titles are mangled by MediaStore insanity] + val previousAlbumIndex = albums.indexOfFirst { album -> + album.name.lowercase() == albumName.lowercase() && + album.artistName.lowercase() == artistName.lowercase() + } + + if (previousAlbumIndex > -1) { + val previousAlbum = albums[previousAlbumIndex] + albums[previousAlbumIndex] = Album( + previousAlbum.id, + previousAlbum.name, + previousAlbum.artistName, + previousAlbum.year, + previousAlbum.songs + albumSongs ) - ) + } else { + albums.add( + Album( + albumId, + albumName, + artistName, + albumYear, + albumSongs + ) + ) + } } albums.removeAll { it.songs.isEmpty() } @@ -207,27 +247,18 @@ class MusicLoader { MediaStore.UNKNOWN_STRING -> context.getString(R.string.def_artist) else -> name } - val artistAlbums = entry.value.toMutableList() + val artistAlbums = entry.value - // Music files from the same artist may format the artist differently, such as being - // in uppercase/lowercase forms. If we have already built an artist that has a - // functionally identical name to this one, then simply merge the artists instead - // of removing them. - val previousArtistIndex = artists.indexOfFirst { artist -> - artist.name.lowercase() == name.lowercase() - } - - // In most cases, MediaStore artist IDs are unreliable or omitted for speed. - // Use the hashCode of the artist name as our ID and move on. - if (previousArtistIndex > -1) { - val previousArtist = artists[previousArtistIndex] - artists[previousArtistIndex] = Artist( - previousArtist.name.hashCode().toLong(), previousArtist.name, - previousArtist.resolvedName, previousArtist.albums + artistAlbums + // Due to the black magic we do to get a good artist field, the ID is unreliable. + // Take a hash of the artist name instead. + artists.add( + Artist( + name.hashCode().toLong(), + name, + resolvedName, + artistAlbums ) - } else { - artists.add(Artist(name.hashCode().toLong(), name, resolvedName, artistAlbums)) - } + ) } return artists