address review: fix reveal anchoring and ID3v1 names

RevealNewFrontItem compared the post-anchor firstVisibleItemIndex against the
front threshold, so it only worked when the row was at index 0 and exactly one
item was prepended. Recover the pre-change position by subtracting the number
of items now before the old front, and cover the one-in and multi-prepend cases
with integration tests.

Adopt the canonical TagLib/mutagen names for two extended indices: 133 is
Worldbeat (was Negerpunk) and 148 is Abstract (was Christmas). Drop the unused
default on SongInfoDialog's genres argument.
This commit is contained in:
austinried
2026-10-04 00:30:07 +00:00
parent e8ac683b42
commit c3bfb612f8
6 changed files with 87 additions and 21 deletions
@@ -146,7 +146,7 @@ private val ID3V1_GENRES =
"Terror",
"Indie",
"Britpop",
"Negerpunk",
"Worldbeat",
"Polsk Punk",
"Beat",
"Christian Gangsta Rap",
@@ -161,7 +161,7 @@ private val ID3V1_GENRES =
"Anime",
"Jpop",
"Synthpop",
"Christmas",
"Abstract",
"Art Rock",
"Baroque",
"Bhangra",
@@ -36,7 +36,7 @@ fun SongInfoDialog(
localFile: File?,
streamEncoding: AudioEncoding?,
onDismiss: () -> Unit,
genres: List<String> = emptyList(),
genres: List<String>,
) {
val context = LocalContext.current
var encoding by remember(song.id, localFile) { mutableStateOf<AudioEncoding?>(null) }
@@ -535,13 +535,19 @@ private const val REVEAL_NEAR_FRONT = 1
internal fun shouldRevealNewFront(
previousKey: Any?,
firstKey: Any?,
insertedCount: Int,
firstVisibleItemIndex: Int,
): Boolean = firstKey != null && previousKey != null && previousKey != firstKey && firstVisibleItemIndex <= REVEAL_NEAR_FRONT
): Boolean =
firstKey != null &&
previousKey != null &&
previousKey != firstKey &&
insertedCount >= 0 &&
firstVisibleItemIndex - insertedCount <= REVEAL_NEAR_FRONT
/**
* Scrolls a row back to the front when a new first item appears while it is already at/near the
* start. The lazy layout anchors to the previously visible key, so an item that moves to the front
* after a play lands left of the viewport and is not revealed.
* start. The lazy layout anchors to the previously visible key, so a row at the start advances by
* the number of prepended items; subtracting that recovers the position the user was actually at.
*/
@Composable
internal fun <T> RevealNewFrontItem(
@@ -552,7 +558,11 @@ internal fun <T> RevealNewFrontItem(
val firstKey = items.firstOrNull()?.let(key)
var previous by remember { mutableStateOf<Any?>(null) }
LaunchedEffect(firstKey) {
if (shouldRevealNewFront(previous, firstKey, state.firstVisibleItemIndex)) state.animateScrollToItem(0)
val oldFront = previous
val inserted = if (oldFront != null) items.indexOfFirst { key(it) == oldFront } else -1
if (shouldRevealNewFront(oldFront, firstKey, inserted, state.firstVisibleItemIndex)) {
state.animateScrollToItem(0)
}
if (firstKey != null) previous = firstKey
}
}
@@ -10,6 +10,8 @@ class Id3v1GenresTest {
assertEquals("Blues", id3v1Genre("0"))
assertEquals("Rock", id3v1Genre("17"))
assertEquals("Electronic", id3v1Genre("52"))
assertEquals("Worldbeat", id3v1Genre("133"))
assertEquals("Abstract", id3v1Genre("148"))
assertEquals("Psybient", id3v1Genre("191"))
}
@@ -9,6 +9,7 @@ import androidx.compose.foundation.lazy.LazyRow
import androidx.compose.foundation.lazy.items
import androidx.compose.foundation.lazy.rememberLazyListState
import androidx.compose.material3.Text
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.mutableStateOf
import androidx.compose.ui.Modifier
import androidx.compose.ui.test.junit4.v2.createAndroidComposeRule
@@ -16,7 +17,9 @@ import androidx.compose.ui.unit.dp
import androidx.test.ext.junit.runners.AndroidJUnit4
import com.github.takahirom.roborazzi.RobolectricDeviceQualifiers
import com.subtracks.ui.theme.SubtracksTheme
import kotlinx.coroutines.runBlocking
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
@@ -33,6 +36,52 @@ class HomeRevealScrollTest {
@Test
fun aNewFrontItemAtTheStartScrollsBack() {
val labels = mutableStateOf(listOf("a", "b", "c", "d", "e"))
val state = render(labels)
composeRule.runOnIdle { labels.value = listOf("z") + labels.value }
composeRule.waitForIdle()
assertEquals(0, state.firstVisibleItemIndex)
}
@Test
fun aNewFrontItemOneRowInScrollsBack() {
val labels = mutableStateOf(listOf("a", "b", "c", "d", "e"))
val state = render(labels)
composeRule.runOnIdle { runBlocking { state.scrollToItem(1) } }
composeRule.waitForIdle()
composeRule.runOnIdle { labels.value = listOf("z") + labels.value }
composeRule.waitForIdle()
assertEquals(0, state.firstVisibleItemIndex)
}
@Test
fun twoNewFrontItemsAtTheStartScrollBack() {
val labels = mutableStateOf(listOf("a", "b", "c", "d", "e"))
val state = render(labels)
composeRule.runOnIdle { labels.value = listOf("y", "z") + labels.value }
composeRule.waitForIdle()
assertEquals(0, state.firstVisibleItemIndex)
}
@Test
fun aNewFrontItemDoesNotYankAScrolledAwayRow() {
val labels = mutableStateOf(listOf("a", "b", "c", "d", "e"))
val state = render(labels)
composeRule.runOnIdle { runBlocking { state.scrollToItem(3) } }
composeRule.waitForIdle()
composeRule.runOnIdle { labels.value = listOf("z") + labels.value }
composeRule.waitForIdle()
assertTrue(state.firstVisibleItemIndex > 1)
}
private fun render(labels: MutableState<List<String>>): LazyListState {
lateinit var state: LazyListState
composeRule.setContent {
SubtracksTheme {
@@ -46,11 +95,6 @@ class HomeRevealScrollTest {
}
}
composeRule.waitForIdle()
assertEquals(0, state.firstVisibleItemIndex)
composeRule.runOnIdle { labels.value = listOf("z") + labels.value }
composeRule.waitForIdle()
assertEquals(0, state.firstVisibleItemIndex)
return state
}
}
@@ -7,31 +7,41 @@ import org.junit.Test
class HomeRevealTest {
@Test
fun noPreviousFrontDoesNotReveal() {
assertFalse(shouldRevealNewFront(previousKey = null, firstKey = "a", firstVisibleItemIndex = 0))
assertFalse(shouldRevealNewFront(previousKey = null, firstKey = "a", insertedCount = 1, firstVisibleItemIndex = 1))
}
@Test
fun noFrontItemDoesNotReveal() {
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = null, firstVisibleItemIndex = 0))
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = null, insertedCount = 1, firstVisibleItemIndex = 1))
}
@Test
fun anUnchangedFrontDoesNotReveal() {
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = "a", firstVisibleItemIndex = 0))
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = "a", insertedCount = 0, firstVisibleItemIndex = 0))
}
@Test
fun aChangedFrontRevealsAtTheStart() {
assertTrue(shouldRevealNewFront(previousKey = "a", firstKey = "b", firstVisibleItemIndex = 0))
fun aMissingPreviousFrontDoesNotReveal() {
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = "b", insertedCount = -1, firstVisibleItemIndex = 0))
}
@Test
fun aChangedFrontRevealsOneRowIn() {
assertTrue(shouldRevealNewFront(previousKey = "a", firstKey = "b", firstVisibleItemIndex = 1))
fun aChangedFrontRevealsWhenTheRowWasAtTheStart() {
assertTrue(shouldRevealNewFront(previousKey = "a", firstKey = "b", insertedCount = 1, firstVisibleItemIndex = 1))
}
@Test
fun aChangedFrontRevealsWhenTheRowWasOneIn() {
assertTrue(shouldRevealNewFront(previousKey = "a", firstKey = "b", insertedCount = 1, firstVisibleItemIndex = 2))
}
@Test
fun aChangedFrontRevealsWhenSeveralItemsArePrepended() {
assertTrue(shouldRevealNewFront(previousKey = "a", firstKey = "c", insertedCount = 2, firstVisibleItemIndex = 2))
}
@Test
fun aChangedFrontDoesNotRevealWhenScrolledAway() {
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = "b", firstVisibleItemIndex = 2))
assertFalse(shouldRevealNewFront(previousKey = "a", firstKey = "b", insertedCount = 1, firstVisibleItemIndex = 3))
}
}