diff --git a/CHANGELOG.md b/CHANGELOG.md index ff563f7bb..49e79ef5a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - Added GitHub/email feedback forms to about page #### What's Improved +- Album grouping no longer done with artist in mind by default +- MusicBrainz IDs will no longer split albums/artists in less tagged libraries - M3U playlist file name is now proposed if one cannot be found within the file - Sorting songs by date now uses songs date first, before the earliest album date - Added working layouts for small split-screen form factors diff --git a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt index b8bc3b2a6..c10f4d03f 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceLibrary.kt @@ -21,12 +21,14 @@ package org.oxycblt.auxio.music.device import android.content.Context import android.net.Uri import android.provider.OpenableColumns +import java.util.UUID import javax.inject.Inject import kotlinx.coroutines.channels.Channel import org.oxycblt.auxio.music.Album import org.oxycblt.auxio.music.Artist import org.oxycblt.auxio.music.Genre import org.oxycblt.auxio.music.Music +import org.oxycblt.auxio.music.MusicParent import org.oxycblt.auxio.music.MusicRepository import org.oxycblt.auxio.music.Song import org.oxycblt.auxio.music.fs.Path @@ -51,10 +53,13 @@ import timber.log.Timber as L interface DeviceLibrary { /** All [Song]s in this [DeviceLibrary]. */ val songs: Collection + /** All [Album]s in this [DeviceLibrary]. */ val albums: Collection + /** All [Artist]s in this [DeviceLibrary]. */ val artists: Collection + /** All [Genre]s in this [DeviceLibrary]. */ val genres: Collection @@ -134,11 +139,9 @@ class DeviceLibraryFactoryImpl @Inject constructor() : DeviceLibrary.Factory { nameFactory: Name.Known.Factory ): DeviceLibraryImpl { val songGrouping = mutableMapOf() - val albumGrouping = mutableMapOf>() - val artistGrouping = mutableMapOf>() - val genreGrouping = mutableMapOf>() - - // TODO: Use comparators here + val albumGrouping = mutableMapOf>>() + val artistGrouping = mutableMapOf>>() + val genreGrouping = mutableMapOf>() // All music information is grouped as it is indexed by other components. rawSongs.forEachWithTimeout { rawSong -> @@ -159,62 +162,22 @@ class DeviceLibraryFactoryImpl @Inject constructor() : DeviceLibrary.Factory { songGrouping[song.uid] = song // Group the new song into an album. - val albumKey = song.rawAlbum.key - val albumBody = albumGrouping[albumKey] - if (albumBody != null) { - albumBody.music.add(song) - val prioritized = albumBody.raw.src - // Since albums are grouped fuzzily, we pick the song with the earliest track to - // use for album information to ensure consistent metadata and UIDs. Fall back to - // the name otherwise. - val higherPriority = - song.track != null && - (prioritized.track == null || - song.track < prioritized.track || - (song.track == prioritized.track && song.name < prioritized.name)) - - if (higherPriority) { - albumBody.raw = PrioritizedRaw(song.rawAlbum, song) - } - } else { - // Need to initialize this grouping. - albumGrouping[albumKey] = - Grouping(PrioritizedRaw(song.rawAlbum, song), mutableSetOf(song)) + appendToMusicBrainzIdTree(song, song.rawAlbum, albumGrouping) { old, new -> + compareSongTracks(old, new) } - // Group the song into each of it's artists. for (rawArtist in song.rawArtists) { - val artistKey = rawArtist.key - val artistBody = artistGrouping[artistKey] - if (artistBody != null) { - // Since artists are not guaranteed to have songs, song artist information is - // de-prioritized compared to album artist information. - artistBody.music.add(song) - } else { - // Need to initialize this grouping. - artistGrouping[artistKey] = - Grouping(PrioritizedRaw(rawArtist, song), mutableSetOf(song)) + appendToMusicBrainzIdTree(song, rawArtist, artistGrouping) { old, new -> + // Artist information from earlier dates is prioritized, as it is less likely to + // change with the addition of new tracks. Fall back to the name otherwise. + check(old is SongImpl) // This should always be the case. + compareSongDates(old, new) } } // Group the song into each of it's genres. for (rawGenre in song.rawGenres) { - val genreKey = rawGenre.key - val genreBody = genreGrouping[genreKey] - if (genreBody != null) { - genreBody.music.add(song) - // Genre information from higher songs in ascending alphabetical order are - // prioritized. - val prioritized = genreBody.raw.src - val higherPriority = song.name < prioritized.name - if (higherPriority) { - genreBody.raw = PrioritizedRaw(rawGenre, song) - } - } else { - // Need to initialize this grouping. - genreGrouping[genreKey] = - Grouping(PrioritizedRaw(rawGenre, song), mutableSetOf(song)) - } + appendToNameTree(song, rawGenre, genreGrouping) { old, new -> new.name < old.name } } processedSongs.sendWithTimeout(rawSong) @@ -222,47 +185,154 @@ class DeviceLibraryFactoryImpl @Inject constructor() : DeviceLibrary.Factory { // Now that all songs are processed, also process albums and group them into their // respective artists. - val albums = albumGrouping.values.mapTo(mutableSetOf()) { AlbumImpl(it, nameFactory) } + pruneMusicBrainzIdTree(albumGrouping) { old, new -> + compareSongTracks(old, new) + } + val albums = flattenMusicBrainzIdTree(albumGrouping) { AlbumImpl(it, nameFactory) } for (album in albums) { for (rawArtist in album.rawArtists) { - val key = RawArtist.Key(rawArtist) - val body = artistGrouping[key] - if (body != null) { - body.music.add(album) - when (val prioritized = body.raw.src) { + appendToMusicBrainzIdTree(album, rawArtist, artistGrouping) { old, new -> + when (old) { // Immediately replace any songs that initially held the priority position. - is SongImpl -> body.raw = PrioritizedRaw(rawArtist, album) + is SongImpl -> true is AlbumImpl -> { - // Album artist information from earlier dates is prioritized, as it is - // less likely to change with the addition of new tracks. Fall back to - // the name otherwise. - val prioritize = - album.dates != null && - (prioritized.dates == null || - album.dates < prioritized.dates || - (album.dates == prioritized.dates && - album.name < prioritized.name)) - - if (prioritize) { - body.raw = PrioritizedRaw(rawArtist, album) - } + compareAlbumDates(old, new) } else -> throw IllegalStateException() } - } else { - // Need to initialize this grouping. - artistGrouping[key] = - Grouping(PrioritizedRaw(rawArtist, album), mutableSetOf(album)) } } } // Artists and genres do not need to be grouped and can be processed immediately. - val artists = artistGrouping.values.mapTo(mutableSetOf()) { ArtistImpl(it, nameFactory) } - val genres = genreGrouping.values.mapTo(mutableSetOf()) { GenreImpl(it, nameFactory) } + pruneMusicBrainzIdTree(artistGrouping) { old, new -> + when { + // Immediately replace any songs that initially held the priority position. + old is SongImpl && new is AlbumImpl -> true + old is AlbumImpl && new is SongImpl -> false + old is SongImpl && new is SongImpl -> { + compareSongDates(old, new) + } + old is AlbumImpl && new is AlbumImpl -> { + compareAlbumDates(old, new) + } + else -> throw IllegalStateException() + } + } + val artists = flattenMusicBrainzIdTree(artistGrouping) { ArtistImpl(it, nameFactory) } + val genres = flattenNameTree(genreGrouping) { GenreImpl(it, nameFactory) } return DeviceLibraryImpl(songGrouping.values.toSet(), albums, artists, genres) } + + private inline fun appendToNameTree( + music: N, + raw: R, + tree: MutableMap>, + prioritize: (old: O, new: N) -> Boolean, + ) { + val nameKey = raw.name?.lowercase() + val body = tree[nameKey] + if (body != null) { + body.music.add(music) + if (prioritize(body.raw.src, music)) { + body.raw = PrioritizedRaw(raw, music) + } + } else { + // Need to initialize this grouping. + tree[nameKey] = Grouping(PrioritizedRaw(raw, music), mutableSetOf(music)) + } + } + + private inline fun flattenNameTree( + tree: MutableMap>, + map: (Grouping) -> P + ): Set

