Merge pull request #503 from arkivanov/fix-pages-onPageSelected-calle…

…d-multiple-times Fix Pages onPageSelected called multiple times and not rendering the current page sometimes
arkivanov · Oct 11, 2023 · 318949b · 318949b
2 parents 73018ed + 2892e7d
commit 318949b
Show file tree

Hide file tree

Showing 6 changed files with 174 additions and 32 deletions.
diff --git a/deps.versions.toml b/deps.versions.toml
@@ -25,6 +25,7 @@ androidxTestCore = "1.5.0"
 [libraries]
 
 kotlin-kotlinGradlePlug = { group = "org.jetbrains.kotlin", name = "kotlin-gradle-plugin", version.ref = "kotlin" }
+kotlin-test = { group = "org.jetbrains.kotlin", name = "kotlin-test", version.ref = "kotlin" }
 
 essenty-lifecycle = { group = "com.arkivanov.essenty", name = "lifecycle", version.ref = "essenty" }
 essenty-stateKeeper = { group = "com.arkivanov.essenty", name = "state-keeper", version.ref = "essenty" }

diff --git a/...src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/jetbrains/pages/Pages.kt b/...src/commonMain/kotlin/com/arkivanov/decompose/extensions/compose/jetbrains/pages/Pages.kt
@@ -11,6 +11,7 @@ import androidx.compose.runtime.DisposableEffect
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.State
 import androidx.compose.runtime.getValue
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import com.arkivanov.decompose.ExperimentalDecomposeApi
 import com.arkivanov.decompose.extensions.compose.jetbrains.subscribeAsState
