Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a key to CircuitContent to keep UI and Presenter consistent #1254

Merged
merged 3 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@ public fun CircuitContent(
circuit: Circuit = requireNotNull(LocalCircuit.current),
unavailableContent: (@Composable (screen: Screen, modifier: Modifier) -> Unit) =
circuit.onUnavailableContent,
key: Any? = screen,
) {
CircuitContent(screen, Navigator.NoOp, modifier, circuit, unavailableContent)
CircuitContent(screen, Navigator.NoOp, modifier, circuit, unavailableContent, key)
}

@Composable
Expand All @@ -39,6 +40,7 @@ public fun CircuitContent(
circuit: Circuit = requireNotNull(LocalCircuit.current),
unavailableContent: (@Composable (screen: Screen, modifier: Modifier) -> Unit) =
circuit.onUnavailableContent,
key: Any? = screen,
) {
val navigator =
remember(onNavEvent) {
Expand Down Expand Up @@ -66,7 +68,7 @@ public fun CircuitContent(
override fun peekBackStack(): ImmutableList<Screen> = persistentListOf(screen)
}
}
CircuitContent(screen, navigator, modifier, circuit, unavailableContent)
CircuitContent(screen, navigator, modifier, circuit, unavailableContent, key)
}

@Composable
Expand All @@ -77,6 +79,7 @@ public fun CircuitContent(
circuit: Circuit = requireNotNull(LocalCircuit.current),
unavailableContent: (@Composable (screen: Screen, modifier: Modifier) -> Unit) =
circuit.onUnavailableContent,
key: Any? = screen,
) {
val parent = LocalCircuitContext.current
@OptIn(InternalCircuitApi::class)
Expand All @@ -85,7 +88,7 @@ public fun CircuitContent(
CircuitContext(parent).also { it.circuit = circuit }
}
CompositionLocalProvider(LocalCircuitContext provides context) {
CircuitContent(screen, modifier, navigator, circuit, unavailableContent, context)
CircuitContent(screen, modifier, navigator, circuit, unavailableContent, context, key)
}
}

