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

Change rememberRetain not to retain the value of removed node #1794

Merged
merged 40 commits into from
Feb 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
019c8cc
Added navigableCircuitContentRetainTest
vulpeszerda Nov 8, 2024
5b279d4
Added condition retain test
vulpeszerda Nov 13, 2024
4b0cc82
Changed rememberRetained to be cleared when node removed
vulpeszerda Nov 12, 2024
d769169
Changed AndroidContinuity to save values on stopped
vulpeszerda Nov 12, 2024
465f98f
Fixed recreation test scenario
vulpeszerda Nov 12, 2024
24d8547
Replaced registry usage with RetainedStateProvider
vulpeszerda Nov 12, 2024
affec25
Fixed retainedTest to use RetainedStateProvider
vulpeszerda Nov 12, 2024
c42dd26
Merge branch 'main' into remember-retained-redesigned
vulpeszerda Nov 21, 2024
67d156f
Generate apiDumps
vulpeszerda Nov 21, 2024
ff654b4
Replaced RetainedStateProvider with RetainedStateHolder
vulpeszerda Nov 22, 2024
5d30dd7
Removed duplicated codes
vulpeszerda Nov 22, 2024
a367cbb
Changed to use record.registryKey
vulpeszerda Nov 22, 2024
0e4e8e3
Moved RetainedStateProvider to inner of movableContentOf
vulpeszerda Nov 22, 2024
9fbc2df
Restored un-intended changes
vulpeszerda Nov 22, 2024
fb672cf
Added removeState in RetainedStateHolder
vulpeszerda Nov 23, 2024
b6b0cec
Merge branch 'main' into remember-retained-redesigned
vulpeszerda Dec 17, 2024
16cf579
Changed RetainedStateHolder to use ReusableContent
vulpeszerda Jan 11, 2025
ff477ad
Changed RetainedStateHolderWithReturn to use key (instead of Reusable…
vulpeszerda Jan 12, 2025
7900ffb
Changed test method names for more readability
vulpeszerda Jan 12, 2025
df33d07
Added saveAll return value
vulpeszerda Jan 12, 2025
82e84a1
Changed RetainedStateRegistry not to save empty registry
vulpeszerda Jan 12, 2025
1e8083e
Add RetainedStateHolderTests (cherry-pick)
ZacSweers Feb 2, 2024
afebc7a
Added default RetainedStateRegistry
vulpeszerda Jan 12, 2025
9714ce3
Added RetainedStateHolder to star sample
vulpeszerda Jan 12, 2025
0c3c6b1
spotless
vulpeszerda Jan 12, 2025
ffcf6f6
apiDump
vulpeszerda Jan 12, 2025
91c2fcd
Fixed lint
vulpeszerda Jan 12, 2025
b726772
Moved rememberRetainedStateHolder declaration in NavigableCircuitContent
vulpeszerda Jan 15, 2025
28d601a
Removed redundant contentComposed = true
vulpeszerda Feb 4, 2025
b9d1afd
Changed NavigableCircuitContent comment
vulpeszerda Feb 4, 2025
ae1cd94
Merge branch 'main' into remember-retained-redesigned
vulpeszerda Feb 5, 2025
b267800
Handled CanRetainChecker stale case
vulpeszerda Feb 6, 2025
0eb7efb
Added CanRetainChecker switch test cases
vulpeszerda Feb 5, 2025
17b40a5
Added scoped CanRetainChecker tests
vulpeszerda Feb 6, 2025
6b6467e
Changed rememberRetained not to save its value when CanRetainChecker …
vulpeszerda Feb 6, 2025
b6bdea4
apiDumps
vulpeszerda Feb 6, 2025
2fe5950
Merge branch 'main' into remember-retained-redesigned
vulpeszerda Feb 6, 2025
a9407b1
Removed debug code
vulpeszerda Feb 6, 2025
58ba1cd
Minor code re-arrange
vulpeszerda Feb 6, 2025
32efffd
Added changelog
vulpeszerda Feb 6, 2025
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
34 changes: 34 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,40 @@ Changelog

**Unreleased**
--------------
### Behaviour Change: rememberRetained
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great writeup!

Previously, `rememberRetained` could sometimes restore values when a composable was re-added, depending on whether its parent `RetainedStateRegistry` had been saved (#1783).
Now, `rememberRetained` aligns with `remember` and `rememberSaveable`: if a composable is removed and later re-added, its value will not be restored unless it is explicitly saved and then restored via the registry.

### Behaviour Change: RetainedStateRegistry
- `saveAll` now returns the saved values.
- `RetainedStateRegistry.Entry.unregister` now returns whether the unsaved valueProvider was actually removed.
- `saveAll` and `saveValue` now skip storing child values when `CanRetainChecker` returns `false`.

### New: RetainedStateHolder
Similar to `SaveableStateHolder`, `RetainedStateHolder` provides a mechanism to maintain separate `RetainedStateRegistry` entries for specific keys. This allows saving the state defined with `rememberRetained` for a subtree before it is disposed, so that the subtree can later be recomposed with its state restored.

```kotlin
val retainedStateHolder = rememberRetainedStateHolder()
var currentTab by remember { mutableStateOf(TabA) }

retainedStateHolder.RetainedStateProvider(key = currentTab.name) {
// rememberRetained values in tab content are preserved across tab switches
when(currentTab) {
TabA -> {
TabAContent()
}
TabB -> {
TabBContent()
}
TabC -> {
TabCContent()
}
}
}
```

### Internal Change: NavigableCircuitContent
The approach of managing a separate `RetainedStateRegistry` for each record has been changed to use `RetainedStateHolder` instead.

0.25.0
------
Expand Down
2 changes: 2 additions & 0 deletions build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,8 @@ allprojects {
"**/Remove.kt",
"**/Pets.kt",
"**/SystemUiController.kt",
"**/RetainedStateHolderTest.kt",
"**/RetainedStateRestorationTester.kt",
)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import com.slack.circuit.retained.LocalCanRetainChecker
import com.slack.circuit.retained.LocalRetainedStateRegistry
import com.slack.circuit.retained.RetainedStateRegistry
import com.slack.circuit.retained.rememberRetained
import com.slack.circuit.retained.rememberRetainedStateHolder
import com.slack.circuit.runtime.InternalCircuitApi
import com.slack.circuit.runtime.Navigator
import com.slack.circuit.runtime.screen.Screen
Expand All @@ -67,14 +68,6 @@ public fun <R : Record> NavigableCircuitContent(
unavailableRoute: (@Composable (screen: Screen, modifier: Modifier) -> Unit) =
circuit.onUnavailableContent,
) {
val activeContentProviders =
buildCircuitContentProviders(
backStack = backStack,
navigator = navigator,
circuit = circuit,
unavailableRoute = unavailableRoute,
)

if (backStack.isEmpty) return

/*
Expand Down Expand Up @@ -113,6 +106,14 @@ public fun <R : Record> NavigableCircuitContent(
val outerRegistry = rememberRetained(key = outerKey) { RetainedStateRegistry() }

CompositionLocalProvider(LocalRetainedStateRegistry provides outerRegistry) {
val activeContentProviders =
buildCircuitContentProviders(
backStack = backStack,
navigator = navigator,
circuit = circuit,
unavailableRoute = unavailableRoute,
)

decoration.DecoratedContent(activeContentProviders, backStack.size, modifier) { provider ->
val record = provider.record

Expand Down Expand Up @@ -174,6 +175,7 @@ private fun <R : Record> buildCircuitContentProviders(
val lastUnavailableRoute by rememberUpdatedState(unavailableRoute)

val saveableStateHolder = rememberSaveableStateHolder()
val retainedStateHolder = rememberRetainedStateHolder()

fun createRecordContent() =
movableContentOf<R> { record ->
Expand All @@ -186,32 +188,27 @@ private fun <R : Record> buildCircuitContentProviders(
remember { MutableRecordLifecycle() }.apply { isActive = lastBackStack.topRecord == record }

CompositionLocalProvider(LocalCanRetainChecker provides recordInBackStackRetainChecker) {
// Now provide a new registry to the content for it to store any retained state in,
// along with a retain checker which is always true (as upstream registries will
// maintain the lifetime), and the other provided values
val recordRetainedStateRegistry =
rememberRetained(key = record.registryKey) { RetainedStateRegistry() }
saveableStateHolder.SaveableStateProvider(record.registryKey) {
CompositionLocalProvider(
LocalRetainedStateRegistry provides recordRetainedStateRegistry,
LocalCanRetainChecker provides CanRetainChecker.Always,
LocalRecordLifecycle provides lifecycle,
) {
CircuitContent(
screen = record.screen,
navigator = lastNavigator,
circuit = lastCircuit,
unavailableContent = lastUnavailableRoute,
key = record.key,
)
// Provides a RetainedStateRegistry that is maintained independently for each record while
// the record exists in the back stack.
retainedStateHolder.RetainedStateProvider(record.registryKey) {
CompositionLocalProvider(LocalRecordLifecycle provides lifecycle) {
CircuitContent(
screen = record.screen,
navigator = lastNavigator,
circuit = lastCircuit,
unavailableContent = lastUnavailableRoute,
key = record.key,
)
}
}
}
// Remove saved states for records that are no longer in the back stack
DisposableEffect(record.registryKey) {
onDispose {
if (!lastBackStack.containsRecord(record, includeSaved = true)) {
saveableStateHolder.removeState(record.registryKey)
}
}
// Remove saved states for records that are no longer in the back stack
DisposableEffect(record.registryKey) {
onDispose {
if (!lastBackStack.containsRecord(record, includeSaved = true)) {
saveableStateHolder.removeState(record.registryKey)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@ package com.slack.circuit.foundation
import androidx.compose.runtime.Composable
import androidx.compose.runtime.Stable
import androidx.compose.runtime.remember
import com.slack.circuit.foundation.internal.withCompositionLocalProvider
import com.slack.circuit.retained.LocalRetainedStateRegistry
import com.slack.circuit.retained.RetainedStateRegistry
import com.slack.circuit.retained.rememberRetained
import com.slack.circuit.runtime.CircuitUiState
import com.slack.circuit.runtime.presenter.Presenter

Expand Down Expand Up @@ -60,14 +56,13 @@ public fun <T> pausableState(
val state = remember(key) { MutableRef<T>(null) }

val saveableStateHolder = rememberSaveableStateHolderWithReturn()
val retainedStateHolder = rememberRetainedStateHolderWithReturn()

return if (isActive || state.value == null) {
val retainedStateRegistry = rememberRetained(key = key) { RetainedStateRegistry() }
withCompositionLocalProvider(LocalRetainedStateRegistry provides retainedStateRegistry) {
saveableStateHolder.SaveableStateProvider(
key = key ?: "pausable_state",
content = produceState,
)
val finalKey = key ?: "pausable_state"
saveableStateHolder
.SaveableStateProvider(finalKey) {
retainedStateHolder.RetainedStateProvider(key = finalKey, content = produceState)
}
.also {
// Store the last emitted state
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// Copyright (C) 2024 Slack Technologies, LLC
// SPDX-License-Identifier: Apache-2.0
package com.slack.circuit.foundation

import androidx.compose.runtime.Composable
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.key
import androidx.compose.runtime.remember
import com.slack.circuit.foundation.internal.withCompositionLocalProvider
import com.slack.circuit.retained.CanRetainChecker
import com.slack.circuit.retained.LocalCanRetainChecker
import com.slack.circuit.retained.LocalRetainedStateRegistry
import com.slack.circuit.retained.RetainedStateRegistry
import com.slack.circuit.retained.RetainedValueProvider
import com.slack.circuit.retained.rememberRetained

/** Copy of [RetainedStateHolder] to return content value */
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: do you think we can/should reconcile these into one API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be good to consolidate them, but there are a few considerations:

  1. Just like SaveableStateHolder in Add withCompositionLocalProvider to avoid backward writes #1451 , we need to investigate further whether converting a RetainedStateHolder without a return value into one with a return value would cause any issues.

  2. The RetainedStateHolder without a return value internally uses ReusableContent(key), while the one with a return value uses key(key). I’m not entirely sure if using ReusableContent(key) in the case of a return value would cause any issues.

internal interface RetainedStateHolder {

@Composable fun <T> RetainedStateProvider(key: String, content: @Composable () -> T): T

fun removeState(key: String)
}

/** Creates and remembers the instance of [RetainedStateHolder]. */
@Composable
internal fun rememberRetainedStateHolderWithReturn(): RetainedStateHolder {
return rememberRetained { RetainedStateHolderImpl() }
}

private class RetainedStateHolderImpl : RetainedStateHolder, RetainedStateRegistry {

private val registry = RetainedStateRegistry()

private val entries = mutableMapOf<String, Entry>()

@Composable
override fun <T> RetainedStateProvider(key: String, content: @Composable (() -> T)): T {
return withCompositionLocalProvider(LocalRetainedStateRegistry provides registry) {
key(key) {
val entry = remember { Entry() }
val childRegistry = rememberRetained(key = key) { RetainedStateRegistry() }
withCompositionLocalProvider(
LocalRetainedStateRegistry provides childRegistry,
LocalCanRetainChecker provides CanRetainChecker.Always,
content = content,
)
.also {
DisposableEffect(Unit) {
entries[key] = entry
onDispose {
if (entry.shouldSave) {
registry.saveValue(key)
}
entries -= key
}
}
}
}
}
}

override fun removeState(key: String) {
val entry = entries[key]
if (entry != null) {
entry.shouldSave = false
} else {
registry.consumeValue(key)
}
}

override fun consumeValue(key: String): Any? {
return registry.consumeValue(key)
}

override fun registerValue(
key: String,
valueProvider: RetainedValueProvider,
): RetainedStateRegistry.Entry {
return registry.registerValue(key, valueProvider)
}

override fun saveAll(): Map<String, List<Any?>> {
return registry.saveAll()
}

override fun saveValue(key: String) {
registry.saveValue(key)
}

override fun forgetUnclaimedValues() {
registry.forgetUnclaimedValues()
}

private data class Entry(var shouldSave: Boolean = true)
}
Loading