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

@@ -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

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)

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:

app/src/main/java/org/oxycblt/auxio/music/parsing/ParsingUtil.kt

@@ -107,12 +107,45 @@ private fun String.maybeParseBySeparators(settings: MusicSettings): List<String>
 /// --- 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

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