rememberRetained works inconsistently in Ui and Presenter

[vulpeszerda]

Initial problem

rememberRetained or other implementations built with it (like ImpresseionEffect) works differently in Ui and Presenter.

When using CircuitContent (and NavigableCircuitContent also because it is built with CircuitContent), if screen changes from A1 to A2 (A1 and A2 is same screen class but different argument), rememberRetained in presenter is being reset but it is retained in ui.

I found out it is because of presenter.present() is wrapped with key(screen) but not for ui (I read https://github.com/slackhq/circuit/pull/799)

@Composable
public fun <UiState : CircuitUiState> CircuitContent(
  screen: Screen,
  modifier: Modifier,
  presenter: Presenter<UiState>,
  ui: Ui<UiState>,
  eventListener: EventListener = EventListener.NONE,
) {
  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() }

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

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

What is right feature after all

I have thought about what is right feature after all?.

Should screen be a key for retain? (1) or record uuid be a key for retain (2)?

For 1.

If screen is the key for retain, then screen A1, A2 (different screen argument for same screen class) is treated like B, C (different screen class).

If so, when I want to change page content from outside of CircuitContent but want to retain previous states, how to accomplish this?

The example case for this is below.

1. Page have to load some data for initialize.
2. Page shows child content A from initialized data
3. When user click notification, then page should show content B from initialized data

The data for initialize is not needed to load every user's notification click.
Therefore, initialize data should be retained.

We can do this with wrapping CircuitContent with CompositionLocalProvider and provide some changing value. And then presenter or ui can consume provided value (showing content type in above example)

But this implementation could be more and more difficult for NavigableCircuitContent. I know there is providedValues for providing CompositionLocalProvider per record, but it is still too difficult 😢 (I searched some example use cases, but not found)

And if I should use CompositionLocalProvider, then why does screen's arguments stand for?

For 2.

Making record uuid be a key for retain means that CircuitContent will not reset retained values when screen changes from A1 to A2. In NavigableCircuitContent, if record`s uuid is not changed, then state will be retained.

This is the similar treats screen as intent in android. If the screen argument is changed, presenter or ui will act like receiving onNewIntent in activity.

But this will change basic architecture of presenter, and rememberPresenter implementations.

For solving https://github.com/slackhq/circuit/pull/799 while retain values in presenter, we can use fake staticCompositionLocals with screen. (It is a bit hacky)

Conclusion

Above 1 and 2 decision depends on circuit's architecture philosophy. I want to know the thoughts of the main contributors.

And depends on the decision, I want the retain strategy to be changed as consitent among presenter and ui.

[stagg]

Interesting, I wonder if we can clarify the uses here more.

1. NavigableCircuitContent

I'd expect both ui and presenter to retain based on the record, as you can't change the screen without a new record instance.

2. CircuitContent

I think there's two uses. a) Each Screen instance is a new "page" and would behave the same as NavigableCircuitContent. Where both the ui and presenter retain based on that 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.

The 2. b) case is potentially common with a "widget" sub-circuit case. In this case would we expect the ui and presenter to retain across different instances of the same Screen type?

@ZacSweers, @kierse thoughts?

Test branch for this

[vulpeszerda]

Any update..? 👀

[ZacSweers]

Sorry haven't had a chance to circle to this

[chrisbanes]

I agree with @stagg's comments above.

AFAICT, the discussion around CircuitContent basically boils down to structural/referential equality. #799 basically forces to use referential equality, so I'd propose that that is documented. The 2.a case can easily be handled by callers by doing a remember(screen.id) { screen }

[vulpeszerda]

Firstly, UI and Presenter's retain strategy should to consistent. This will be indisputable.

The aspect open to debate would be as follows:

As previously noted, considering A1 and A2 (which belong to the same class but have different arguments) as entirely distinct pages complicates the transfer of parent parameters to child circuit content.

For instance, if screen A contains a nested NavigableCircuit within its UI, transitioning from A1 to A2 would result in the loss of the entire backstack.

In essence, I am advocating for the support of @stagg's 2.b scenario.

[chrisbanes]

Firstly, UI and Presenter's retain strategy should to consistent. This will be indisputable.

I'm not sure that is indisputable. Retained state was created for presenters to be able to retain state, not the UI. If you're retaining things in the UI, then I see that as a 'code smell', and something which should probably be moved up to the presenter. What's the use case you have for this?

Retained state in a raw CircuitContent is also not going to work so well. Depending on where it is called, it's state could be retained in the root registry, meaning that it is retained for the entire lifetime of the root content (aka the continuity registry). IMO, if you're using CircuitContent, the scoping of the retained state (i.e. which registry is used) is completely up to the caller. Circuit should not be opinionated here, as it's already opinionated for NavigableCircuitContent.

[ZacSweers]

I'm leaning 2b here as well for CircuitContent, and just making sure the NavigableCircuitContent continues to work correctly. I think @stagg's branch covers both of these cases nicely, I'd say let's go with that 👍

[vulpeszerda]

Firstly, UI and Presenter's retain strategy should to consistent. This will be indisputable.

I'm not sure that is indisputable. Retained state was created for presenters to be able to retain state, not the UI. If you're retaining things in the UI, then I see that as a 'code smell', and something which should probably be moved up to the presenter. What's the use case you have for this?

In my case, I was using ImpressionEffect in the UI to trigger a toast only once per page load. However, due to ImpressionEffect being built with rememberRetained, the operation of ImpressionEffect was inconsistent between the presenter and the UI as noted above.

@chrisbanes, do you consider using ImpressionEffect within the UI also a 'code smell'?

I'm leaning 2b here as well for CircuitContent, and just making sure the NavigableCircuitContent continues to work correctly. I think @stagg's branch covers both of these cases nicely, I'd say let's go with that 👍

@ZacSweers great! 👍