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

#467 add viewmodel saved state support #496

Open
wants to merge 4 commits into
base: master
from

Conversation

@kedzie
Copy link

commented Jun 14, 2019

This is for issue #467

@kedzie

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

@arnaudgiuliani any thoughts? I was wondering about the sharedViewModel, if we need variations of the methods for those as well. sharedViewModelWithState, I was trying to avoid that.

I doubt master is the right target branch for this, but I'll let you decide where it should go.

@arnaudgiuliani

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

Hello @kedzie , good job.

Could you propose something without adding a new injection function?

We could pass a param from the viewModel dsl keyword like viewModel(useState = true) { (state : ...) -> YourVIewModel(sate) } or even propose a new keyword stateViewModel { ... }

Don't know what could be the best.

And from the resolution step, we can detect if we have the flag property to use the new factory

@kedzie

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

Will do ! thanks for taking a look

@kedzie

This comment has been minimized.

Copy link
Author

commented Jun 29, 2019

@arnaudgiuliani Great advice! made the PR much much cleaner. This attribute method is great that the consumer at the injection point doesn't care about what kind of viewmodel it is.

@coro101

This comment has been minimized.

Copy link

commented Jul 7, 2019

@kedzie I hope this pull request will be merged soon! Great work!

@ustadenis

This comment has been minimized.

Copy link

commented Jul 22, 2019

@kedzie Could you please say whan it will be merged? Thank you.

@arnaudgiuliani

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@kedzie one last question, why do we need to expose defaultArguments: Bundle? = null in FragmentExt.kt or other viewModel() extension?

@arnaudgiuliani arnaudgiuliani added this to the 2.1.0 milestone Jul 25, 2019

@kedzie

This comment has been minimized.

Copy link
Author

commented Jul 26, 2019

The AbstractSavedStateVMFactory constructor takes a Bundle defaultArgs for initial values in the SavedStateHandle. I opted to expose it at the injection point as opposed to the module DSL because the values may depend on fragment arguments or something. @arnaudgiuliani On second thought maybe would be better to have a lazy approach like parameters. So a lambda returning a bundle as opposed to an actual bundle.

@arnaudgiuliani

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

A lazy approach can be interesting effectively. The thing is to avoid conflict with the actual parameters lambda. Could you reuse it also? 🤔

I mean, reuse the parametersOf lambda stuff to pass your bundle.

@kedzie

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

i can think of 3 potential ways to pass in the default bundle in the definition parameters:

  1. pass Bundle as an additional first parameter of definitions.. so something like by currentScope.viewModel(this ) { parametersOf(Bundle().apply { putString("bling" "blang") }, "vm2") } . problem is that bundle is optional so when resolving this must detect if first parameter is a bundle? what if 2nd parameter is also a bundle? resolving can be complicated..
  2. pass Bundle as an additional last parameter of definitions.. so something like by currentScope.viewModel(this ) { parametersOf("vm2", Bundle().apply { putString("bling" "blang") }) } . if we know the # of parameters expected would could detect if an extra one has been passed in, which is the Bundle.
  3. Could have bundle be passed into the parameters method and populated... but this doesn't really match the rest of koin.. by currentScope.viewModel(this) { defaultArgs -> defaultArgs.putString("bling", "blang") parametersOf("vm2") }

An argument for keeping it the way it is:

ViewModelParameters already has another lambda for val from: ViewModelStoreOwnerDefinition? = null
where typealias ViewModelStoreOwnerDefinition = () -> ViewModelStoreOwner

So the defaultArguments bundle as it's own lambda follows that same pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.