diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/ComingInSheet.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/ComingInSheet.kt index d02b4835..66d94daa 100644 --- a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/ComingInSheet.kt +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/ComingInSheet.kt @@ -98,8 +98,8 @@ fun ComingInSheet(shell: ShellState, modifier: Modifier = Modifier) { data.ordered.subList(start, (start + state.windowSize).coerceAtMost(data.ordered.size)) } LazyColumn(Modifier.clipToBounds().then(if (data.status == ComingInStatus.Unavailable) Modifier.weight(1f, fill = false) else Modifier), verticalArrangement = Arrangement.spacedBy(T.listGap)) { - itemsIndexed(visibleRows, - key = { _, item -> item.id }, contentType = { _, _ -> "request" }) { _, item -> + val keys = visibleRows.map { it.id }.uniqueKeys() + itemsIndexed(visibleRows, key = { index, _ -> keys[index] }, contentType = { _, _ -> "request" }) { _, item -> ComingInRow(item, shell.person, !state.recoveryFocused && item.id == state.focusedId, state.expanded && item.id == state.focusedId, state.ratingsVisible && data.signedIn && item.id == state.focusedId, onFocus = { state.focus(item.id) }, onClick = { state.focus(item.id); shell.activateRequest() }) diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/HomeShelf.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/HomeShelf.kt index 4f29d5f1..01555c7c 100644 --- a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/HomeShelf.kt +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/HomeShelf.kt @@ -49,7 +49,8 @@ fun HomeShelf(row: HomeRow, memory: HomeViewMemory, focused: Boolean, column: In if (!row.skeleton) Text(row.title, color = C.fg, style = T.rowHeading, maxLines = 1, modifier = Modifier.offset(x = L.safeSide)) val cards = if (row.skeleton) List(if (row.cardKind == HomeCardKind.Poster) 8 else 5) { null } else row.cards + if (row.hasSeeAll) listOf(null) else emptyList() LazyRow(state = track, modifier = Modifier.fillMaxWidth().offset(y = L.rowHeadingBand - L.rowGapAfter).height(height + caption + L.rowGapAfter * 2).skeletonPulse(row.skeleton).rowReveal(row.id, !row.skeleton), contentPadding = PaddingValues(horizontal = L.safeSide, vertical = L.rowGapAfter), horizontalArrangement = Arrangement.spacedBy(K.posterGap), userScrollEnabled = !row.skeleton) { - itemsIndexed(cards, key = { index, title -> title?.id ?: "${row.id}:tail:$index" }, contentType = { _, title -> if (row.skeleton) "skeleton" else if (title == null) "seeAll" else row.cardKind.name.lowercase() }) { index, title -> + val keys = cards.mapIndexed { index, title -> title?.id ?: "${row.id}:tail:$index" }.uniqueKeys() + itemsIndexed(cards, key = { index, _ -> keys[index] }, contentType = { _, title -> if (row.skeleton) "skeleton" else if (title == null) "seeAll" else row.cardKind.name.lowercase() }) { index, title -> val selected = !row.skeleton && focused && column == index val cardModifier = Modifier.zIndex(if (selected) 2f else 0f).testTag("home-card:${row.id}:$index") .then(if (!row.skeleton) Modifier.focusOnPointerEnter { onHover(index) } else Modifier) diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LazyKeys.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LazyKeys.kt new file mode 100644 index 00000000..b2cdcf1d --- /dev/null +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LazyKeys.kt @@ -0,0 +1,21 @@ +package org.justinvisser.wolfskin.design + +/** + * Lazy lists throw "Key was already used" when two items share a key, and server lists can repeat an identity + * (one person credited for several roles, a title listed twice). The first occurrence keeps its plain key so focus + * and scroll restoration by key are unchanged for every list that has no repeat; each repeat gets a stable + * `key#n` that cannot collide with another key in the list. + */ +internal fun List.uniqueKeys(): List { + val used = HashSet(size) + val repeats = HashMap() + return map { base -> + var key = base + while (!used.add(key)) { + val count = (repeats[base] ?: 0) + 1 + repeats[base] = count + key = "$base#$count" + } + key + } +} diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LibraryScreen.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LibraryScreen.kt index 322437be..bef9e51a 100644 --- a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LibraryScreen.kt +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/LibraryScreen.kt @@ -129,10 +129,13 @@ fun LibraryScreen( LibraryStatus.Unavailable -> item(key = "unavailable", contentType = "state") { LibraryRecovery(grid, active) } LibraryStatus.Ready -> if (items.isEmpty()) item(key = "empty", contentType = "state") { LibraryRecovery(grid, active && !grid.toolbar) } else { - items((items.size + grid.columns - 1) / grid.columns, key = { row -> + val rowCount = (items.size + grid.columns - 1) / grid.columns + // Keys come from the same items and column count as the row count, never from an earlier composition. + val rowKeys = (0 until rowCount).map { row -> val title = items[row * grid.columns] "grid-${if (grid.catalogue) title.catalogueKey else title.id}" - }, contentType = { "grid" }) { row -> + }.uniqueKeys() + items(rowCount, key = { rowKeys[it] }, contentType = { "grid" }) { row -> Box(Modifier.fillMaxWidth().height(rowHeight)) { Row(Modifier.padding(start = L.safeSide).rowReveal("grid-$row"), horizontalArrangement = Arrangement.spacedBy(K.posterGap)) { for (column in 0 until grid.columns) { diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/PersonPage.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/PersonPage.kt index 3f5be1e8..c6d45ab7 100644 --- a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/PersonPage.kt +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/PersonPage.kt @@ -146,7 +146,9 @@ private fun PersonPage(state: PersonState, shell: ShellState, modifier: Modifier item(key = "filmography", contentType = "header") { Text("Filmography", color = C.fg, style = TitleTextStyles.rowHeading, modifier = Modifier.height(L.row_gh).padding(start = L.safeSide, top = D.gridHeadingTop)) } - itemsIndexed(state.credits.chunked(columns), key = { _, row -> row.first().id }, contentType = { _, _ -> "grid" }) { rowIndex, row -> + val rowsOfCredits = state.credits.chunked(columns) + val rowKeys = rowsOfCredits.map { it.first().id }.uniqueKeys() + itemsIndexed(rowsOfCredits, key = { index, _ -> rowKeys[index] }, contentType = { _, _ -> "grid" }) { rowIndex, row -> Row(Modifier.height(D.gridHeight).padding(start = L.safeSide), horizontalArrangement = Arrangement.spacedBy(WolfskinTokens.Card.posterGap)) { row.forEachIndexed { column, item -> val index = rowIndex * columns + column diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitlePage.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitlePage.kt index b4e29411..81fb0ce0 100644 --- a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitlePage.kt +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitlePage.kt @@ -204,8 +204,10 @@ private fun TitleSeasons(state: TitleState, active: Boolean) { } } androidx.compose.foundation.lazy.LazyRow(Modifier.height(L.row_chips).padding(top = D.chipTop), state = list, contentPadding = androidx.compose.foundation.layout.PaddingValues(horizontal = L.safeSide), horizontalArrangement = Arrangement.spacedBy(D.chipGap)) { - items(state.data.seasonNumbers.size, key = { "season-${state.data.seasonNumbers[it]}" }, contentType = { "chip" }) { index -> - val number = state.data.seasonNumbers[index] + val seasonNumbers = state.data.seasonNumbers + val keys = seasonNumbers.map { "season-$it" }.uniqueKeys() + items(seasonNumbers.size, key = { keys[it] }, contentType = { "chip" }) { index -> + val number = seasonNumbers[index] val focused = active && state.column == index val selected = state.season == number val shape = RoundedCornerShape(WolfskinTokens.Radius.chip) diff --git a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitleShelf.kt b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitleShelf.kt index 708bd7ca..63423a91 100644 --- a/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitleShelf.kt +++ b/wolfskin-design/src/main/java/org/justinvisser/wolfskin/design/TitleShelf.kt @@ -59,7 +59,9 @@ fun TitleCast(state: TitleState, active: Boolean) { items(WolfskinTokens.Card.personSlots, key = { "cast-skeleton-$it" }, contentType = { "skeleton" }) { Box(Modifier.size(L.castCircle).background(C.surface2, CircleShape)) } - } else itemsIndexed(state.data.cast, key = { _, person -> person.id }, contentType = { _, _ -> "person" }) { index, person -> + } else { + val keys = state.data.cast.map { it.id }.uniqueKeys() + itemsIndexed(state.data.cast, key = { index, _ -> keys[index] }, contentType = { _, _ -> "person" }) { index, person -> val focused = active && state.column == index Box(Modifier.size(WolfskinTokens.Card.personW, WolfskinTokens.Card.personH).zIndex(if (focused) 1f else 0f) .onGloballyPositioned { state.recordCenter("cast", index, it.boundsInRoot().center.x) } @@ -71,6 +73,7 @@ fun TitleCast(state: TitleState, active: Boolean) { } } } + } } } } @@ -97,11 +100,14 @@ fun TitleMediaShelf(state: TitleState, episodes: Boolean, active: Boolean) { items(visible, key = { "episode-skeleton-$it" }, contentType = { "skeleton" }) { index -> Box(Modifier.size(width, height).testTag("title-episode-slot-$index").background(C.surface2, RoundedCornerShape(WolfskinTokens.Radius.card))) } - } else itemsIndexed(titles, key = { _, item -> item.id }, contentType = { _, _ -> if (episodes) "tile" else "poster" }) { index, item -> + } else { + val keys = titles.map { it.id }.uniqueKeys() + itemsIndexed(titles, key = { index, _ -> keys[index] }, contentType = { _, _ -> if (episodes) "tile" else "poster" }) { index, item -> TitlePoster(item, active && state.column == index, Modifier.width(width).then(if (episodes) Modifier.testTag("title-episode-slot-$index") else Modifier) .onGloballyPositioned { state.recordCenter(id, index, it.boundsInRoot().center.x) }, episodes, state.data.connection != TitleConnection.SignedOut, onFocus = { state.focus(id, index) }) { state.click(id, index) } } + } } } } diff --git a/wolfskin-design/src/test/java/org/justinvisser/wolfskin/design/DuplicateLazyKeysTest.kt b/wolfskin-design/src/test/java/org/justinvisser/wolfskin/design/DuplicateLazyKeysTest.kt new file mode 100644 index 00000000..ea7d0977 --- /dev/null +++ b/wolfskin-design/src/test/java/org/justinvisser/wolfskin/design/DuplicateLazyKeysTest.kt @@ -0,0 +1,103 @@ +package org.justinvisser.wolfskin.design + +import android.app.Application +import androidx.compose.foundation.layout.requiredSize +import androidx.compose.ui.Modifier +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.test.onAllNodesWithTag +import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.unit.dp +import org.junit.Assert.assertEquals +import org.junit.Assert.assertTrue +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Lazy lists throw "Key ... was already used" in measure when two items share an id. Server lists can repeat one: + * a Jellyfin person is credited once per role, so the title page crashed on a cast with the same person twice. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34], application = Application::class, qualifiers = "w960dp-h540dp-xhdpi") +class DuplicateLazyKeysTest { + @get:Rule val rule = createComposeRule() + private val viewer = ShellPerson("justin", "Justin", "J", false) + private fun shell(route: String) = ShellState(listOf(viewer), viewer.key).apply { pickerVisible = false; pickerLaunch = false; enterDestination(route) } + private fun repeated(id: String, name: String = id) = TitleItem(id, name, "movie", held = true) + + @Test fun repeatedKeysKeepTheFirstAndSuffixTheRest() { + assertEquals(listOf("a", "b", "a#1", "a#2", "a#1#1"), listOf("a", "b", "a", "a", "a#1").uniqueKeys()) + assertEquals(listOf("a", "b"), listOf("a", "b").uniqueKeys()) + } + + @Test fun castCreditedForTwoRolesComposes() { + val state = TitleState(TitlePageData(repeated("film"), cast = listOf( + TitlePerson("person", "Actor", "Hero"), TitlePerson("other", "Other", "Friend"), TitlePerson("person", "Actor", "Villain"))), viewer) {} + rule.setContent { WolfskinTheme { TitleCast(state, active = false) } } + rule.waitForIdle() + assertEquals(2, rule.onAllNodesWithTag("title-cast-person").fetchSemanticsNodes().size) + rule.onNodeWithTag("title-cast-other").assertExists() + } + + @Test fun relatedTitleListedTwiceComposes() { + val state = TitleState(TitlePageData(repeated("film"), related = listOf(repeated("same"), repeated("other"), repeated("same"))), viewer) {} + rule.setContent { WolfskinTheme { TitleMediaShelf(state, episodes = false, active = false) } } + rule.waitForIdle() + rule.onNodeWithTag("title-shelf:related").assertExists() + } + + @Test fun episodeListedTwiceComposes() { + val series = TitleItem("series", "A series", "tv", seasons = 1, held = true) + val episode = TitleItem("episode", "Pilot", "episode", held = true, season = 1, episode = 1) + val state = TitleState(TitlePageData(series, playTarget = episode, episodes = listOf(episode, episode), connection = TitleConnection.SignedIn), viewer) {} + rule.setContent { WolfskinTheme { TitleMediaShelf(state, episodes = true, active = false) } } + rule.waitForIdle() + rule.onNodeWithTag("title-shelf:episodes").assertExists() + } + + @Test fun repeatedSeasonNumberComposes() { + val series = TitleItem("series", "A series", "tv", seasons = 2, held = true) + val state = TitleState(TitlePageData(series, seasonNumbers = listOf(1, 1, 2)), viewer) {} + rule.setContent { WolfskinTheme { TitleSurface(state, shell("title/series"), Modifier.requiredSize(960.dp, 540.dp)) } } + rule.waitForIdle() + rule.onNodeWithTag("title-shelf:episodes").assertDoesNotExist() + assertTrue(state.rows.contains("seasons")) + } + + @Test fun filmographyWithARepeatedCreditComposes() { + val state = PersonState(TitlePerson("person", "Actor"), List(WolfskinTokens.Behaviour.gridColumns.toInt() + 1) { repeated("credit") }, {}) + rule.setContent { WolfskinTheme { PersonSurface(state, shell("title/person"), Modifier.requiredSize(960.dp, 540.dp)) } } + rule.waitForIdle() + rule.onNodeWithTag("person-page").assertExists() + } + + @Test fun homeShelfWithARepeatedCardComposes() { + val card = HomeTitle("episode", "Pilot", "episode") + val snapshot = HomeSnapshot(listOf(HomeRow("cw", HomeRowKind.Shelf, "Continue watching", listOf(card, HomeTitle("other", "Other", "movie"), card), HomeCardKind.Wide)), HomeAvailability.Ready) + val shell = shell("home") + rule.setContent { WolfskinTheme { ShellSurface(shell) { modifier -> HomeScreen(shell, snapshot, modifier) {} } } } + rule.waitForIdle() + rule.onNodeWithTag("home-card:cw:0").assertExists() + rule.onNodeWithTag("home-card:cw:2").assertExists() + } + + @Test fun libraryGridWithARepeatedTitleComposes() { + val columns = WolfskinTokens.Behaviour.gridColumns.toInt() + val titles = List(columns * 2) { LibraryTitle("same", "Same title", "movie") } + val shell = shell("movies") + val grid = LibraryGridState("Movies", titles) + rule.setContent { WolfskinTheme { ShellSurface(shell) { modifier -> LibraryScreen(shell, grid, modifier) } } } + rule.waitForIdle() + rule.onNodeWithTag("library-grid").assertExists() + } + + @Test fun requestListedTwiceComposes() { + val request = ComingInRequest("r1", "movie", 1, "A film", "justin", "Justin", null, null, ComingInStage.Waiting) + val shell = shell("settings").apply { requests.update(ComingInSnapshot(listOf(request, request.copy(id = "r2"), request), ComingInStatus.Ready, signedIn = true)); openRequests() } + rule.setContent { WolfskinTheme { ShellSurface(shell) } } + rule.waitForIdle() + rule.onNodeWithTag("wolfskin-shell").assertExists() + } +}