sockeqwe / mosby

A Model-View-Presenter / Model-View-Intent library for modern Android apps
http://hannesdorfmann.com/mosby/
Apache License 2.0
5.49k stars 841 forks source link

Showing Snackbar via MVI #255

Closed qwert2603 closed 7 years ago

qwert2603 commented 7 years ago

Hello! I was really excited by MVI patters and concept of StateReducer particularly. It seems to be very useful and powerful. But there is a problem with some actions that should be executed once. For example, show Snackbar or scroll list to top after refreshing. In some cases this actions should be executed only once, because user doesn't expect that list will be scrolled to top after screen rotation. In comments to Your article (http://hannesdorfmann.com/android/mosby3-mvi-1) I found two ways of making such actions:

  1. "the proper way (or the clean way) would be to let the intent of having dismissed snackbar (or snackbar disappeared) sink down to your model!" -- but there may be a sutuation when several new ViewState instances created before SnackbarShownIntent will be triggered, so Snackbar will be shown several times
    whatever.
  2. Reset flags (like "showSnackbar" or "scrollToTop") in StateResucer, but it pollutes StateResucer if there are several such flags.

After some research and experimenting I found some solution when such action will be executed once and only once. I called them OneShotFlag. Here is what have I come to:

class OneShotFlag {
    var readId: Long? = null

    fun getFlag(vsId: Long) =
            if (readId == null) true.also { readId = vsId }
            else readId == vsId

    companion object {
        val USED = OneShotFlag().also { it.readId = -1L }
    }
}

open class VS_ID {
    val id: Long = VS_ID.getNextId()

    companion object {
        private var lastId = 1L

        fun getNextId() = lastId++
    }
}

data class VS(
        val model: Int,
        val scrollToTop: OneShotFlag
) : VS_ID()

fun render(vs: VS) {
    println("${vs.id} ${vs.scrollToTop.readId} ${vs.scrollToTop.getFlag(vs.id)}")
}

fun main(args: Array<String>) {
    val v1 = VS(4, OneShotFlag.USED)
    render(v1)
    val v2 = v1.copy(scrollToTop = OneShotFlag())
    render(v2)
    val v3 = v2.copy(model = 5)
    render(v3)
    val v4 = v3.copy(scrollToTop = OneShotFlag())
    val v5 = v4.copy(model = 6)
    render(v4)
    render(v5)
    render(v4)
    render(v3)
    render(v4)
    render(v5)
}

Output is:

1 -1 false
2 null true
3 2 false
4 null true
5 4 false
4 4 true
3 2 false
4 4 true
5 4 false

Class OneShotFlag represents action that should be executed once. It has readId field where stored ID of ViewState that was being rendered when action was executed. So (as we can see in fun getFlag(vsId: Long)) this action will not be executed when we will render another VS. Moreover if later we will render VS with VS.id == OneShotFlag.readId action will be executed again. So we still have Undo/Redo support while renderind ViewStates. This is experiment and I'm looking forward for Your comments and critique. Does it seems to be good approach?

sevar83 commented 7 years ago

I just use some flag in the view that is set to true on the first scroll. After rotation it's not a problem because the presenter is kept and the last rendered (not the initial one) ViewState is emitted again. So the scrolling happens just one time as it should be. This could work also with stuff like animated transitions.

dimsuz commented 7 years ago

Interesting solution with OneShotFlag, but can present a bit of an overhead if there are a lot of such flags.

As for me I usually have my ViewState models have a method clearTransientState() which resets all one-shot state variables to their default values (by calling this.copy()) and it is the first to be called in the state reducer function. Of course, one needs to update this method every time a new one-shot variable is added, but as it is a part of a ViewState data class, I usually do not forget.

qwert2603 commented 7 years ago

@dimsuz but some flags may be lost in a situation when several ViewStates will be created while Fragment (or Activity) is recreating and only last ViewState will be rendered when View is reattached.

sockeqwe commented 7 years ago

Super interesting conversation! Thanks for sharing your strategies. I think all of your solutions are valid and work for your use case but not necessarily for the general case.

With that said, I still think that sinking the information, i.e. that you have scrolled to a given position, down as intent through a state reducer is the way I would implement something like this and how I would recommend to implement it. That might seems to be over architected and over complicated and by no means I'm saying that you should change your OneShotFlag solution, or clearTransientState() as described by @dimsuz . I just can speak for myself and for me the most important thing is that the "model" is immutable and driving the state. Also I don't see a major difference to what @dimsuz or @sevar83 recommended: They all have to call 1 extra method (clearTransientState() or some flag in the view). So it's the "same amount of work" compared to firing a dismissSnackBarIntent().

but there may be a sutuation when several new ViewState instances created before SnackbarShownIntent will be triggered, so Snackbar will be shown several times

I think this is just an implementation detail of your UI. If Snackbar is already showing then don't display it again. State Reducer guarantees that the order of those render() events dont matter: at the end you are in the correct state. I'm also experimenting around with the idea of using Rx backpressure to disallow such things like calling render(newState) while "old state is not entirely finished with rendering. In this example "rendering is finished" could mean after snackbar disapeared (might be easier to implement with another base presenter class #255). Not sure if this solves this problem though, but I think it could be useful in the case of RecyclerView Item animations where one render call might trigger an animation of a item and a consecutive render call might cause a that the item that is animating right now will removed. Sure, we could coordinate this in the View layer, but if there would be a way to signalize when view.render() is finished we simply could wait until item animation is finished (first view.render() call) before dispatching the next view.render() call which would remove the item. With unidirectional data flow and Rx backpressure we could get this for free.

but it pollutes StateResucer if there are several such flags.

True but at the same time you get the guarantee that state is always the correct regardless of amount of events happening over time as they all go the same way through state reducer.

qwert2603 commented 7 years ago

@sockeqwe @dimsuz Thank You for your comments. Let's imagine the situation when:

Another approach when we have intent dismissSnackBarIntent(). There may be a situation when:

I'm also experimenting around with the idea of using Rx backpressure to disallow such things like calling render(newState) while "old state is not entirely finished with rendering.

This may solve the problem, but we can't wait seconds for Snackbar's dismissing or animation's finish.

Also I'd like to say that OneShotFlag becomes immutable after first call OneShotFlag#getFlag(vsId: Long). It returns same value for same ViewState_Id. Moreover var readId: Long? = null can be private.

qwert2603 commented 7 years ago

@sevar83 I used such approach before, but refused because I try to make View as passive as possible. And this is duplicating flags in ViewState and View (represented by Fragment or Activity).

sockeqwe commented 7 years ago

In such situation flag showSnackbar=true is lost.

Sounds like the expected behavior for me OR your state reducer should work differently then and set showSnackbar to whatever you would like it to be. I don't see how this makes any difference in having a view attached or not.

Also you can call dismissSnackBarIntent() immediately in render() once the snackbar has been fired. This is just an implementation detail, and SnackBars and Toast behave differently then regular UI widgets. Something like this:

// View Layer
public void render(MyState state) {
   if (state.showSnackBarError) {
      SnackBar.make(view, "An error has occurred", LONG).show()
      fireSnackBarErrorShownIntent(); 
   }
}

So fireSnackBarErrorShownIntent() will set the state properly ((state.showSnackBarError=false) and you are back on the right state (although the SnackBar itself is shown for few more seconds on the screen, but again I think this is ok since it is just a UI implementation detail). Of course fireSnackBarErrorShownIntent() will cause to render() state again but then no UI widget will be update, so no big deal.

qwert2603 commented 7 years ago

Sounds like the expected behavior for me

It all depends on use case. Looks like there is no simple way to solve "Snackbar's problem". Anyway we should write some additional code :)

prithivraj commented 5 years ago

@sockeqwe

How about side effecting on the same stream by merging with a publish relay after the reduce operation? This way the last state for the scan is intact and will be supplied to the view on config change, and yet one-time events can be sent to the same single render function.

https://github.com/prithivraj-toast/DialogMVI/blob/61caf3eb2b4056444840fc38ab1d76b06c0bd444/app/src/main/java/com/zestworks/mvicalc/CalculatorPresenter.kt#L23