music: move multi-value util to separators

Move all multi-value utilities to a new Separators interface.

This should allow separator config to be dynamically compared across
song instances, and generally make songs easier to test.

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

@@ -43,7 +43,7 @@ interface MusicSettings : Settings<MusicSettings.Listener> {
     /** Whether to be actively watching for changes in the music library. */
     val shouldBeObserving: Boolean
     /** A [String] of characters representing the desired characters to denote multi-value tags. */
-    var multiValueSeparators: String
+    var separators: String
     /** Whether to enable more advanced sorting by articles and numbers. */
     val intelligentSorting: Boolean
 
@@ -85,7 +85,7 @@ class MusicSettingsImpl @Inject constructor(@ApplicationContext context: Context
     override val shouldBeObserving: Boolean
         get() = sharedPreferences.getBoolean(getString(R.string.set_key_observing), false)
 
-    override var multiValueSeparators: String
+    override var separators: String
         // Differ from convention and store a string of separator characters instead of an int
         // code. This makes it easier to use and more extendable.
         get() = sharedPreferences.getString(getString(R.string.set_key_separators), "") ?: ""

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

@@ -32,6 +32,7 @@ import org.oxycblt.auxio.music.MusicSettings
 import org.oxycblt.auxio.music.Song
 import org.oxycblt.auxio.music.fs.contentResolverSafe
 import org.oxycblt.auxio.music.fs.useQuery
+import org.oxycblt.auxio.music.metadata.Separators
 import org.oxycblt.auxio.util.logW
 import org.oxycblt.auxio.util.unlikelyToBeNull
 
@@ -118,6 +119,8 @@ class DeviceLibraryFactoryImpl @Inject constructor(private val musicSettings: Mu
         rawSongs: Channel<RawSong>,
         processedSongs: Channel<RawSong>
     ): DeviceLibraryImpl {
+        val separators = Separators.from(musicSettings.separators)
+
         val songGrouping = mutableMapOf<Music.UID, SongImpl>()
         val albumGrouping = mutableMapOf<RawAlbum.Key, Grouping<RawAlbum, SongImpl>>()
         val artistGrouping = mutableMapOf<RawArtist.Key, Grouping<RawArtist, Music>>()
@@ -127,7 +130,7 @@ class DeviceLibraryFactoryImpl @Inject constructor(private val musicSettings: Mu
 
         // All music information is grouped as it is indexed by other components.
         for (rawSong in rawSongs) {
-            val song = SongImpl(rawSong, musicSettings)
+            val song = SongImpl(rawSong, musicSettings, separators)
             // At times the indexer produces duplicate songs, try to filter these. Comparing by
             // UID is sufficient for something like this, and also prevents collisions from
             // causing severe issues elsewhere.

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

@@ -36,8 +36,8 @@ import org.oxycblt.auxio.music.info.Date
 import org.oxycblt.auxio.music.info.Disc
 import org.oxycblt.auxio.music.info.Name
 import org.oxycblt.auxio.music.info.ReleaseType
+import org.oxycblt.auxio.music.metadata.Separators
 import org.oxycblt.auxio.music.metadata.parseId3GenreNames
-import org.oxycblt.auxio.music.metadata.parseMultiValue
 import org.oxycblt.auxio.playback.replaygain.ReplayGainAdjustment
 import org.oxycblt.auxio.util.positiveOrNull
 import org.oxycblt.auxio.util.toUuidOrNull
@@ -48,10 +48,12 @@ import org.oxycblt.auxio.util.update
  * Library-backed implementation of [Song].
  *
  * @param rawSong The [RawSong] to derive the member data from.
+ * @param separators The [Separators] to parse multi-value tags with.
  * @param musicSettings [MusicSettings] to for user parsing configuration.
  * @author Alexander Capehart (OxygenCobalt)
  */
-class SongImpl(private val rawSong: RawSong, musicSettings: MusicSettings) : Song {
+class SongImpl(private val rawSong: RawSong, musicSettings: MusicSettings, separators: Separators) :
+    Song {
     override val uid =
         // Attempt to use a MusicBrainz ID first before falling back to a hashed UID.
         rawSong.musicBrainzId?.toUuidOrNull()?.let { Music.UID.musicBrainz(MusicType.SONGS, it) }
@@ -95,42 +97,11 @@ class SongImpl(private val rawSong: RawSong, musicSettings: MusicSettings) : Son
             track = rawSong.replayGainTrackAdjustment, album = rawSong.replayGainAlbumAdjustment)
 
     override val dateAdded = requireNotNull(rawSong.dateAdded) { "Invalid raw: No date added" }
+
     private var _album: AlbumImpl? = null
     override val album: Album
         get() = unlikelyToBeNull(_album)
 
-    private val hashCode = 31 * uid.hashCode() + rawSong.hashCode()
-
-    override fun hashCode() = hashCode
-
-    override fun equals(other: Any?) =
-        other is SongImpl && uid == other.uid && rawSong == other.rawSong
-
-    override fun toString() = "Song(uid=$uid, name=$name)"
-
-    private val artistMusicBrainzIds = rawSong.artistMusicBrainzIds.parseMultiValue(musicSettings)
-    private val artistNames = rawSong.artistNames.parseMultiValue(musicSettings)
-    private val artistSortNames = rawSong.artistSortNames.parseMultiValue(musicSettings)
-    private val rawIndividualArtists =
-        artistNames.mapIndexed { i, name ->
-            RawArtist(
-                artistMusicBrainzIds.getOrNull(i)?.toUuidOrNull(),
-                name,
-                artistSortNames.getOrNull(i))
-        }
-
-    private val albumArtistMusicBrainzIds =
-        rawSong.albumArtistMusicBrainzIds.parseMultiValue(musicSettings)
-    private val albumArtistNames = rawSong.albumArtistNames.parseMultiValue(musicSettings)
-    private val albumArtistSortNames = rawSong.albumArtistSortNames.parseMultiValue(musicSettings)
-    private val rawAlbumArtists =
-        albumArtistNames.mapIndexed { i, name ->
-            RawArtist(
-                albumArtistMusicBrainzIds.getOrNull(i)?.toUuidOrNull(),
-                name,
-                albumArtistSortNames.getOrNull(i))
-        }
-
     private val _artists = mutableListOf<ArtistImpl>()
     override val artists: List<Artist>
         get() = _artists
@@ -139,44 +110,87 @@ class SongImpl(private val rawSong: RawSong, musicSettings: MusicSettings) : Son
     override val genres: List<Genre>
         get() = _genres
 
+    private val hashCode = 31 * uid.hashCode() + rawSong.hashCode()
+
+    override fun hashCode() = hashCode
+
+    // TODO: I cant compare by raw information actually, as it also means that any settings
+    //  configuration will be lost as well.
+    override fun equals(other: Any?) =
+        other is SongImpl && uid == other.uid && rawSong == other.rawSong
+
+    override fun toString() = "Song(uid=$uid, name=$name)"
+
     /**
      * The [RawAlbum] instances collated by the [Song]. This can be used to group [Song]s into an
      * [Album].
      */
-    val rawAlbum =
+    val rawAlbum: RawAlbum
-        RawAlbum(
-            mediaStoreId = requireNotNull(rawSong.albumMediaStoreId) { "Invalid raw: No album id" },
-            musicBrainzId = rawSong.albumMusicBrainzId?.toUuidOrNull(),
-            name = requireNotNull(rawSong.albumName) { "Invalid raw: No album name" },
-            sortName = rawSong.albumSortName,
-            releaseType = ReleaseType.parse(rawSong.releaseTypes.parseMultiValue(musicSettings)),
-            rawArtists =
-                rawAlbumArtists
-                    .ifEmpty { rawIndividualArtists }
-                    .distinctBy { it.key }
-                    .ifEmpty { listOf(RawArtist(null, null)) })
 
     /**
      * The [RawArtist] instances collated by the [Song]. The artists of the song take priority,
      * followed by the album artists. If there are no artists, this field will be a single "unknown"
      * [RawArtist]. This can be used to group up [Song]s into an [Artist].
      */
-    val rawArtists =
+    val rawArtists: List<RawArtist>
-        rawIndividualArtists
-            .ifEmpty { rawAlbumArtists }
-            .distinctBy { it.key }
-            .ifEmpty { listOf(RawArtist()) }
 
     /**
      * The [RawGenre] instances collated by the [Song]. This can be used to group up [Song]s into a
      * [Genre]. ID3v2 Genre names are automatically converted to their resolved names.
      */
-    val rawGenres =
+    val rawGenres: List<RawGenre>
-        rawSong.genreNames
+
-            .parseId3GenreNames(musicSettings)
+    init {
-            .map { RawGenre(it) }
+        val artistMusicBrainzIds = separators.split(rawSong.artistMusicBrainzIds)
-            .distinctBy { it.key }
+        val artistNames = separators.split(rawSong.artistNames)
-            .ifEmpty { listOf(RawGenre()) }
+        val artistSortNames = separators.split(rawSong.artistSortNames)
+        val rawIndividualArtists =
+            artistNames
+                .mapIndexedTo(mutableSetOf()) { i, name ->
+                    RawArtist(
+                        artistMusicBrainzIds.getOrNull(i)?.toUuidOrNull(),
+                        name,
+                        artistSortNames.getOrNull(i))
+                }
+                .toList()
+
+        val albumArtistMusicBrainzIds = separators.split(rawSong.albumArtistMusicBrainzIds)
+        val albumArtistNames = separators.split(rawSong.albumArtistNames)
+        val albumArtistSortNames = separators.split(rawSong.albumArtistSortNames)
+        val rawAlbumArtists =
+            albumArtistNames
+                .mapIndexedTo(mutableSetOf()) { i, name ->
+                    RawArtist(
+                        albumArtistMusicBrainzIds.getOrNull(i)?.toUuidOrNull(),
+                        name,
+                        albumArtistSortNames.getOrNull(i))
+                }
+                .toList()
+
+        rawAlbum =
+            RawAlbum(
+                mediaStoreId =
+                    requireNotNull(rawSong.albumMediaStoreId) { "Invalid raw: No album id" },
+                musicBrainzId = rawSong.albumMusicBrainzId?.toUuidOrNull(),
+                name = requireNotNull(rawSong.albumName) { "Invalid raw: No album name" },
+                sortName = rawSong.albumSortName,
+                releaseType = ReleaseType.parse(separators.split(rawSong.releaseTypes)),
+                rawArtists =
+                    rawAlbumArtists
+                        .ifEmpty { rawIndividualArtists }
+                        .ifEmpty { listOf(RawArtist()) })
+
+        rawArtists =
+            rawIndividualArtists.ifEmpty { rawAlbumArtists }.ifEmpty { listOf(RawArtist()) }
+
+        val genreNames =
+            (rawSong.genreNames.parseId3GenreNames() ?: separators.split(rawSong.genreNames))
+        rawGenres =
+            genreNames
+                .mapTo(mutableSetOf()) { RawGenre(it) }
+                .toList()
+                .ifEmpty { listOf(RawGenre()) }
+    }
 
     /**
      * Links this [Song] with a parent [Album].

app/src/main/java/org/oxycblt/auxio/music/metadata/Separators.kt

@@ -0,0 +1,59 @@
+/*
+ * Copyright (c) 2023 Auxio Project
+ * Separators.kt is part of Auxio.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+ 
+package org.oxycblt.auxio.music.metadata
+
+/**
+ * Defines the user-specified parsing of multi-value tags. This should be used to parse any tags
+ * that may be delimited with a separator character.
+ *
+ * @author Alexander Capehart (OxygenCobalt)
+ */
+interface Separators {
+    /**
+     * Parse a separated value from one or more strings. If the value is already composed of more
+     * than one value, nothing is done. Otherwise, it will attempt to split it based on the user's
+     * separator preferences.
+     *
+     * @return A new list of one or more [String]s parsed by the separator configuration
+     */
+    fun split(strings: List<String>): List<String>
+
+    companion object {
+        const val COMMA = ','
+        const val SEMICOLON = ';'
+        const val SLASH = '/'
+        const val PLUS = '+'
+        const val AND = '&'
+
+        fun from(selector: String) =
+            if (selector.isNotEmpty()) CharSeparators(selector.toSet()) else NoSeparators
+    }
+}
+
+private data class CharSeparators(private val chars: Set<Char>) : Separators {
+    override fun split(strings: List<String>) =
+        if (strings.size == 1) splitImpl(strings.first()) else strings
+
+    private fun splitImpl(string: String) =
+        string.splitEscaped { chars.contains(it) }.correctWhitespace()
+}
+
+private object NoSeparators : Separators {
+    override fun split(strings: List<String>) = strings
+}

app/src/main/java/org/oxycblt/auxio/music/metadata/SeparatorsDialog.kt

@@ -52,7 +52,7 @@ class SeparatorsDialog : ViewBindingMaterialDialogFragment<DialogSeparatorsBindi
             .setTitle(R.string.set_separators)
             .setNegativeButton(R.string.lbl_cancel, null)
             .setPositiveButton(R.string.lbl_save) { _, _ ->
-                musicSettings.multiValueSeparators = getCurrentSeparators()
+                musicSettings.separators = getCurrentSeparators()
             }
     }
 
@@ -68,8 +68,7 @@ class SeparatorsDialog : ViewBindingMaterialDialogFragment<DialogSeparatorsBindi
         // More efficient to do one iteration through the separator list and initialize
         // the corresponding CheckBox for each character instead of doing an iteration
         // through the separator list for each CheckBox.
-        (savedInstanceState?.getString(KEY_PENDING_SEPARATORS)
+        (savedInstanceState?.getString(KEY_PENDING_SEPARATORS) ?: musicSettings.separators)
-                ?: musicSettings.multiValueSeparators)
             .forEach {
                 when (it) {
                     Separators.COMMA -> binding.separatorComma.isChecked = true
@@ -102,14 +101,6 @@ class SeparatorsDialog : ViewBindingMaterialDialogFragment<DialogSeparatorsBindi
         return separators
     }
 
-    private object Separators {
-        const val COMMA = ','
-        const val SEMICOLON = ';'
-        const val SLASH = '/'
-        const val PLUS = '+'
-        const val AND = '&'
-    }
-
     private companion object {
         const val KEY_PENDING_SEPARATORS = BuildConfig.APPLICATION_ID + ".key.PENDING_SEPARATORS"
     }

app/src/main/java/org/oxycblt/auxio/music/metadata/TagUtil.kt

@@ -18,27 +18,10 @@
  
 package org.oxycblt.auxio.music.metadata
 
-import org.oxycblt.auxio.music.MusicSettings
 import org.oxycblt.auxio.util.positiveOrNull
 
 /// --- GENERIC PARSING ---
 
-/**
- * Parse a multi-value tag based on the user configuration. If the value is already composed of more
- * than one value, nothing is done. Otherwise, this function will attempt to split it based on the
- * user's separator preferences.
- *
- * @param settings [MusicSettings] required to obtain user separator configuration.
- * @return A new list of one or more [String]s.
- */
-fun List<String>.parseMultiValue(settings: MusicSettings) =
-    if (size == 1) {
-        first().maybeParseBySeparators(settings)
-    } else {
-        // Nothing to do.
-        this
-    }
-
 // TODO: Remove the escaping checks, it's too expensive to do this for every single tag.
 
 /**
@@ -101,17 +84,6 @@ fun String.correctWhitespace() = trim().ifBlank { null }
  */
 fun List<String>.correctWhitespace() = mapNotNull { it.correctWhitespace() }
 
-/**
- * Attempt to parse a string by the user's separator preferences.
- *
- * @param settings [MusicSettings] required to obtain user separator configuration.
- * @return A list of one or more [String]s that were split up by the user-defined separators.
- */
-private fun String.maybeParseBySeparators(settings: MusicSettings): List<String> {
-    if (settings.multiValueSeparators.isEmpty()) return listOf(this)
-    return splitEscaped { settings.multiValueSeparators.contains(it) }.correctWhitespace()
-}
-
 /// --- ID3v2 PARSING ---
 
 /**
@@ -165,12 +137,12 @@ fun transformPositionField(pos: Int?, total: Int?) =
  * representations of genre fields into their named counterparts, and split up singular ID3v2-style
  * integer genre fields into one or more genres.
  *
- * @param settings [MusicSettings] required to obtain user separator configuration.
+ * @return A list of one or more genre names, or null if this multi-value list has no valid
- * @return A list of one or more genre names..
+ *   formatting.
  */
-fun List<String>.parseId3GenreNames(settings: MusicSettings) =
+fun List<String>.parseId3GenreNames() =
     if (size == 1) {
-        first().parseId3MultiValueGenre(settings)
+        first().parseId3MultiValueGenre()
     } else {
         // Nothing to split, just map any ID3v1 genres to their name counterparts.
         map { it.parseId3v1Genre() ?: it }
@@ -179,11 +151,10 @@ fun List<String>.parseId3GenreNames(settings: MusicSettings) =
 /**
  * Parse a single ID3v1/ID3v2 integer genre field into their named representations.
  *
- * @param settings [MusicSettings] required to obtain user separator configuration.
+ * @return list of one or more genre names, or null if this is not in ID3v2 format.
- * @return A list of one or more genre names.
  */
-private fun String.parseId3MultiValueGenre(settings: MusicSettings) =
+private fun String.parseId3MultiValueGenre() =
-    parseId3v1Genre()?.let { listOf(it) } ?: parseId3v2Genre() ?: maybeParseBySeparators(settings)
+    parseId3v1Genre()?.let { listOf(it) } ?: parseId3v2Genre()
 
 /**
  * Parse an ID3v1 integer genre field.

app/src/test/java/org/oxycblt/auxio/music/info/NameTest.kt

@@ -27,7 +27,7 @@ import org.oxycblt.auxio.music.MusicSettings
 
 class NameTest {
     private fun mockIntelligentSorting(enabled: Boolean) =
-        mockk<MusicSettings>().apply { every { intelligentSorting } returns enabled }
+        mockk<MusicSettings>() { every { intelligentSorting } returns enabled }
 
     @Test
     fun name_from_simple_withoutPunct() {

app/src/test/java/org/oxycblt/auxio/music/metadata/SeparatorsTest.kt

@@ -0,0 +1,47 @@
+/*
+ * Copyright (c) 2023 Auxio Project
+ * SeparatorsTest.kt is part of Auxio.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+ 
+package org.oxycblt.auxio.music.metadata
+
+import org.junit.Assert.assertEquals
+import org.junit.Test
+
+class SeparatorsTest {
+    @Test
+    fun separators_split_withString_withSingleChar() {
+        assertEquals(listOf("a", "b", "c"), Separators.from(",").split(listOf("a,b,c")))
+    }
+
+    @Test
+    fun separators_split_withMultiple_withSingleChar() {
+        assertEquals(listOf("a,b", "c", "d"), Separators.from(",").split(listOf("a,b", "c", "d")))
+    }
+
+    @Test
+    fun separators_split_withString_withMultipleChar() {
+        assertEquals(
+            listOf("a", "b", "c", "d", "e", "f"),
+            Separators.from(",;/+&").split(listOf("a,b;c/d+e&f")))
+    }
+
+    @Test
+    fun separators_split_withList_withMultipleChar() {
+        assertEquals(
+            listOf("a,b;c/d", "e&f"), Separators.from(",;/+&").split(listOf("a,b;c/d", "e&f")))
+    }
+}

app/src/test/java/org/oxycblt/auxio/music/metadata/TagUtilTest.kt

@@ -18,34 +18,10 @@
  
 package org.oxycblt.auxio.music.metadata
 
-import io.mockk.every
-import io.mockk.mockk
 import org.junit.Assert.assertEquals
 import org.junit.Test
-import org.oxycblt.auxio.music.MusicSettings
 
 class TagUtilTest {
-    private fun mockSeparators(separators: String) =
-        mockk<MusicSettings>().apply { every { multiValueSeparators } returns separators }
-
-    @Test
-    fun parseMultiValue_single() {
-        assertEquals(listOf("a", "b", "c"), listOf("a,b,c").parseMultiValue(mockSeparators(",")))
-    }
-
-    @Test
-    fun parseMultiValue_many() {
-        assertEquals(
-            listOf("a", "b", "c"), listOf("a", "b", "c").parseMultiValue(mockSeparators(",")))
-    }
-
-    @Test
-    fun parseMultiValue_several() {
-        assertEquals(
-            listOf("a", "b", "c", "d", "e", "f"),
-            listOf("a,b;c/d+e&f").parseMultiValue(mockSeparators(",;/+&")))
-    }
-
     @Test
     fun splitEscaped_correct() {
         assertEquals(listOf("a", "b", "c"), "a,b,c".splitEscaped { it == ',' })
@@ -136,37 +112,30 @@ class TagUtilTest {
     fun parseId3v2Genre_multi() {
         assertEquals(
             listOf("Post-Rock", "Shoegaze", "Glitch"),
-            listOf("Post-Rock", "Shoegaze", "Glitch").parseId3GenreNames(mockSeparators(",")))
+            listOf("Post-Rock", "Shoegaze", "Glitch").parseId3GenreNames())
     }
 
     @Test
     fun parseId3v2Genre_multiId3v1() {
         assertEquals(
             listOf("Post-Rock", "Shoegaze", "Glitch"),
-            listOf("176", "178", "Glitch").parseId3GenreNames(mockSeparators(",")))
+            listOf("176", "178", "Glitch").parseId3GenreNames())
     }
 
     @Test
     fun parseId3v2Genre_wackId3() {
-        assertEquals(listOf("2941"), listOf("2941").parseId3GenreNames(mockSeparators(",")))
+        assertEquals(null, listOf("2941").parseId3GenreNames())
     }
 
     @Test
     fun parseId3v2Genre_singleId3v23() {
         assertEquals(
             listOf("Post-Rock", "Shoegaze", "Remix", "Cover", "Glitch"),
-            listOf("(176)(178)(RX)(CR)Glitch").parseId3GenreNames(mockSeparators(",")))
+            listOf("(176)(178)(RX)(CR)Glitch").parseId3GenreNames())
-    }
-
-    @Test
-    fun parseId3v2Genre_singleSeparated() {
-        assertEquals(
-            listOf("Post-Rock", "Shoegaze", "Glitch"),
-            listOf("Post-Rock, Shoegaze, Glitch").parseId3GenreNames(mockSeparators(",")))
     }
 
     @Test
     fun parsId3v2Genre_singleId3v1() {
-        assertEquals(listOf("Post-Rock"), listOf("176").parseId3GenreNames(mockSeparators(",")))
+        assertEquals(listOf("Post-Rock"), listOf("176").parseId3GenreNames())
     }
 }