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

Enable RememberObserver to work with rememberRetained #1210

Merged
merged 5 commits into from
Feb 17, 2024

Conversation

chrisbanes
Copy link
Contributor

@chrisbanes chrisbanes commented Feb 15, 2024

This PR manually calls RememberObserver calls as appropriate when used in rememberRetained. The semantics of rememberRetained are obviously different to remember, so we call them at different times:

  • onRemembered will be called the first time that the retained item is first remembered. The difference here is that we will not call onRemembered again once a the object is restored and remembered back into composition.
  • onForgotten will be called only once the retained state is cleared from both composition and any retained registries.
  • Thus, the logical lifecycle is maintained of created (onRemembered called), through to being no longer retained/remembered (onForgotten called).
  • We do not call onAbandoned as retained state doesn't meet the contract of the function.

I did think about creating a RetainedObserver interface, but figured that using the existing RememberObserver, even with slightly different semantics, is a better solution.

This has a variety of use-cases, but I have been playing around with a rememberRetainedCorotineScope in Tivi: chrisbanes/tivi#1763

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

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

Nice find

@chrisbanes chrisbanes marked this pull request as ready for review February 17, 2024 14:56
@chrisbanes chrisbanes changed the title [WIP] Enable RememberObserver to work with rememberRetained Enable RememberObserver to work with rememberRetained Feb 17, 2024
@chrisbanes
Copy link
Contributor Author

Ready for review @ZacSweers!

@ZacSweers ZacSweers added this pull request to the merge queue Feb 17, 2024
Merged via the queue into slackhq:main with commit 4a29084 Feb 17, 2024
5 checks passed
@ZacSweers ZacSweers deleted the cb/retained-rememberobserver branch February 17, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants