Skip to content

Enable circuit-retained without composition local manual setup #1670

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

Closed
takahirom opened this issue Sep 20, 2024 · 18 comments
Closed

Enable circuit-retained without composition local manual setup #1670

takahirom opened this issue Sep 20, 2024 · 18 comments
Labels
discussion Open discussions! enhancement New feature or request
Milestone

Comments

@takahirom
Copy link

takahirom commented Sep 20, 2024

Thank you for developing this fantastic library. I’d like to use circuit-retained with Jetpack Navigation (or JetBrains Compose Multiplatform Navigation) without needing to set up a composition local for rememberRetained{}. Is this possible, or could you provide guidance on how to achieve this?

@takahirom takahirom changed the title Sample or Integration Implementation using circuit-retained with Jetpack Navigation (or JetBrains Compose Multiplatform Navigation) Enable circuit-retained without composition local setup Sep 20, 2024
@takahirom takahirom changed the title Enable circuit-retained without composition local setup Enable circuit-retained without composition local manual setup Sep 20, 2024
@stagg
Copy link
Collaborator

stagg commented Sep 28, 2024

I think this would be possible. Would need to pull something together to setup a multi-platform continuity instance as a default. Wouldn't necessarily want to do that setup at each retained use though 🤔

Think at a minimum we should add a RetainedCompositionLocals. Like the CircuitCompositionLocals we have in foundation that sets up the continuity registry for circuit. Would make it easier to use retained on it's own.

Curious whats the limitation around setting up a composition local, or adding a RetainedCompositionLocals, for this at the root level?

@takahirom
Copy link
Author

takahirom commented Sep 28, 2024

I think managing the backstack might be challenging if only one composition local is provided. It might be beneficial if we could use ViewModels and Lifecycles from Compose Multiplatform instead of relying on the current composition local to achieve smooth integration, assuming they do not already exist. However, this approach may be very complex.

@stagg
Copy link
Collaborator

stagg commented Oct 3, 2024

Ahh so specifically an integration with Jetpack/Compose Navigation where the view model store is setup per route. We're doing something similar in NavigableCircuitContent for the LocalRetainedStateRegistry.

Could be doable, let folks provide a default instead of the NoOpRetainedStateRegistry 🤔 Involves some sort of a factory which would default to using a multi-platform continuity.
This would be assuming a lot about the lifecycle, which I think ties this down to being a very Jetpack Navigation specific change.

@ZacSweers ZacSweers added enhancement New feature or request discussion Open discussions! labels Oct 23, 2024
@ZacSweers ZacSweers added this to the 1.0.0 milestone Nov 14, 2024
@ZacSweers
Copy link
Collaborator

This now works entirely via local RetainedStateRegistry and not a local CanRetainChecker. I'm not entirely sure how this could be implemented without a local and without requiring a registry. Where would it store anything?

@takahirom
Copy link
Author

I think you can use JetBrains Compose Multiplatform Navigation's ViewModel instead of RetainedStateRegistry. The title "without composition local manual setup" might not be appropriate because my intention is to use circuit-retained with Jetpack Navigation. However, I'm uncertain whether this approach aligns with Circuit's architectural principles.

@ZacSweers
Copy link
Collaborator

Can you elaborate more on how that would look? Circuit-retained already uses a backing viewmodel in android

@takahirom
Copy link
Author

Well, as you know, this is my library, and it just uses androidx.lifecycle.viewmodel.compose.viewModel.
https://github.com/takahirom/Rin/blob/main/rin/src/commonMain/kotlin/io/github/takahirom/rin/Rin.kt

@ZacSweers
Copy link
Collaborator

Yes but I don't see androidx navigation used in that file? It appears to have its own backing viewmodel and a new one is instantiated for each rememberRetained?

@takahirom
Copy link
Author

@ZacSweers
Copy link
Collaborator

The title is requesting rememberRetained support without use of a composition local but that seems to require a composition local? Or rather, it seems like the request is to just use a different composition local (LocalViewModelStoreOwner) so that the pattern I mentioned here can be used? I'm struggling to follow how this is simpler 😅, it seems to be much more overhead.

@takahirom
Copy link
Author

To be clear, I think we needed to be done with different CompositionLocal implementations.

