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 rememberRetainedSaveable #1305

Merged
merged 13 commits into from
May 27, 2024

Conversation

alexvanyo
Copy link
Contributor

Adds a rememberRetainedSaveable variant that participates in both RetainedStateRegistry and SaveableStateRegistry restoration.

The logic is the following upon rememberRetainedSaveable entering composition:

  • consume from both RetainedStateRegistry and SaveableStateRegistry, if available
  • if the retained value is available, use that
  • otherwise, if the saveable restored value is available, use that
  • otherwise, re-initialize the value

The RememberObserver behavior is duplicated from the rememberRetained behavior.

@ZacSweers
Copy link
Collaborator

Can we do this? #722 (comment)

@alexvanyo alexvanyo force-pushed the av/remember-retained-saveable branch 2 times, most recently from 85249fb to 8f784f0 Compare March 28, 2024 02:07
@alexvanyo
Copy link
Contributor Author

alexvanyo commented Mar 28, 2024

Pushed up two commits for comparison:

7872adb replaces the existing rememberRetained implementation with rememberRetainedSaveable, but keeps the rememberRetainedSaveable function names.

8f784f0 effectively adds a saver parameter to rememberRetained. Unlike rememberSaveable, which uses an autoSaver() by default, the default value for rememberRetained is a Saver({ null }, { null }), which is effectively a "never save" Saver.

@alexvanyo alexvanyo force-pushed the av/remember-retained-saveable branch from 8f784f0 to 15466a3 Compare April 23, 2024 16:01
@stagg
Copy link
Collaborator

stagg commented May 7, 2024

Think the rememberRetained that takes the saver is a good place to start. If we can put the new one under an experimental api to iron it out without impacting the existing rememberRetained that'd be great.

Not a huge fan of rememberRetainedSaveable as a name, bit of a mouthful. Randomly sort of like keep as a name for all this is doing now?

@ZacSweers
Copy link
Collaborator

ZacSweers commented May 7, 2024

I like keep, but feel like remember is an important convention to use too. Not sure what other options there are, rememberKept doesn't have the same ring

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.

Had some stale comments that I apparently never sent!

* It behaves similarly to [remember], but the stored value will survive configuration changes, such
* as a screen rotation.
*
* Use this overload if you remember a mutable state.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate a bit on this? I think I'm learning something new here heh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated - this matches the overloads of rememberSaveable, where it can be convenient to just write the Saver for the thing inside of mutableStateOf, instead of the whole MutableState

Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL!

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.

One nit around a param name, otherwise I think with the nullable Saver change this can be good to go!

@ZacSweers ZacSweers marked this pull request as ready for review May 27, 2024 20:08
@ZacSweers ZacSweers requested a review from stagg May 27, 2024 20:08
@ZacSweers ZacSweers added this pull request to the merge queue May 27, 2024
Merged via the queue into slackhq:main with commit b314c71 May 27, 2024
5 checks passed
@alexvanyo alexvanyo deleted the av/remember-retained-saveable branch May 28, 2024 00:37
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.

None yet

3 participants