commit b261247aa43ae317aed022705f66d614370fe311
author vinekumar <vinekumar@google.com> Fri Jan 27 22:31:37 2023 +0530
committer vinekumar <vinekumar@google.com> Sat Jan 28 14:40:33 2023 +0530
tree f3ec27406792192e979ebfb760767198a086d033
parent 4fd035860243984314ed5404e4755cc135de3a1a

Fixed possibility of slideIndex being greater than slideCount

There are two issues addressed here
* `AutoScrollSideEffect` has a `LaunchedEffect` which would not get
  updates to slideCount. Addressed this by using rememberUpdatedState.
* The slide-count could change when `AnimatedContent` is animating
  from one slide to the next. Addressed this by verifying the
  slideIndex against slideCount.

Bug: b/266160424

Test: Added a unit-test that fails without the current fix.
Change-Id: Idd3e84fcd30eec71b185a36cd6cafb59e871eb89

tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt

diff --git a/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt b/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt
index adb434b..5e2da37 100644
--- a/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt
+++ b/tv/tv-material/src/androidTest/java/androidx/tv/material3/CarouselTest.kt
@@ -65,6 +65,7 @@
 import androidx.compose.ui.unit.dp
 import androidx.test.platform.app.InstrumentationRegistry
 import com.google.common.truth.Truth.assertThat
+import kotlinx.coroutines.delay
 import org.junit.Rule
 import org.junit.Test
 
@@ -619,6 +620,7 @@
         rule.onNodeWithText("Play ${finalSlide + 3}").assertIsFocused()
     }
 
+    @Test
     fun carousel_manualScrolling_onDpadLongPress() {
         rule.setContent {
             SampleCarousel(slideCount = 6) { index ->
@@ -757,6 +759,39 @@
         // Assert that slide 1 is in view
         rule.onNodeWithText("Button 1").assertIsDisplayed()
     }
+
+    @Test
+    fun carousel_slideCountChangesDuringAnimation_shouldNotCrash() {
+        val slideDisplayDurationMs: Long = 100
+        var slideChanges = 0
+        // number of slides will fall from 4 to 2, but 4 slide transitions should happen without a
+        // crash
+        val minSuccessfulSlideChanges = 4
+        rule.setContent {
+            var slideCount by remember { mutableStateOf(4) }
+            LaunchedEffect(Unit) {
+                while (slideCount >= 2) {
+                    delay(slideDisplayDurationMs)
+                    slideCount--
+                }
+            }
+            SampleCarousel(
+                slideCount = slideCount,
+                timeToDisplaySlideMillis = slideDisplayDurationMs
+            ) { index ->
+                if (index >= slideCount) {
+                    // slideIndex requested should not be greater than slideCount. User could be
+                    // using a data-structure that could throw an IndexOutOfBoundsException.
+                    // This can happen when the slideCount changes during the transition between
+                    // slides.
+                    throw Exception("Index is larger, index=$index, slideCount=$slideCount")
+                }
+                slideChanges++
+            }
+        }
+
+        rule.waitUntil(timeoutMillis = 5000) { slideChanges > minSuccessfulSlideChanges }
+    }
 }
 
 @OptIn(ExperimentalTvMaterial3Api::class)

tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt

diff --git a/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt b/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt
index 70c5480..2d04b43 100644
--- a/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt
+++ b/tv/tv-material/src/main/java/androidx/tv/material3/Carousel.kt
@@ -43,6 +43,7 @@
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
+import androidx.compose.runtime.rememberUpdatedState
 import androidx.compose.runtime.setValue
 import androidx.compose.runtime.snapshotFlow
 import androidx.compose.ui.Alignment
@@ -147,7 +148,7 @@
         AnimatedContent(
             targetState = carouselState.activeSlideIndex,
             transitionSpec = { enterTransition.with(exitTransition) }
-        ) {
+        ) { slideIndex ->
             LaunchedEffect(Unit) {
                 this@AnimatedContent.onAnimationCompletion {
                     // Outer box is focused
@@ -157,7 +158,13 @@
                     }
                 }
             }
-            content.invoke(it)
+            // it is possible for the slideCount to have changed during the transition.
+            // This can cause the slideIndex to be greater than or equal to slideCount and cause
+            // IndexOutOfBoundsException. Guarding against this by checking against slideCount
+            // before invoking.
+            if (slideCount > 0) {
+                content.invoke(if (slideIndex < slideCount) slideIndex else 0)
+            }
         }
         this.carouselIndicator()
     }
@@ -186,6 +193,8 @@
     doAutoScroll: Boolean,
     onAutoScrollChange: (isAutoScrollActive: Boolean) -> Unit = {},
 ) {
+    // Needed to ensure that the code within LaunchedEffect receives updates to the slideCount.
+    val updatedSlideCount by rememberUpdatedState(newValue = slideCount)
     if (doAutoScroll) {
         LaunchedEffect(carouselState) {
             while (true) {
@@ -195,7 +204,7 @@
                     snapshotFlow { carouselState.activePauseHandlesCount }
                         .first { pauseHandleCount -> pauseHandleCount == 0 }
                 }
-                carouselState.moveToNextSlide(slideCount)
+                carouselState.moveToNextSlide(updatedSlideCount)
             }
         }
     }