From 82a64b5e17516edeef7ae83470bddcfda5fdb77d Mon Sep 17 00:00:00 2001 From: Alexander Capehart Date: Sat, 21 Jan 2023 16:43:41 -0700 Subject: [PATCH] music: accept zero positions with non-zero totals Accept positions that are zeroed, but only if there are non-zero total values as well. Sometimes zeroed positions are deliberate, but other times they are placeholders meant to indicate a lack of a position. To resolve this, Auxio now considers zeroed track/disc numbers in the presence of non-zero track/disc numbers to be valid. --- CHANGELOG.md | 6 +++ .../music/extractor/MediaStoreExtractor.kt | 12 +++--- .../music/extractor/MetadataExtractor.kt | 23 ++++++---- .../auxio/music/parsing/ParsingUtil.kt | 43 ++++++++++++++++--- .../auxio/music/parsing/ParsingUtilTest.kt | 34 ++++++++++++--- 5 files changed, 92 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d5088867b..8d1b6ce7b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## dev + +#### What's Improved +- Auxio will now accept zeroed track/disc numbers in the presence of non-zero total +track/disc fields. + ## 3.0.2 #### What's New diff --git a/app/src/main/java/org/oxycblt/auxio/music/extractor/MediaStoreExtractor.kt b/app/src/main/java/org/oxycblt/auxio/music/extractor/MediaStoreExtractor.kt index 0145222aa..211d20a4b 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/extractor/MediaStoreExtractor.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/extractor/MediaStoreExtractor.kt @@ -29,7 +29,8 @@ import androidx.core.database.getStringOrNull import java.io.File import org.oxycblt.auxio.music.MusicSettings import org.oxycblt.auxio.music.Song -import org.oxycblt.auxio.music.parsing.parseId3v2Position +import org.oxycblt.auxio.music.parsing.parseId3v2PositionField +import org.oxycblt.auxio.music.parsing.transformPositionField import org.oxycblt.auxio.music.storage.Directory import org.oxycblt.auxio.music.storage.contentResolverSafe import org.oxycblt.auxio.music.storage.directoryCompat @@ -40,7 +41,6 @@ import org.oxycblt.auxio.music.storage.useQuery import org.oxycblt.auxio.music.tags.Date import org.oxycblt.auxio.util.getSystemServiceCompat import org.oxycblt.auxio.util.logD -import org.oxycblt.auxio.util.nonZeroOrNull /** * The layer that loads music from the [MediaStore] database. This is an intermediate step in the @@ -564,8 +564,8 @@ class Api30MediaStoreExtractor(context: Context, cacheExtractor: CacheExtractor) // the tag itself, which is to say that it is formatted as NN/TT tracks, where // N is the number and T is the total. Parse the number while ignoring the // total, as we have no use for it. - cursor.getStringOrNull(trackIndex)?.parseId3v2Position()?.let { raw.track = it } - cursor.getStringOrNull(discIndex)?.parseId3v2Position()?.let { raw.disc = it } + cursor.getStringOrNull(trackIndex)?.parseId3v2PositionField()?.let { raw.track = it } + cursor.getStringOrNull(discIndex)?.parseId3v2PositionField()?.let { raw.disc = it } } } @@ -576,7 +576,7 @@ class Api30MediaStoreExtractor(context: Context, cacheExtractor: CacheExtractor) * @return The track number extracted from the combined integer value, or null if the value was * zero. */ -private fun Int.unpackTrackNo() = mod(1000).nonZeroOrNull() +private fun Int.unpackTrackNo() = transformPositionField(mod(1000), null) /** * Unpack the disc number from a combined track + disc [Int] field. These fields appear within @@ -584,4 +584,4 @@ private fun Int.unpackTrackNo() = mod(1000).nonZeroOrNull() * disc number is the 4th+ digit. * @return The disc number extracted from the combined integer field, or null if the value was zero. */ -private fun Int.unpackDiscNo() = div(1000).nonZeroOrNull() +private fun Int.unpackDiscNo() = transformPositionField(div(1000), null) diff --git a/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataExtractor.kt b/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataExtractor.kt index b80b45882..cf7257194 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataExtractor.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/extractor/MetadataExtractor.kt @@ -23,7 +23,8 @@ import com.google.android.exoplayer2.MediaItem import com.google.android.exoplayer2.MetadataRetriever import kotlinx.coroutines.flow.flow import org.oxycblt.auxio.music.Song -import org.oxycblt.auxio.music.parsing.parseId3v2Position +import org.oxycblt.auxio.music.parsing.parseId3v2PositionField +import org.oxycblt.auxio.music.parsing.parseVorbisPositionField import org.oxycblt.auxio.music.storage.toAudioUri import org.oxycblt.auxio.music.tags.Date import org.oxycblt.auxio.util.logD @@ -182,10 +183,10 @@ class Task(context: Context, private val raw: Song.Raw) { textFrames["TSOT"]?.let { raw.sortName = it[0] } // Track. Only parse out the track number and ignore the total tracks value. - textFrames["TRCK"]?.run { first().parseId3v2Position() }?.let { raw.track = it } + textFrames["TRCK"]?.run { first().parseId3v2PositionField() }?.let { raw.track = it } // Disc. Only parse out the disc number and ignore the total discs value. - textFrames["TPOS"]?.run { first().parseId3v2Position() }?.let { raw.disc = it } + textFrames["TPOS"]?.run { first().parseId3v2PositionField() }?.let { raw.disc = it } // Dates are somewhat complicated, as not only did their semantics change from a flat year // value in ID3v2.3 to a full ISO-8601 date in ID3v2.4, but there are also a variety of @@ -278,13 +279,17 @@ class Task(context: Context, private val raw: Song.Raw) { comments["title"]?.let { raw.name = it[0] } comments["titlesort"]?.let { raw.sortName = it[0] } - // Track. The total tracks value is in a different comment, so we can just - // convert the entirety of this comment into a number. - comments["tracknumber"]?.run { first().toIntOrNull() }?.let { raw.track = it } + // Track. + parseVorbisPositionField( + comments["tracknumber"]?.first(), + (comments["totaltracks"] ?: comments["tracktotal"] ?: comments["trackc"])?.first()) + ?.let { raw.track = it } - // Disc. The total discs value is in a different comment, so we can just - // convert the entirety of this comment into a number. - comments["discnumber"]?.run { first().toIntOrNull() }?.let { raw.disc = it } + // Disc. + parseVorbisPositionField( + comments["discnumber"]?.first(), + (comments["totaldiscs"] ?: comments["disctotal"] ?: comments["discc"])?.first()) + ?.let { raw.disc = it } // Vorbis dates are less complicated, but there are still several types // Our hierarchy for dates is as such: diff --git a/app/src/main/java/org/oxycblt/auxio/music/parsing/ParsingUtil.kt b/app/src/main/java/org/oxycblt/auxio/music/parsing/ParsingUtil.kt index 658b1cbea..db9c53945 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/parsing/ParsingUtil.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/parsing/ParsingUtil.kt @@ -107,12 +107,45 @@ private fun String.maybeParseBySeparators(settings: MusicSettings): List /// --- ID3v2 PARSING --- /** - * Parse the number out of a ID3v2-style number + total position [String] field. These fields - * consist of a number and an (optional) total value delimited by a /. - * @return The number value extracted from the string field, or null if the value could not be - * parsed or if the value was zero. + * Parse an ID3v2-style position + total [String] field. These fields consist of a number and an + * (optional) total value delimited by a /. + * @return The position value extracted from the string field, or null if: + * - The position could not be parsed + * - The position was zeroed AND the total value was not present/zeroed + * @see transformPositionField */ -fun String.parseId3v2Position() = split('/', limit = 2)[0].toIntOrNull()?.nonZeroOrNull() +fun String.parseId3v2PositionField() = + split('/', limit = 2).let { + transformPositionField(it[0].toIntOrNull(), it.getOrNull(1)?.toIntOrNull()) + } + +/** + * Parse a vorbis-style position + total field. These fields consist of two fields for the position + * and total numbers. + * @param pos The position value, or null if not present. + * @param total The total value, if not present. + * @return The position value extracted from the field, or null if: + * - The position could not be parsed + * - The position was zeroed AND the total value was not present/zeroed + * @see transformPositionField + */ +fun parseVorbisPositionField(pos: String?, total: String?) = + transformPositionField(pos?.toIntOrNull(), total?.toIntOrNull()) + +/** + * Transform a raw position + total field into a position a way that tolerates placeholder values. + * @param pos The position value, or null if not present. + * @param total The total value, if not present. + * @return The position value extracted from the field, or null if: + * - The position could not be parsed + * - The position was zeroed AND the total value was not present/zeroed + */ +fun transformPositionField(pos: Int?, total: Int?) = + if (pos != null && (pos > 0 || (total?.nonZeroOrNull() != null))) { + pos + } else { + null + } /** * Parse a multi-value genre name using ID3 rules. This will convert any ID3v1 integer diff --git a/app/src/test/java/org/oxycblt/auxio/music/parsing/ParsingUtilTest.kt b/app/src/test/java/org/oxycblt/auxio/music/parsing/ParsingUtilTest.kt index 687653a14..1a3025447 100644 --- a/app/src/test/java/org/oxycblt/auxio/music/parsing/ParsingUtilTest.kt +++ b/app/src/test/java/org/oxycblt/auxio/music/parsing/ParsingUtilTest.kt @@ -92,18 +92,40 @@ class ParsingUtilTest { } @Test - fun parseId3v2Position_correct() { - assertEquals(16, "16/32".parseId3v2Position()) + fun parseId3v2PositionField_correct() { + assertEquals(16, "16/32".parseId3v2PositionField()) + assertEquals(16, "16".parseId3v2PositionField()) } @Test - fun parseId3v2Position_noTotal() { - assertEquals(16, "16".parseId3v2Position()) + fun parseId3v2PositionField_zeroed() { + assertEquals(null, "0".parseId3v2PositionField()) + assertEquals(0, "0/32".parseId3v2PositionField()) } @Test - fun parseId3v2Position_wack() { - assertEquals(16, "16/".parseId3v2Position()) + fun parseId3v2PositionField_wack() { + assertEquals(16, "16/".parseId3v2PositionField()) + assertEquals(null, "a".parseId3v2PositionField()) + assertEquals(null, "a/b".parseId3v2PositionField()) + } + + @Test + fun parseVorbisPositionField_correct() { + assertEquals(16, parseVorbisPositionField("16", "32")) + assertEquals(16, parseVorbisPositionField("16", null)) + } + + @Test + fun parseVorbisPositionField_zeroed() { + assertEquals(null, parseVorbisPositionField("0", null)) + assertEquals(0, parseVorbisPositionField("0", "32")) + } + + @Test + fun parseVorbisPositionField_wack() { + assertEquals(null, parseVorbisPositionField("a", null)) + assertEquals(null, parseVorbisPositionField("a", "b")) } @Test