= tree.values.mapTo(mutableSetOf()) { map(it) } + + private inline fun appendToMusicBrainzIdTree( + music: N, + raw: R, + tree: MutableMap>>, + prioritize: (old: O, new: N) -> Boolean, + ) { + val nameKey = raw.name?.lowercase() + val musicBrainzIdGroups = tree[nameKey] + if (musicBrainzIdGroups != null) { + val body = musicBrainzIdGroups[raw.musicBrainzId] + if (body != null) { + body.music.add(music) + if (prioritize(body.raw.src, music)) { + body.raw = PrioritizedRaw(raw, music) + } + } else { + // Need to initialize this grouping. + musicBrainzIdGroups[raw.musicBrainzId] = + Grouping(PrioritizedRaw(raw, music), mutableSetOf(music)) + } + } else { + // Need to initialize this grouping. + tree[nameKey] = + mutableMapOf( + raw.musicBrainzId to Grouping(PrioritizedRaw(raw, music), mutableSetOf(music))) + } + } + + private inline fun pruneMusicBrainzIdTree( + tree: MutableMap>>, + prioritize: (old: M, new: M) -> Boolean + ) { + for ((_, musicBrainzIdGroups) in tree) { + var nullGroup = musicBrainzIdGroups[null] + if (nullGroup == null) { + // Full MusicBrainz ID tagging. Nothing to do. + continue + } + // Only partial MusicBrainz ID tagging. For the sake of basic sanity, just + // collapse all of them into the null group. + // TODO: More advanced heuristics eventually (tm) + musicBrainzIdGroups + .filter { it.key != null } + .forEach { + val (_, group) = it + nullGroup.music.addAll(group.music) + if (prioritize(group.raw.src, nullGroup.raw.src)) { + nullGroup.raw = group.raw + } + musicBrainzIdGroups.remove(it.key) + } + } + } + + private inline fun flattenMusicBrainzIdTree( + tree: MutableMap>>, + map: (Grouping) -> T + ): Set { + val result = mutableSetOf() + for ((_, musicBrainzIdGroups) in tree) { + for (group in musicBrainzIdGroups.values) { + result += map(group) + } + } + return result + } + + private fun compareSongTracks(old: SongImpl, new: SongImpl) = + new.track != null && + (old.track == null || + new.track < old.track || + (new.track == old.track && new.name < old.name)) + + private fun compareAlbumDates(old: AlbumImpl, new: AlbumImpl) = + new.dates != null && + (old.dates == null || + new.dates < old.dates || + (new.dates == old.dates && new.name < old.name)) + + private fun compareSongDates(old: SongImpl, new: SongImpl) = + new.date != null && + (old.date == null || + new.date < old.date || + (new.date == old.date && new.name < old.name)) } // TODO: Avoid redundant data creation diff --git a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt index eded8ec50..1a90b4cd7 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/device/DeviceMusicImpl.kt @@ -160,7 +160,14 @@ class SongImpl( name, artistSortNames.getOrNull(i)) } - .distinctBy { it.key } + // Some songs have the same artist listed multiple times (sometimes with different + // casing!), + // so we need to deduplicate lest finalization reordering fails. + // Since MBID data can wind up clobbered later in the grouper, we can't really + // use it to deduplicate. That means that a hypothetical track with two artists + // of the same name but different MBIDs will be grouped wrong. That is a bridge + // I will cross when I get to it. + .distinctBy { it.name?.lowercase() } val albumArtistMusicBrainzIds = separators.split(rawSong.albumArtistMusicBrainzIds) val albumArtistNames = separators.split(rawSong.albumArtistNames) @@ -173,7 +180,7 @@ class SongImpl( name, albumArtistSortNames.getOrNull(i)) } - .distinctBy { it.key } + .distinctBy { it.name?.lowercase() } rawAlbum = RawAlbum( @@ -199,7 +206,10 @@ class SongImpl( val genreNames = (rawSong.genreNames.parseId3GenreNames() ?: separators.split(rawSong.genreNames)) rawGenres = - genreNames.map { RawGenre(it) }.distinctBy { it.key }.ifEmpty { listOf(RawGenre()) } + genreNames + .map { RawGenre(it) } + .distinctBy { it.name?.lowercase() } + .ifEmpty { listOf(RawGenre()) } hashCode = 31 * hashCode + rawSong.hashCode() hashCode = 31 * hashCode + nameFactory.hashCode() @@ -507,7 +517,7 @@ class ArtistImpl( * @return The index of the [Artist]'s [RawArtist] within the list. */ fun getOriginalPositionIn(rawArtists: List) = - rawArtists.indexOfFirst { it.key == rawArtist.key } + rawArtists.indexOfFirst { it.name?.lowercase() == rawArtist.name?.lowercase() } /** * Perform final validation and organization on this instance. @@ -600,7 +610,7 @@ class GenreImpl( * @return The index of the [Genre]'s [RawGenre] within the list. */ fun getOriginalPositionIn(rawGenres: List) = - rawGenres.indexOfFirst { it.key == rawGenre.key } + rawGenres.indexOfFirst { it.name?.lowercase() == rawGenre.name?.lowercase() } /** * Perform final validation and organization on this instance. diff --git a/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt b/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt index 5b1b6df03..25a4a398e 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/device/RawMusic.kt @@ -107,49 +107,16 @@ data class RawAlbum( */ val mediaStoreId: Long, /** @see Music.uid */ - val musicBrainzId: UUID?, + override val musicBrainzId: UUID?, /** @see Music.name */ - val name: String, + override val name: String, /** @see Music.name */ val sortName: String?, /** @see Album.releaseType */ val releaseType: ReleaseType?, /** @see RawArtist.name */ val rawArtists: List -) { - val key = Key(this) - - /** - * Allows [RawAlbum]s to be compared by "fundamental" information that is unlikely to change on - * an item-by-item - */ - data class Key(private val inner: 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. - // - If we do not have a MusicBrainz ID, compare by the lowercase album name and lowercase - // artist name. This allows for case-insensitive artist/album grouping, which can be common - // for albums/artists that have different naming (ex. "RAMMSTEIN" vs. "Rammstein"). - private val artistKeys = inner.rawArtists.map { it.key } - - // Cache the hash-code for HashMap efficiency. - private val hashCode = - inner.musicBrainzId?.hashCode() - ?: (31 * inner.name.lowercase().hashCode() + artistKeys.hashCode()) - - override fun hashCode() = hashCode - - override fun equals(other: Any?) = - other is Key && - when { - inner.musicBrainzId != null && other.inner.musicBrainzId != null -> - inner.musicBrainzId == other.inner.musicBrainzId - inner.musicBrainzId == null && other.inner.musicBrainzId == null -> - inner.name.equals(other.inner.name, true) && artistKeys == other.artistKeys - else -> false - } - } -} +) : MusicBrainzGroupable /** * Raw information about an [ArtistImpl] obtained from the component [SongImpl] and [AlbumImpl] @@ -159,49 +126,12 @@ data class RawAlbum( */ data class RawArtist( /** @see Music.UID */ - val musicBrainzId: UUID? = null, + override val musicBrainzId: UUID? = null, /** @see Music.name */ - val name: String? = null, + override val name: String? = null, /** @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(private val inner: 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. - // - If we do not have a MusicBrainz ID, compare by the lowercase name. This allows artist - // grouping to be case-insensitive. - - // Cache the hashCode for HashMap efficiency. - val hashCode = inner.musicBrainzId?.hashCode() ?: inner.name?.lowercase().hashCode() - - // Compare names and MusicBrainz IDs in order to differentiate artists with the - // same name in large libraries. - - override fun hashCode() = hashCode - - override fun equals(other: Any?) = - other is Key && - when { - inner.musicBrainzId != null && other.inner.musicBrainzId != null -> - inner.musicBrainzId == other.inner.musicBrainzId - inner.musicBrainzId == null && other.inner.musicBrainzId == null -> - when { - inner.name != null && other.inner.name != null -> - inner.name.equals(other.inner.name, true) - inner.name == null && other.inner.name == null -> true - else -> false - } - else -> false - } - } -} +) : MusicBrainzGroupable /** * Raw information about a [GenreImpl] obtained from the component [SongImpl] instances. @@ -210,32 +140,15 @@ data class RawArtist( */ data class RawGenre( /** @see Music.name */ - val name: String? = null -) { - val key = Key(this) + override val name: String? = null +) : NameGroupable - /** - * Allows [RawGenre]s to be compared by "fundamental" information that is unlikely to change on - * an item-by-item - */ - data class Key(private val inner: RawGenre) { - // Cache the hashCode for HashMap efficiency. - private val hashCode = inner.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 - // formatting genres. - override fun hashCode() = hashCode +interface NameGroupable { + val name: String? +} - override fun equals(other: Any?) = - other is Key && - when { - inner.name != null && other.inner.name != null -> - inner.name.equals(other.inner.name, true) - inner.name == null && other.inner.name == null -> true - else -> false - } - } +interface MusicBrainzGroupable : NameGroupable { + val musicBrainzId: UUID? } /**