What I wanted was an easy setup for most Compose users.
Perhaps "Enable circuit-retained without manual CompositionLocal setup for Compose Navigation users" would be a more appropriate title for this. For Compose Navigation users, this would eliminate the need to set up the CompositionLocal manually because it would be automatically handled by the navigation library.

That said, I also believe it might be challenging to implement this in circuit-retained. However, I thought that with this support, I could use circuit-retained in many projects.

@ZacSweers
Copy link
Collaborator

Could you elaborate on why you feel it's difficult to set up a local RetainedStateRegistry? Seems to me like you could just create one that delegates to an underlying LocalViewModelStoreOwner for its implementation. I do think that creating a whole new ViewModel instance for each rememberRetained call is wasteful though.

@takahirom
Copy link
Author

It's preferable to have the ability to manually provide the RetainedStateRegistry to the ViewModel, rather than not having this option at all. I'm uncertain whether we can implement rememberRetained{} using RetainedStateRegistry and LocalViewModelStoreOwner. If a sample implementation were available, it would be greatly appreciated. I was trying to implement it, but I forgot the details. Sorry about that.

Regarding the creation of a new ViewModel instance for each rememberRetained call, I don't believe it's necessary if we have a data structure like mutableMapOf<String, ArrayDeque<RinViewModelEntity<Any?>>>() within a ViewModel that maps the currentCompositeKeyHash to values. However, I'm not entirely sure about the Circuit implementation.

@ZacSweers
Copy link
Collaborator

ZacSweers commented Mar 6, 2025

It's preferable to have the ability to manually provide the RetainedStateRegistry to the ViewModel, rather than not having this option at all.

There is an overload of continuityRetainedStateRegistry() that accepts a custom ViewModelProvider.Factory, which should allow customization of the backing viewmodel. Ref. Does that satisfy this need?

I don't believe it's necessary if we have a data structure like mutableMapOf<String, ArrayDeque<RinViewModelEntity<Any?>>>() within a ViewModel that maps the currentCompositeKeyHash to values. However, I'm not entirely sure about the Circuit implementation.

This is functionally what the circuit implementation does :). The viewmodel and the impl it delegates to.

@ZacSweers
Copy link
Collaborator

@takahirom given that, do you think I can close this?

@takahirom
Copy link
Author

Since the original issue title references "Sample or Integration Implementation using circuit-retained with Jetpack Navigation (or JetBrains Compose Multiplatform Navigation)", I'd be interested in seeing a sample implementation or test case demonstrating rememberRetained with Navigation components. However, I'm currently uncertain whether this integration is feasible.

If you prefer to close this issue, that's acceptable - this request primarily reflects my personal interest rather than an urgent technical requirement. We can always create a new issue if specific implementation challenges emerge later. Please feel free to proceed with closing this.

@8cAyqpVKio
Copy link

SavedState (called Bundle in Android) now supports kotlinx.serialization in the alpha version, so making it easier to use rememberSaveable.

val state = rememberSaveable(
    saver = Saver(
        save = { encodeToSavedState(it) },
        restore = { decodeFromSavedState(it) }
    )
) { UiState(emptyList(), emptyList()) }

@Serializable
data class UiState(
    val cats: List<Cat>,
    val dogs: List<Dog>
)

I think rememberRetained will no longer be necessary in the future.

@ZacSweers
Copy link
Collaborator

I think that claim reveals a lack of understanding in what circuit-retained is for :). Marshaling and unmarshaling every time isn't free, nor is it possible or practical to make everything that goes into circuit-retained today serializable. For example - coroutine scopes.

I am going to close this for now as it's no longer an issue/FR and really just a question. Please feel free to post in the Q&A section of the discussions panel of the repo if you want to pursue this. My conclusion from the above conversation is that circuit-retained supports everything you need, that fork seems to just interop to LocalViewModelStoreOwner and allocate a ViewModel instance per every rememberRetained rather than use a single instance with an inner map. How that relates to androidx.navigation is really up to the developer, I am unfamiliar with it nor is it something we use, so I'm not sure we're well-equipped to advise. If you ask in the discussions section though, maybe someone else from the community can help. I do think there would be a lot of value in learning how circuit-retained works under the hood, the blog post I linked above is helpful for that too.

@ZacSweers ZacSweers closed this as not planned Won't fix, can't repro, duplicate, stale Mar 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussions! enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants