Fix new presenter instances of the same type not being recomposed #799
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is an interesting edge case I ran into and subsequently learned about after talking with other folks. In short, because compose treats these by type and source position, you can think of the following two snippets as equivalent.
```kotlin
class Foo(val value: String) {
@composable
fun Content() {
val myState = remember { init(value) }
}
}
```
```kotlin
@composable
fun Content(self: Foo) {
val myState = remember { init(self.value) }
}
```
This affects Circuit presenters disproportionately because their present() calls have no other inputs to disambiguate. As a result, compose skips this in cases where you have a possibly different screen but same presenter _type_, and we need to explicitly force recomposition with a `key()`.
A good example of this issue can be seen like so
```kotlin
var count by remember { mutableIntStateOf(0) }
val screen = CountScreen(id)
Column {
Button(onClick = { id++ }) {
Text("Increment count")
}
CircuitContent(screen)
}
```
Where `ExampleScreen` just displays the current count. Before this change, this would never recompose the state.
|
Depends on #800 |
ZacSweers
commented
Aug 14, 2023
|
|
||
| // Regression test for https://github.com/slackhq/circuit/pull/799 | ||
| @Test | ||
| fun contentShouldRecomposeStateWhenPresenterInstanceChanges() { |
There was a problem hiding this comment.
Confirmed this fails with the previous behavior and passes now 🎉
kierse
approved these changes
Aug 14, 2023
|
|
||
| // Regression test for https://github.com/slackhq/circuit/pull/799 | ||
| @Test | ||
| fun contentShouldRecomposeStateWhenPresenterInstanceChanges() { |
The surprising thing is that it's not exactly zero argument, the enclosing class becomes an implicit receiver parameter and in theory that has changed, but compose doesn't consider it that way unless the type is different. |
chrisbanes
added a commit
to chrisbanes/circuit
that referenced
this pull request
Sep 19, 2023
The current implementation uses the presenter itself as the key(), which does fix the original issue. For our usage though, the presenter will always be a new instance, and therefore there will never be a consistent key across back stack changes (i.e. a push, then pop). This then forces the currentCompositeKeyHash for the underlying content underneath to change, which then breaks rememberRetained() when used without an explicit key. Fixed by changing the key to the `screen` instance instead, which achieves the same original fix, but the screen is consistent across navigation changes, which allows the currentCompositeKeyHash to be consistent.
chrisbanes
added a commit
to chrisbanes/circuit
that referenced
this pull request
Sep 19, 2023
The current implementation uses the presenter itself as the key(), which does fix the original issue. For our usage though, the presenter will always be a new instance, and therefore there will never be a consistent key across back stack changes (i.e. a push, then pop). This then forces the currentCompositeKeyHash for the underlying content underneath to change, which then breaks rememberRetained() when used without an explicit key. Fixed by changing the key to the `screen` instance instead, which achieves the same original fix, but the screen is consistent across navigation changes, which allows the currentCompositeKeyHash to be consistent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an interesting edge case I ran into and subsequently learned about after talking with other folks. In short, because compose treats these by type and source position, you can think of the following two snippets as equivalent.
This affects Circuit presenters disproportionately because their present() calls have no other inputs to disambiguate. As a result, compose skips this in cases where you have a possibly different screen but same presenter type, and we need to explicitly force recomposition with a
key().A good example of this issue can be seen like so
Where
ExampleScreenjust displays the current count. Before this change, this would never recompose the state.