@@ -58,7 +59,7 @@ fun <T : Any> Pages(
  pager: Pager = defaultHorizontalPager(),
  pageContent: @Composable PagerScope.(index: Int, page: T) -> Unit,
 ) {
- val childPages by pages 
+ val childPages by pages
  val selectedIndex = childPages.selectedIndex
  val state = rememberPagerState(
  initialPage = selectedIndex,
@@ -76,7 +77,10 @@ fun <T : Any> Pages(
  }
 
  DisposableEffect(state.currentPage) {
- onPageSelected(state.currentPage)
+ if (state.currentPage == state.targetPage) {
+ onPageSelected(state.currentPage)
+ }
+
  onDispose {}
  }
 
@@ -85,7 +89,9 @@ fun <T : Any> Pages(
  state,
  { childPages.items[it].configuration.hashString() },
  ) { pageIndex ->
- childPages.items[pageIndex].instance?.also { page ->
+ val item = childPages.items[pageIndex]
+ val page = remember(item.configuration) { item.instance }
+ if (page != null) {
  pageContent(pageIndex, page)
  }
  }

diff --git a/...rc/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/jetbrains/pages/PagesTest.kt b/...rc/jvmTest/kotlin/com/arkivanov/decompose/extensions/compose/jetbrains/pages/PagesTest.kt
@@ -15,14 +15,15 @@ import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.runBlocking
 import org.junit.Rule
 import kotlin.test.Test
+import kotlin.test.assertContentEquals
 
+@OptIn(ExperimentalDecomposeApi::class, ExperimentalFoundationApi::class)
 @Suppress("TestFunctionName")
 class PagesTest {
 
  @get:Rule
  val composeRule = createComposeRule()
 
- @OptIn(ExperimentalDecomposeApi::class)
  @Test
  fun GIVEN_pages_displayed_WHEN_page_state_changed_THEN_pageCount_updated() {
  runBlocking(Dispatchers.Main) {
@@ -39,7 +40,7 @@ class PagesTest {
  setContent(state)
  composeRule.onNodeWithText(text = "Page0", substring = true).assertExists()
 
- state.setValueOnIdle(
+ state.updateOnIdle {
  ChildPages(
  items = listOf(
  Child.Created(Config.Config1, Config.Config1),
@@ -48,13 +49,12 @@ class PagesTest {
  ),
  selectedIndex = 2,
  )
- )
+ }
 
  composeRule.onNodeWithText(text = "Page2", substring = true).assertExists()
  }
  }
 
- @OptIn(ExperimentalDecomposeApi::class)
  @Test
  fun GIVEN_page0_displayed_WHEN_switching_pages_THEN_current_page_updated() {
  runBlocking(Dispatchers.Main) {
@@ -71,18 +71,84 @@ class PagesTest {
  setContent(state)
  composeRule.onNodeWithText(text = "Page0", substring = true).assertExists()
 
- state.setValueOnIdle(state.value.copy(selectedIndex = 1))
+ state.updateOnIdle { it.copy(selectedIndex = 1) }
 
  composeRule.onNodeWithText(text = "Page1", substring = true).assertExists()
  }
  }
 
- @OptIn(ExperimentalDecomposeApi::class, ExperimentalFoundationApi::class)
- private suspend fun setContent(stack: State<ChildPages<Config, Config>>, ) {
+ @Test
+ fun GIVEN_pages_without_animation_WHEN_page_changed_from_0_to_2_THEN_onPageSelected_called_with_index_2() {
+ runBlocking(Dispatchers.Main) {
+ val state =
+ mutableStateOf(
+ ChildPages(
+ items = listOf(
+ Child.Created(Config.Config1, Config.Config1),
+ Child.Created(Config.Config2, Config.Config2),
+ Child.Created(Config.Config3, Config.Config3),
+ ),
+ selectedIndex = 0,
+ ),
+ )
+
+ val indices = ArrayList<Int>()
+
+ setContent(
+ pages = state,
+ onPageSelected = { indices += it },
+ )
+
+ indices.clear()
+
+ state.updateOnIdle { it.copy(selectedIndex = 2) }
+
+ assertContentEquals(listOf(2), indices)
+ }
+ }
+
+
+ @Test
+ fun GIVEN_pages_with_animation_WHEN_page_changed_from_0_to_2_THEN_onPageSelected_called_with_index_2() {
+ runBlocking(Dispatchers.Main) {
+ val state =
+ mutableStateOf(
+ ChildPages(
+ items = listOf(
+ Child.Created(Config.Config1, Config.Config1),
+ Child.Created(Config.Config2, Config.Config2),
+ Child.Created(Config.Config3, Config.Config3),
+ ),
+ selectedIndex = 0,
+ ),
+ )
+
+ val indices = ArrayList<Int>()
+
+ setContent(
+ pages = state,
+ onPageSelected = { indices += it },
+ scrollAnimation = PagesScrollAnimation.Default,
+ )
+
+ indices.clear()
+
+ state.updateOnIdle { it.copy(selectedIndex = 2) }
+
+ assertContentEquals(listOf(2), indices)
+ }
+ }
+
+ private suspend fun setContent(
+ pages: State<ChildPages<Config, Config>>,
+ onPageSelected: (index: Int) -> Unit = {},
+ scrollAnimation: PagesScrollAnimation = PagesScrollAnimation.Disabled,
+ ) {
  composeRule.setContent {
  Pages(
- pages = stack,
- onPageSelected = {},
+ pages = pages,
+ onPageSelected = onPageSelected,
+ scrollAnimation = scrollAnimation,
  ) { index, page ->
  Page(index, page)
  }
@@ -96,9 +162,9 @@ class PagesTest {
  BasicText(text = "Page$index=$page")
  }
 
- private suspend fun <T> MutableState<T>.setValueOnIdle(value: T) {
- runOnIdle { this.value = value }
- runOnIdle {}
+ private fun <T> MutableState<T>.updateOnIdle(func: (T) -> T) {
+ composeRule.runOnIdle { value = func(value) }
+ composeRule.runOnIdle {}
  }
 
  private suspend fun runOnIdle(block: () -> Unit) {
@@ -107,8 +173,8 @@ class PagesTest {
  }
 
  private sealed class Config {
- data object Config1: Config()
- data object Config2: Config()
- data object Config3: Config()
+ data object Config1 : Config()
+ data object Config2 : Config()
+ data object Config3 : Config()
  }
 }
diff --git a/extensions-compose-jetpack/build.gradle.kts b/extensions-compose-jetpack/build.gradle.kts
@@ -42,4 +42,5 @@ dependencies {
  androidTestImplementation(deps.junit.junit)
  androidTestImplementation(deps.androidx.compose.ui.uiTestManifest)
  androidTestImplementation(deps.androidx.test.core)
+ androidTestImplementation(deps.kotlin.test)
 }
diff --git a/...rc/androidTest/java/com/arkivanov/decompose/extensions/compose/jetpack/pages/PagesTest.kt b/...rc/androidTest/java/com/arkivanov/decompose/extensions/compose/jetpack/pages/PagesTest.kt
@@ -13,15 +13,15 @@ import com.arkivanov.decompose.ExperimentalDecomposeApi
 import com.arkivanov.decompose.router.pages.ChildPages
 import org.junit.Rule
 import org.junit.Test
-import java.io.Serializable
+import kotlin.test.assertContentEquals
 
+@OptIn(ExperimentalFoundationApi::class, ExperimentalDecomposeApi::class)
 @Suppress("TestFunctionName")
 class PagesTest {
 
  @get:Rule
  val composeRule = createComposeRule()
 
- @OptIn(ExperimentalDecomposeApi::class)
  @Test
  fun GIVEN_pages_displayed_WHEN_page_state_changed_THEN_pageCount_updated() {
  val state = mutableStateOf(
@@ -37,7 +37,7 @@ class PagesTest {
  setContent(state)
  composeRule.onNodeWithText(text = "Page0", substring = true).assertExists()
 
- state.setValueOnIdle(
+ state.updateOnIdle {
  ChildPages(
  items = listOf(
  Child.Created(Config.Config1, Config.Config1),
@@ -46,12 +46,11 @@ class PagesTest {
  ),
  selectedIndex = 2,
  )
- )
+ }
 
  composeRule.onNodeWithText(text = "Page2", substring = true).assertExists()
  }
 
- @OptIn(ExperimentalDecomposeApi::class)
  @Test
  fun GIVEN_page0_displayed_WHEN_switching_pages_THEN_current_page_updated() {
  val state = mutableStateOf(
@@ -67,17 +66,80 @@ class PagesTest {
  setContent(state)
  composeRule.onNodeWithText(text = "Page0", substring = true).assertExists()
 
- state.setValueOnIdle(state.value.copy(selectedIndex = 1))
+ state.updateOnIdle { it.copy(selectedIndex = 1) }
 
  composeRule.onNodeWithText(text = "Page1", substring = true).assertExists()
  }
 
- @OptIn(ExperimentalDecomposeApi::class, ExperimentalFoundationApi::class)
- private fun setContent(stack: State<ChildPages<Config, Config>>, ) {
+ @Test
+ fun GIVEN_pages_without_animation_WHEN_page_changed_from_0_to_2_THEN_onPageSelected_called_with_index_2() {
+ val state =
+ mutableStateOf(
+ ChildPages(
+ items = listOf(
+ Child.Created(Config.Config1, Config.Config1),
+ Child.Created(Config.Config2, Config.Config2),
+ Child.Created(Config.Config3, Config.Config3),
+ ),
+ selectedIndex = 0,
+ ),
+ )
+
+ val indices = ArrayList<Int>()
+
+ setContent(
+ pages = state,
+ onPageSelected = { indices += it },
+ )
+
+ indices.clear()
+
+ state.updateOnIdle { it.copy(selectedIndex = 2) }
+
+ assertContentEquals(listOf(2), indices)
+ }
+
+
+ @Test
+ fun GIVEN_pages_with_animation_WHEN_page_changed_from_0_to_2_THEN_onPageSelected_called_with_index_2() {
+ val state =
+ mutableStateOf(
+ ChildPages(
+ items = listOf(
+ Child.Created(Config.Config1, Config.Config1),
+ Child.Created(Config.Config2, Config.Config2),
+ Child.Created(Config.Config3, Config.Config3),
+ ),
+ selectedIndex = 0,
+ ),
+ )
+
+ val indices = ArrayList<Int>()
+
+ setContent(
+ pages = state,
+ onPageSelected = { indices += it },
+ scrollAnimation = PagesScrollAnimation.Default,
+ )
+
+ indices.clear()
+
+ state.updateOnIdle { it.copy(selectedIndex = 2) }
+
+ assertContentEquals(listOf(2), indices)
+ }
+
+
+ private fun setContent(
+ pages: State<ChildPages<Config, Config>>,
+ onPageSelected: (index: Int) -> Unit = {},
+ scrollAnimation: PagesScrollAnimation = PagesScrollAnimation.Disabled,
+ ) {
  composeRule.setContent {
  Pages(
- pages = stack,
- onPageSelected = {},
+ pages = pages,
+ onPageSelected = onPageSelected,
+ scrollAnimation = scrollAnimation,
  ) { index, page ->
  Page(index, page)
  }
@@ -91,8 +153,8 @@ class PagesTest {
  BasicText(text = "Page$index=$page")
  }
 
- private fun <T> MutableState<T>.setValueOnIdle(value: T) {
- composeRule.runOnIdle { this.value = value }
+ private fun <T> MutableState<T>.updateOnIdle(func: (T) -> T) {
+ composeRule.runOnIdle { value = func(value) }
  composeRule.runOnIdle {}
  }
 

diff --git a/...e-jetpack/src/main/java/com/arkivanov/decompose/extensions/compose/jetpack/pages/Pages.kt b/...e-jetpack/src/main/java/com/arkivanov/decompose/extensions/compose/jetpack/pages/Pages.kt
@@ -11,6 +11,7 @@ import androidx.compose.runtime.DisposableEffect
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.State
 import androidx.compose.runtime.getValue
+import androidx.compose.runtime.remember
 import androidx.compose.ui.Modifier
 import com.arkivanov.decompose.ExperimentalDecomposeApi
 import com.arkivanov.decompose.InternalDecomposeApi
@@ -78,7 +79,10 @@ fun <T : Any> Pages(
  }
 
  DisposableEffect(state.currentPage) {
- onPageSelected(state.currentPage)
+ if (state.currentPage == state.targetPage) {
+ onPageSelected(state.currentPage)
+ }
+
  onDispose {}
  }
 
@@ -87,7 +91,9 @@ fun <T : Any> Pages(
  state,
  { childPages.items[it].configuration.hashString() },
  ) { pageIndex ->
- childPages.items[pageIndex].instance?.also { page ->
+ val item = childPages.items[pageIndex]
+ val page = remember(item.configuration) { item.instance }
+ if (page != null) {
  pageContent(pageIndex, page)
  }
  }