Add initialState to MvRxFactory, give access to args in create ViewModel #169

BenSchwab · 2019-01-10T00:57:06Z

Closes #83

Motivation:

If you need to use a value from args only in the viewModel (say an id to call an API) the only way to access in the view model is to manually find the arguments (painful) or add a "dead" field to state class.
If you need to access information to create initialState that is not stored in args, but say in your dagger component, there is no way to access it via a secondary constructor.

Breaking changes

MvRxFactory<State>#create(activity: Activity, state: State) : BaseMvRxViewModel<State> ->
MvRxFactory<State, ViewModel>#create(viewModelContext: ViewModelContext, state: State) : ViewModel

Note, activity is now referenceable as viewModelContext.activity.

The second parameter was added to MvRxFactory because it guarantees that you cannot accidentally return a ViewModel of the wrong type in create.

(unreleased) Removes MvRxFragmentViewModelFactory Allow for Fragment ViewModel factory (#69) #148. I prefer the single factory in this PR, as if you used a MvRxFragmentViewModelFactory but scoped your ViewModel to an activity, there will be a crash. This new approach forces that possibility to be acknowledged in code.

New

Factory now has a method initialState(viewModelContext: ViewModelContext). This gives access to the args and the activity/fragment before creating the state. You can now use dagger to inject dependencies into your state. The existing constructor based state creation still works, however, it has lower precedence that initialState in the factory. If both are defined, the initialState from the factory will be used.
No more requirement for @JvmStatic. I wrote some java reflection to access the Companion object. It seems to be working fine, and should make onboarding a lot easier, as forgetting @JvmStatic is very common.

Open questions

Does anyone have a better idea for a name than ViewModelContext. I hate overloading the word context, but it feels like that is what the combination of the owner and args is.
Any typed args. In the old manner, args would be automatically cast to the type of a matching secondary constructor. I couldn't figure out a clean way to get this to work without forcing on generic on ViewModelFactory. As many times you will use a ViewModelFactory without needing args, it felt bad to force many declarations to have a meaningless <Any>. Also the Factory has two generics, a third is a lot...
Currently, the context has rawArgs which is just the untype arg object, and fun <T> args() a convenience method to type args.
Note: it is totally possible, and expected to be able to handle many types of args, which can be done cleaning with a when on rawArgs.

@gpeal @hellohuanlin @elihart

…factory view model create method

BenSchwab · 2019-01-10T00:57:42Z

mvrx/src/test/kotlin/com/airbnb/mvrx/MvRxExtensionsTest.kt

-
-  @Test
-  fun testInitialDemoState1() {
-    val state = _initialStateProvider(DemoState1::class.java, Arg("Hello"))


These providers are gone. Lots of other tests cover this code too.

gpeal

I feel very good about this. 👍 from me.

Let's wait for @hellohuanlin, @elihart, and @marukami to chime in just in case.

gpeal · 2019-01-10T02:45:57Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxExtensions.kt

+ * Looks for [MvRx.KEY_ARG] on the arguments of the fragments.
+ */
+@Suppress("FunctionName")
+fun <T : Fragment> T._fragmentArgsProvider(): Any? = arguments?.get(MvRx.KEY_ARG)


@BenSchwab @RestrictTo(Library)

gpeal · 2019-01-10T02:46:09Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxExtensions.kt

 *
 * Also adds the fragment's MvRx args to the host Activity's [MvRxViewModelStore] so that they can be used to recreate initial state
 * in a new process.
 */
 @Suppress("FunctionName")
-inline fun <reified S : MvRxState, T : Fragment> T._activityViewModelInitialStateProvider(keyFactory: () -> String): S {
+inline fun <T : Fragment> T._activityArgsProvider(keyFactory: () -> String): Any? {


@RestrictTo(Library) this too

elihart

This seems great! I think it the api you landed on is a good compromise.

As to your open questions, I don't have any better suggestions, I think you've made strong points and thought this through well, so I don't have anything else to add. nice stuff :)

great to see it work without jvmstatic too

elihart · 2019-01-10T03:23:19Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxExtensions.kt

 *
 * Also adds the fragment's MvRx args to the host Activity's [MvRxViewModelStore] so that they can be used to recreate initial state
 * in a new process.
 */
 @Suppress("FunctionName")
-inline fun <reified S : MvRxState, T : Fragment> T._activityViewModelInitialStateProvider(keyFactory: () -> String): S {
+inline fun <T : Fragment> T._activityArgsProvider(keyFactory: () -> String): Any? {


the idea of "activity args" is confusing to me since the args are still coming from a fragment, and the function receiver is a fragment

Agreed, let me explain the reasons a bit more in the comments.

elihart · 2019-01-10T03:23:34Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxExtensions.kt

 *
 * Also adds the fragment's MvRx args to the host Activity's [MvRxViewModelStore] so that they can be used to recreate initial state
 * in a new process.
 */
 @Suppress("FunctionName")
-inline fun <reified S : MvRxState, T : Fragment> T._activityViewModelInitialStateProvider(keyFactory: () -> String): S {
+inline fun <T : Fragment> T._activityArgsProvider(keyFactory: () -> String): Any? {
    val args: Any? = arguments?.get(MvRx.KEY_ARG)


could this chain to _fragmentArgsProvider instead?

elihart · 2019-01-10T03:26:37Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

     */
-    fun create(fragment: Fragment, state: S): BaseMvRxViewModel<S>
+    abstract fun <A> args(): A


is the implementation the same both subclasses? can we not have a default implementation?

elihart · 2019-01-10T03:28:41Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

+ * The [ViewModelContext] for a ViewModel created with an
+ * activity scope (`val viewModel by activityViewModel<MyViewModel>`).
+ */
+class ActivityViewModelContext(


It's not immediately clear to me why we need to separate fragment and activity contexts, since an activity scoped viewmodel is still initially created for a specific fragment, and with arguments from that fragment.

I suppose it's just during process restart that we don't have the original fragment anymore? That's subtlety makes this requirement/distinction not immediately clear

Fragment access is needed for simple Dagger support. With access to the fragment, all you need to do is inject a ViewModel factory. With #148 I just forward the MvRx factory invoke to the dagger injected factory in the fragment.

@elihart Yes, during process activity scoped ViewModels will be recreated when the activity is created. So even in a normal creation sequence we will pass it only the activity (even though a fragment reference is technically accessible).

elihart · 2019-01-10T03:31:27Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelProvider.kt

@@ -156,4 +113,38 @@ object MvRxViewModelProvider {
        }
        return null
    }
+
+    /**
+     * For internal use only. Public for inline.


you can make this internal and use @PublishedApi instead of needing to make it public

elihart · 2019-01-10T03:32:10Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelProvider.kt

+                "Attempt to create the MvRx state class ${stateClass.simpleName} has failed. One of the following must be true:" +
+                    "\n 1) The state class has default values for every property." +
+                    "\n 2) The state class has a secondary constructor for ${args?.javaClass?.simpleName ?: "a fragment argument"}." +
+                    "\n 3) The ViewModel using the state must has a companion object implementing MvRxFactory with an initialState function " +


nit: must has

marukami · 2019-01-10T03:56:03Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

+ * The [ViewModelContext] for a ViewModel created with an
+ * activity scope (`val viewModel by activityViewModel<MyViewModel>`).
+ */
+class ActivityViewModelContext(


Fragment access is needed for simple Dagger support. With access to the fragment, all you need to do is inject a ViewModel factory. With #148 I just forward the MvRx factory invoke to the dagger injected factory in the fragment.

marukami · 2019-01-10T04:37:53Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

     */
-    fun create(activity: FragmentActivity, state: S): BaseMvRxViewModel<S>
+    fun create(viewModelContext: ViewModelContext, state: S): VM? = null


The issue I ran into with having a single create in #148 was casting. If the consumer wants the FragmentViewModelContext then you need to downcast the ViewModelContext.

For me, cause Dagger injects the ViewModel Factory implementation into the fragment, I would need to downcast for virtually all my ViewModels factories to use the FragmentViewModelContext.

@marukami Yeah, that is a downside, but I don't see a safe way around this. It seems too risky to have a ViewModelFactory that will crash if you use an activity scope.

I think, if in your application you know that you will only use fragment scoped view models, you could do something like:

interface MvRxViewModelFragmentFactory<VM: BaseMvRxViewModel<S>, S : MvRxState> : MvRxViewModelFactory<VM, S> { fun createFragmentScopedViewModel(viewModelContext: FragmentViewModelContext, state: S) : VM? override fun create(viewModelContext: ViewModelContext, state: S) = createFragmentScopedViewModel(viewModelContext as FragmentViewModelContext, state) override fun initialState(viewModelContext: ViewModelContext) : S? = initialStateFragmentScoped(viewModelContext as FragmentViewModelContext) fun initialStateFragmentScoped(viewModelContext: FragmentViewModelContext) : S? }

That way you are explicitly performing the unsafe cast. Does this seem reasonable?

With the changes @gpeal suggested for the Fragment been a generic, I think an extension function like this should work nicely.

@Suppress("UNCHECKED_CAST") inline fun <F: Fragment> ViewModelContext.withFragmentFactory(factory: (F) -> ViewModel) = (this as FragmentViewModelContext).let { it.fragment as F }.let(factory)

marukami · 2019-01-10T04:45:02Z

mvrx/src/test/kotlin/com/airbnb/mvrx/FactoryTest.kt

+class FactoryViewModelTest : BaseTest() {
+
+    private class TestFactoryViewModel(initialState: FactoryState, val otherProp: Long) : TestMvRxViewModel<FactoryState>(initialState) {
+        companion object : MvRxViewModelFactory<TestFactoryViewModel, FactoryState> {


Should TestFactoryViewModel be using FragmentViewModelContext?

I think you are suggesting making the test actually verify that the a FragmentViewModelContext is used? If so, I agree. Updated the test.

…reference, more robust tests

gpeal · 2019-01-10T16:09:25Z

mvrx/src/main/kotlin/com/airbnb/mvrx/MvRxViewModelFactory.kt

+class FragmentViewModelContext(
+    override val activity: FragmentActivity,
+    override val rawArgs: Any?,
+    val fragment: Fragment


@BenSchwab You could make a <T : Fragment> fragment() and <A : Activity> activity() function that could help with casting to your own app's Activity/Fragment

…ialState (#181) #169 Added a new initialState method and ability to not use @JvmStatic all of these need keep rules. Closes #176 ** Note: Kotlin 1.3+ requires this keep rule as well: keep class kotlin.reflect.jvm.internal.impl.serialization.deserialization.builtins.BuiltInsLoaderImpl Per the discussion in #138, this is not a project specific rule, so not including it in the proguard file, but updated wiki to suggest it. Tested on sample app. Created #182 to prevent this from the future. Would be a great first task if anyone from the community wants to jump on it :)

Allow creating initialState from MvRxFactory, give access to args in …

db8d9c8

…factory view model create method

BenSchwab commented Jan 10, 2019

View reviewed changes

Better comment

5e81915

gpeal approved these changes Jan 10, 2019

View reviewed changes

elihart approved these changes Jan 10, 2019

View reviewed changes

marukami reviewed Jan 10, 2019

View reviewed changes

BenSchwab added 3 commits January 9, 2019 21:49

comments, restrict to library

ec5e674

Chain arg providers

92f7de4

Better explanation for why activity view model can only get activity …

edbb23e

…reference, more robust tests

gpeal reviewed Jan 10, 2019

View reviewed changes

Add convience methods to type fragment and activity

3df423c

BenSchwab merged commit aa9779a into master Jan 11, 2019

gpeal deleted the bschwab--new-factory branch January 11, 2019 16:08

BenSchwab mentioned this pull request Jan 14, 2019

Add new keep rule for MvRxViewModelFactory without JvmStatic and initialState #181

Merged

BenSchwab mentioned this pull request Apr 3, 2019

[internal] Allow createViewModel to override stateFactory #219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initialState to MvRxFactory, give access to args in create ViewModel #169

Add initialState to MvRxFactory, give access to args in create ViewModel #169

BenSchwab commented Jan 10, 2019 •

edited

Loading

BenSchwab Jan 10, 2019 •

edited

Loading

gpeal left a comment

gpeal Jan 10, 2019

gpeal Jan 10, 2019

elihart left a comment

elihart Jan 10, 2019

BenSchwab Jan 10, 2019

elihart Jan 10, 2019

elihart Jan 10, 2019

elihart Jan 10, 2019

marukami Jan 10, 2019

BenSchwab Jan 10, 2019

elihart Jan 10, 2019

elihart Jan 10, 2019

marukami Jan 10, 2019

marukami Jan 10, 2019

BenSchwab Jan 10, 2019

marukami Jan 10, 2019 •

edited

Loading

marukami Jan 10, 2019

BenSchwab Jan 10, 2019 •

edited

Loading

gpeal Jan 10, 2019

Add initialState to MvRxFactory, give access to args in create ViewModel #169

Add initialState to MvRxFactory, give access to args in create ViewModel #169

Conversation

BenSchwab commented Jan 10, 2019 • edited Loading

Breaking changes

New

Open questions

BenSchwab Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

gpeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elihart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marukami Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenSchwab commented Jan 10, 2019 •

edited

Loading

BenSchwab Jan 10, 2019 •

edited

Loading

marukami Jan 10, 2019 •

edited

Loading

BenSchwab Jan 10, 2019 •

edited

Loading