Expand All @@ -97,6 +100,7 @@ internal fun CircuitContent(
circuit: Circuit,
unavailableContent: (@Composable (screen: Screen, modifier: Modifier) -> Unit),
context: CircuitContext,
key: Any? = screen,
) {
val eventListener = rememberEventListener(screen, context, factory = circuit.eventListenerFactory)
DisposableEffect(eventListener, screen, context) { onDispose { eventListener.dispose() } }
Expand All @@ -106,7 +110,7 @@ internal fun CircuitContent(
val ui = rememberUi(screen, context, eventListener, circuit::ui)

if (ui != null && presenter != null) {
(CircuitContent(screen, modifier, presenter, ui, eventListener))
(CircuitContent(screen, modifier, presenter, ui, eventListener, key))
} else {
eventListener.onUnavailableContent(screen, presenter, ui, context)
unavailableContent(screen, modifier)
Expand All @@ -120,29 +124,31 @@ public fun <UiState : CircuitUiState> CircuitContent(
presenter: Presenter<UiState>,
ui: Ui<UiState>,
eventListener: EventListener = EventListener.NONE,
) {
DisposableEffect(screen) {
eventListener.onStartPresent()

onDispose { eventListener.onDisposePresent() }
}
key: Any? = screen,
): Unit =
// While the screen is different, in the eyes of compose its position is _the same_, meaning
// we need to wrap the ui and presenter in a key() to force recomposition if it changes. A good
// example case of this is when you have code that calls CircuitContent with a common screen with
// different inputs (but thus same presenter instance type) and you need this to recompose with
// a different presenter.
key(key) {
DisposableEffect(screen) {
eventListener.onStartPresent()

onDispose { eventListener.onDisposePresent() }
}

// While the presenter is different, in the eyes of compose its position is _the same_, meaning
// we need to wrap the presenter in a key() to force recomposition if it changes. A good example
// case of this is when you have code that calls CircuitContent with a common screen with
// different inputs (but thus same presenter instance type) and you need this to recompose with a
// different presenter.
val state = key(screen) { presenter.present() }
val state = presenter.present()

// TODO not sure why stateFlow + LaunchedEffect + distinctUntilChanged doesn't work here
SideEffect { eventListener.onState(state) }
DisposableEffect(screen) {
eventListener.onStartContent()
// TODO not sure why stateFlow + LaunchedEffect + distinctUntilChanged doesn't work here
SideEffect { eventListener.onState(state) }
DisposableEffect(screen) {
eventListener.onStartContent()

onDispose { eventListener.onDisposeContent() }
onDispose { eventListener.onDisposeContent() }
}
ui.Content(state, modifier)
}
ui.Content(state, modifier)
}

/**
* Remembers a new [EventListener] instance for the given [screen] and [context].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ private fun BackStack<out Record>.buildCircuitContentProviders(
navigator = lastNavigator,
circuit = lastCircuit,
unavailableContent = lastUnavailableRoute,
key = record.key,
)
},
)
Expand Down
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:chefskiss:

Original file line number Diff line number Diff line change
@@ -0,0 +1,243 @@
// Copyright (C) 2024 Slack Technologies, LLC
// SPDX-License-Identifier: Apache-2.0
package com.slack.circuit.foundation

import androidx.compose.foundation.layout.Column
import androidx.compose.material.Text
import androidx.compose.runtime.Composable
import androidx.compose.runtime.MutableState
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.setValue
import androidx.compose.ui.Modifier
import androidx.compose.ui.platform.testTag
import androidx.compose.ui.test.assertTextEquals
import androidx.compose.ui.test.junit4.ComposeContentTestRule
import androidx.compose.ui.test.junit4.createComposeRule
import androidx.compose.ui.test.onNodeWithTag
import com.slack.circuit.backstack.rememberSaveableBackStack
import com.slack.circuit.retained.rememberRetained
import com.slack.circuit.runtime.CircuitUiState
import com.slack.circuit.runtime.Navigator
import com.slack.circuit.runtime.presenter.Presenter
import com.slack.circuit.runtime.screen.Screen
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith

private const val TAG_UI_RETAINED = "TAG_UI_RETAINED"
private const val TAG_PRESENTER_RETAINED = "TAG_PRESENTER_RETAINED"
private const val TAG_STATE = "TAG_STATE"

/**
* This is testing the following use cases for using [CircuitContent] based on
* https://github.com/slackhq/circuit/issues/1169.
*
* Uses:
* 1. [NavigableCircuitContent]
* - The ui and presenter are retain based on the record, as the [Screen] can't change without a new
* record instance.
* 2. [CircuitContent]
* - a) Each [Screen] instance is a new "page" and would behave the same as
* [NavigableCircuitContent], by being keyed on the [Screen] instance.
* - b) The [Screen] is a model, and it's used to update the current [CircuitContent] state, as if
* it were another Compose element. This is potentially common with a "widget" sub-circuit case.
*/
@RunWith(ComposeUiTestRunner::class)
class KeyedCircuitContentTests {

@get:Rule val composeTestRule = createComposeRule()

private val circuit =
Circuit.Builder()
.addPresenter<ScreenA, ScreenA.State> { screen, _, _ -> ScreenAPresenter(screen) }
.addUi<ScreenA, ScreenA.State> { state, modifier -> ScreenAUi(state, modifier) }
.addPresenter<ScreenB, ScreenB.State> { screen, _, _ -> ScreenBPresenter(screen) }
.addUi<ScreenB, ScreenB.State> { state, modifier -> ScreenBUi(state, modifier) }
.build()

@Test
fun one() {
composeTestRule.run {
val navigator = setUpTestOneContent()
// Initial
onNodeWithTag(TAG_STATE).assertTextEquals("1")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("1")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("1")
// Push a new ScreenA
navigator.goTo(ScreenA(2))
onNodeWithTag(TAG_STATE).assertTextEquals("4")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("4")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("4")
// Push a new ScreenA
navigator.goTo(ScreenA(3))
onNodeWithTag(TAG_STATE).assertTextEquals("9")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("9")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("9")
// Push a new ScreenB
navigator.goTo(ScreenB("abc"))
onNodeWithTag(TAG_STATE).assertTextEquals("cba")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("cba")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("cba")
// Back one
navigator.pop()
onNodeWithTag(TAG_STATE).assertTextEquals("9")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("9")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("9")
// Back two
navigator.pop()
onNodeWithTag(TAG_STATE).assertTextEquals("4")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("4")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("4")
}
}

@Test
fun twoA() {
composeTestRule.run {
var screenState by setUpTestTwoAContent()
// Initial
onNodeWithTag(TAG_STATE).assertTextEquals("1")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("1")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("1")
// Set a new ScreenA
screenState = ScreenA(2)
onNodeWithTag(TAG_STATE).assertTextEquals("4")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("4")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("4")
// Set a new ScreenA
screenState = ScreenA(3)
onNodeWithTag(TAG_STATE).assertTextEquals("9")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("9")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("9")
// Set a new ScreenB
screenState = ScreenB("abc")
onNodeWithTag(TAG_STATE).assertTextEquals("cba")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("cba")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("cba")
// Back to a ScreenA
screenState = ScreenA(3)
onNodeWithTag(TAG_STATE).assertTextEquals("9")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("9")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("9")
// Back to another ScreenA
screenState = ScreenA(2)
onNodeWithTag(TAG_STATE).assertTextEquals("4")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("4")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("4")
}
}

@Test
fun twoB() {
composeTestRule.run {
var screenState by setUpTestTwoBContent()
// Initial
onNodeWithTag(TAG_STATE).assertTextEquals("1")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("1")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("1")
// Set a new ScreenA
screenState = ScreenA(2)
onNodeWithTag(TAG_STATE).assertTextEquals("4")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("1")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("1")
// Set a new ScreenA
screenState = ScreenA(3)
onNodeWithTag(TAG_STATE).assertTextEquals("9")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("1")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("1")
// Set a new ScreenB
screenState = ScreenB("abc")
onNodeWithTag(TAG_STATE).assertTextEquals("cba")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("cba")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("cba")
// Back to a ScreenA
screenState = ScreenA(3)
onNodeWithTag(TAG_STATE).assertTextEquals("9")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("9")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("9")
// Back to another ScreenA
screenState = ScreenA(2)
onNodeWithTag(TAG_STATE).assertTextEquals("4")
onNodeWithTag(TAG_UI_RETAINED).assertTextEquals("9")
onNodeWithTag(TAG_PRESENTER_RETAINED).assertTextEquals("9")
}
}

private fun ComposeContentTestRule.setUpTestOneContent(screen: Screen = ScreenA(1)): Navigator {
lateinit var navigator: Navigator
setContent {
CircuitCompositionLocals(circuit) {
val backStack = rememberSaveableBackStack(screen)
navigator = rememberCircuitNavigator(backStack = backStack, onRootPop = {})
NavigableCircuitContent(navigator = navigator, backStack = backStack)
}
}
return navigator
}

private fun ComposeContentTestRule.setUpTestTwoAContent(
screen: Screen = ScreenA(1)
): MutableState<Screen> {
val screenState = mutableStateOf(screen)
setContent { CircuitCompositionLocals(circuit) { CircuitContent(screenState.value) } }
return screenState
}

private fun ComposeContentTestRule.setUpTestTwoBContent(
screen: Screen = ScreenA(1)
): MutableState<Screen> {
val screenState = mutableStateOf(screen)
setContent {
CircuitCompositionLocals(circuit) {
CircuitContent(screenState.value, key = screenState.value::class)
}
}
return screenState
}
}

private class ScreenA(val num: Int) : Screen {
class State(val numSquare: Int, val retainedNumSquare: Int) : CircuitUiState
}

private class ScreenAPresenter(val screen: ScreenA) : Presenter<ScreenA.State> {
@Composable
override fun present(): ScreenA.State {
val square = rememberRetained { screen.num * screen.num }
return ScreenA.State(screen.num * screen.num, square)
}
}

@Composable
private fun ScreenAUi(state: ScreenA.State, modifier: Modifier = Modifier) {
Column(modifier) {
val retained = rememberRetained { state.numSquare }
Text(text = "$retained", modifier = Modifier.testTag(TAG_UI_RETAINED))
Text(text = "${state.numSquare}", modifier = Modifier.testTag(TAG_STATE))
Text(text = "${state.retainedNumSquare}", modifier = Modifier.testTag(TAG_PRESENTER_RETAINED))
}
}

private class ScreenB(val text: String) : Screen {

class State(val textReverse: String, val retainedTextReverse: String) : CircuitUiState
}

private class ScreenBPresenter(val screen: ScreenB) : Presenter<ScreenB.State> {
@Composable
override fun present(): ScreenB.State {
val textReverse = rememberRetained { screen.text.reversed() }
return ScreenB.State(screen.text.reversed(), textReverse)
}
}

@Composable
private fun ScreenBUi(state: ScreenB.State, modifier: Modifier = Modifier) {
Column(modifier) {
val retained = rememberRetained { state.textReverse }
Text(text = retained, modifier = Modifier.testTag(TAG_UI_RETAINED))
Text(text = state.textReverse, modifier = Modifier.testTag(TAG_STATE))
Text(text = state.retainedTextReverse, modifier = Modifier.testTag(TAG_PRESENTER_RETAINED))
}
}
Loading