[OpenQuestion/Idea] One-shot event handling

[ema987]

Hello,

first of all thank you for this amazing library!

This issue is about an idea to handle one-shot events like the one explained in #400 or similar issues.

I also encountered similar scenarios, and after trying using selectSubscribe() and uniqueOnly() I wasn't fully satisfied with the approach. It moves some UI logic away from the invalidate() function where I know the UI logic is.

I eventually started resetting state's asyncs as soon as the UI consumed them, like the following example:

State:

data class HelloWorldState(val moveToNextScreen: Async<Unit> = Uninitialized) : MvRxState

Fragment:

override fun invalidate() {
        withState(viewModel) {state ->
            when(val moveToNextScreen = state.moveToNextScreen) {
                is Loading -> {
                    //show loading
                }
                is Fail -> {
                    //show fail
                }
                is Success -> {
                    //show success and move to next screen
                    viewModel.onMoveToNextScreen() // moveToNextScreen is handled and the viewModel is informed
                }
            }
        }
}

ViewModel:

fun onMoveToNextScreen() {
    setState {
        copy(onMoveToNextScreen = Uninitialized)
    }
}

I started using this approach most of the time and I liked it, but everytime I needed to write these reset functions.

The next approach I followed was writing a general function like this:

fun reset(vararg items: KProperty<Any>) {
    if (items.contains(HelloWorldState::moveToNextScreen)) {
        setState {
            copy(moveToNextScreen = Uninitialized)
        }
    }
}

So, to use the same example as before, instead of calling viewModel.onMoveToNextScreen() you would call viewModel.reset(HelloWorldState::onMoveToNextScreen). This works, but the code I was writing started to became boilerplate, so I though about annotation processing.

This is what I came up with:

3 annotations which should be used on ViewModel, State and its properties.

• ResettableViewModel
• ResettableState
• ResettableProperty

For every property annotated with ResettableProperty inside a state annotated with ResettableState, the annotation processor will create an extension function on the corresponding viewModel annotated with ResettableViewModel which can be used to reset the property to its default value specified in the state.

Here an example with the most relevant parts:

State:

@ResettableState
data class HelloWorldState(@ResettableProperty("DEFAULT_TITLE") val title: String = DEFAULT_TITLE,
                           @ResettableProperty("DEFAULT_DESCRIPTION") val description: Async<String> = DEFAULT_DESCRIPTION) : MvRxState {

    companion object {
        const val DEFAULT_TITLE = "Default title"
        val DEFAULT_DESCRIPTION = Uninitialized
    }

}

Fragment:

class HelloWorldFragment : BaseMvRxFragment() {

    //...

    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        resetTitleButton.setOnClickListener {
            viewModel.resetTitle() //this is generated by the annotation processor
        }
        resetDescriptionButton.setOnClickListener {
            viewModel.resetDescription() //this is generated by the annotation processor
        }
        resetAllButton.setOnClickListener {
            viewModel.reset() //this is generated by the annotation processor
        }
    }
}

ViewModel:

@ResettableViewModel
class HelloWorldViewModel(initialState: HelloWorldState) : MvRxViewModel<HelloWorldState>(initialState) { 
    //...
}

The functions resetTitle(), resetDescription() and reset() are generated by the annotation processor and can be called on the viewModel. The reset() function will reset all the ResettableProperty at once. For the assignment of the default values I followed the approach you use in Epoxy and Paris.

For the sake of simplicity I created some buttons with their click listeners to easily invoke them.

This is the generated code for resetDescription():

fun HelloWorldViewModel.resetDescription() {
  val setStateFunction = findBaseMvRxViewModelClass(this::class.java).declaredMethods.singleOrNull {
      it.name == "setState" } ?: throw IllegalStateException("setState function not found!")
  setStateFunction.isAccessible = true
  val newState: (HelloWorldState.() -> HelloWorldState) = {
  copy(description = HelloWorldState.DEFAULT_DESCRIPTION)
  }
  setStateFunction.invoke(this, newState)
}

What I like of this approach:

• very easy to have reset functions just annotating some classes and properties
• you don't care about those, they are compiler generated

What I don't like:

• the resetNAMEOFTHEPROPERTY() name is probably not correct, I would like to change to something like onNAMEOFTHEPROPERTYHandled(), otherwise it seems like is the fragment to tell the viewModel what to do instead of the viewModel to react on something that happens in the fragment
• it uses reflection (only Java, no Kotlin-reflect) to invoke setState() and I feel kind of bad because it seems you are calling setState() outside of the viewModel.

You can see a working example at https://github.com/ema987/MvRx/tree/feature/state-resetter

Before opening a PR and improving the code I'd like to know from you if you like the idea and would like to have it in your repository or if you dislike it at all :)

Sorry for the long post, I tried to show you what was the full path that led me to this and I hope it makes it more understandable.

Please let me know. Thank you!

[elihart]

Thanks for the writeup and sharing your approach.

I personally want to avoid annotation processors as much as possible, as they add quite a bit of complexity and also increase compilation time. Many things can also be done by leveraging kotlin language techniques instead. This is coming from my experience maintaining several annotation processor based libraries, such as Epoxy.

What do you think about using reflection instead of annotation processing? You could have a function like this in your base viewmodel

fun <T> reset(property: KProperty<S, Async<T>>)

Which could then use reflection to do the same resetting behavior.

Usage would be like

is Success -> {
        viewModel.reset(HelloWorldState::moveToNextScreen)

[gpeal]

@ema987 @elihart Exposing something like that would basically give anybody access to clear a specific state property which is something that should be owned by the ViewModel. A reset function can be a single line like:

fun resetName() = setState { copy(name = null) }

I don't think it's bad to enforce being explicit about what can and can't be cleared this way

[ema987]

@elihart @gpeal thank you for your responses!

@elihart the use of reflection was another approach I tried to follow (after discussing with my friend @reavcn) which I didn't mention before. I created a similar function like the one you suggested in the base viewModel, but using only the Java reflection I wasn't able to invoke the copy method because it looks like to me it's impossible to know the parameters' order, neither the state data class members order (as stated in Class:getDeclaredFields() documentation which says The elements in the returned array are not sorted and are not in any particular order). I didn't want to include the Kotlin reflection library but it seems it's the only way, so after your message I tried again and I've came up with something that works.

On the base viewModel you have two functions which you can use to reset the state's properties:

fun reset(), which will reset all the resettable properties (more on this later) fun <T> reset(vararg propertiesToReset: KProperty1<out S, T>), which will reset the properties given as parameters (if possible, more on this later)

@gpeal I agree with you. To try to enforce this, I've made an annotation ResettableProperty which must be used on the state's properties which you want to be resettable. You can also specify the default value of the property. Trying to reset a not annotated property will throw an exception.

I think it can probably be improved (the functions could be made as extension on BaseMvRxViewModel, I presume) but the sample currently works and you can try it at https://github.com/ema987/MvRx/tree/feature/state-resetter-reflection

Thank you both for making me think about this.

[gpeal]

@ema987 I think your sample actually adds more code and overhead than just making a reset function on your view model since you now need a static const default value and annotation.

I would still recommend this in one of two ways:

data class MyState(
  val myProp: Int = 0
) : MvRxState {
  fun resetProp() = copy(prop = 0)
}

class MyViewModel(...) : MvRxViewModel<MyState>(...) {
    fun resetProp() = setState { resetProp() }
    // or
    fun resetProp() = setState { copy(myProp = 0) }
}

My main issue with the reset function is that the ViewModel no longer exposes an explicit API of actions. There is this entirely new implicit API of resettable things that the ViewModel has no control over and can easily lead to unexpected states or unintended side effects.

A second issue with this is that sometimes, it only makes sense to set or clear multiple properties at the same time and this has no way of enforcing that whereas it could be encapsulated and testable in either of my examples above.

[ema987]

@gpeal the static const default value is actually needed if you want to have a default value different than Uninitialized for Asyncs and null for other types. This is probably the default case you would actually end up having all the time, but with the static const you would be able to customize it. The annotation could be useful to mark in some way the resettable properties to make them different from the others.

I see your point when you say My main issue with the reset function is that the ViewModel no longer exposes an explicit API of actions, which is another reason why I prefer the annotation processing approach even if is more complicated to maintain. Using that approach you will have resetProp() only for the properties you annotated (you can avoid to generate the resetAllProps() function to avoid hiding resets of something you don't immediately get from the name of the function).

The approach you suggest, if I'm understanding it correctly, is to manually write in the state or in the viewModel the reset() function for each property which needs a reset (this is actually what you would have with the annotation processing approach). Even if this is 100% explicit and this is what I was doing at the beginning, in the long run it becomes annoying to write, that's why I was looking for a better approach which could sacrifice some explicitness in favour of less code to write. The state-resetter-reflection sample sacrifices explicitness more than the annotation processor approach (and of the manual approach, of course), but you gain that you don't have to write any more code to reset a property, neither any code has to be generated by the annotation processor, since it will work for all the states and the viewModels.

Unfortunately all of the approaches have their flaws. I appreciate your point of view, thank you for the discussion :D

[gpeal]

@ema987 I'm surprised that the rest functionality is so common for you. Maybe there is another approach. I've definitely seen the approach used but maybe just once or twice each in <1% of all MvRx screens I've seen.

[elihart]

In my opinion this is a rare enough case, with a minimal boilerplate savings, that optimizing it with the reflection or annotation processor approach in the core library is not worthwhile. As a library I think it is best to lean towards explicitness and simplicity at a slight cost to verbosity.

If developers like you want to build on top of this and have a simpler pattern just for your own usage then I recommend putting the reflection approach in your base viewmodel so you can use it in your project, and customize it for however you want, but I agree with @gpeal that it compromises data integrity so is best not to mainstream in mvrx

[ema987]

I see your points and I agree with them, also because I raised some of them by myself.

@gpeal @elihart thank you both for the effort and the time you put in this discussion, it was kind of you and very interesting discussing of different approaches.

I close this issue now :)