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

ViewModel POC #303

Closed

Conversation

KovalevAndrey
Copy link
Collaborator

@KovalevAndrey KovalevAndrey commented Dec 13, 2022

Description

This PR demonstrates how VM can be used and scoped to a Node using reflection API.

By design VM is stored in a ViewModelStore class and lives as long as activity lives.

The desired behaviour is the following:

  1. VM is created when Node is created
  2. VM is persistent when Node is moved in DESTROY state and Activity is changing configuration
  3. VM is destroyed when Node is moved in DESTROY state and Activity IS NOT changing configuration

This behaviour can not be achieved using public API as it's not possible to clear only one VM from ViewModelStore. It has only one public method – ViewModelStore.clear()which clears allVMs.

I the meantime I'm exploring alternative solutions.

vm.mp4

Check list

  • I have updated CHANGELOG.md if required.
  • I have updated documentation if required.

) : Node(
buildContext = buildContext
) {
private val activity = (integrationPoint as ActivityIntegrationPoint).activity
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can wrap all required code in a less uglier way exposing only VM factory

@CherryPerry
Copy link
Collaborator

IMHO it should be implemented in the same way as in Fragment – each Node should be ViewModelStoreOwner with own ViewModelStore. Then throw everything into single FragmentManagerViewModel-like ViewModel that we will register once in activity integration point.

Anyway in both variants we have another problem – we change DI flow. We usually pass all dependencies into Node ready to use, but here we have to instantiate ViewModel inside Node, comparing to everything else that is done outside. Not like a bad thing, something that I want to come to later.

@RationalRank
Copy link
Contributor

IMHO it should be implemented in the same way as in Fragment – each Node should be ViewModelStoreOwner with own ViewModelStore. Then throw everything into single FragmentManagerViewModel-like ViewModel that we will register once in activity integration point.

Anyway in both variants we have another problem – we change DI flow. We usually pass all dependencies into Node ready to use, but here we have to instantiate ViewModel inside Node, comparing to everything else that is done outside. Not like a bad thing, something that I want to come to later.

This is what we're doing internally right now. I've published a sample demonstrating this

@KovalevAndrey
Copy link
Collaborator Author

KovalevAndrey commented Dec 17, 2022

IMHO it should be implemented in the same way as in Fragment – each Node should be ViewModelStoreOwner with own ViewModelStore. Then throw everything into single FragmentManagerViewModel-like ViewModel that we will register once in activity integration point.

Anyway in both variants we have another problem – we change DI flow. We usually pass all dependencies into Node ready to use, but here we have to instantiate ViewModel inside Node, comparing to everything else that is done outside. Not like a bad thing, something that I want to come to later.

yes, that's what I was gonna do as a second option

@mattinger
Copy link

mattinger commented Feb 21, 2023

The standard lifecycle viewModel functions for compose delegate to the composition local LocalViewModelStoreOwner. Wouldn't it just be a matter of coming up with a proper implementation of this which was aware of the navigation graph, and setting up the composition local for each node? (Yes, i know i'm making the implementation seem simpler than it probably would be)

I honestly wonder how compose navigation achieves this.

Update: I took a look and it seems like NavBackStackEntry itself implements several interfaces including the LifecycleOwner and ViewModelStoreOwner. So what i assume happens is that as each backstack entry is created, a completely new store is instantiated and registered as the local store owner. And when it's popped off, it is completely cleared. This makes a ton of sense to me obviously, and I think it could honestly be done in this library as well.

@KovalevAndrey
Copy link
Collaborator Author

KovalevAndrey commented Mar 2, 2023

The standard lifecycle viewModel functions for compose delegate to the composition local LocalViewModelStoreOwner. Wouldn't it just be a matter of coming up with a proper implementation of this which was aware of the navigation graph, and setting up the composition local for each node? (Yes, i know i'm making the implementation seem simpler than it probably would be)

I honestly wonder how compose navigation achieves this.

Update: I took a look and it seems like NavBackStackEntry itself implements several interfaces including the LifecycleOwner and ViewModelStoreOwner. So what i assume happens is that as each backstack entry is created, a completely new store is instantiated and registered as the local store owner. And when it's popped off, it is completely cleared. This makes a ton of sense to me obviously, and I think it could honestly be done in this library as well.

Hi @mattinger we're considering the same approach in this draft pr but the final decision whether to make it as a part of framework hasn't been made yet.

We recently introduced RetainedInstanceStore which allows you to implement ViewModel support the way you described right now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants