oldergod / android-architecture

MVI architecture Implementation of the ToDo app.
Apache License 2.0
669 stars 70 forks source link

Time limited event management #6

Closed oldergod closed 6 years ago

oldergod commented 6 years ago

Basically the case when one needs to display a snackbar and do not want to display it again when the latest state is reemitted (e.g. on rotation or just even onStart() after switching app).

In the blueprint they created a SingleLiveEvent but I really don't think it's a good solution. They are many pros and cons discussed in this thread about this class.

In the above thread, Hannes suggests the view would tell the viewmodel that it has shown the message once it did for the viewmodel to emit a new state containing the message triggering to an off value. I totally agree with this, UI and VM are not coupled, keeps the flow pure, etc. The code looks like this

      if (state.getNextPageError() != null) {
        Snackbar.make(
                   binding.get().loadMoreBar,
                   state.getNextPageError(),
                   Snackbar.LENGTH_LONG
                 )
                 .show();
        searchViewModel.clearNextPageErrorMessage();
      }

So how about the MVI architecture. We have a unidirectional data flow in place, and I think it leads to two solutions (as least I could think of)

Clearing Intent

Instead of telling the viewmodel to clear the error message, we would emit a new intent for the same purpose, turning the message triggering value to null or false.

I think it's ok but

  1. Boilerplate... Having to create a new intent/action/result sounds like an overkill. Might feel better if this happens a lot and could make this logic generic.
  2. Ideally I don't want the rendering function to talk to the intents emitting one. In the architecture graph, they only communicate through the user() and I think it has meanings and respecting this barrier helps make a better app.

Conclusion: works but not fully satisfying (probably being picky)

Emitting two consecutive event

Inside the processor, when emitting a result, it could be done as not only one but two consecutive events would be emitted. The first event will trigger the message and the second event would just clear the trigger. It is basically as above but without any needs to create a new intent from the UI.

It works but

  1. The business logic is now coupled with the UI which is never good.

Conclusion: less boilerplate than above but potentially worse tech debt.

Timely marked event

No implementation but a vague idea about marking the event with a timestamp that the view would check to decide if it needs to display it or not. Something like if (event.within10Seconds() show(). Not sure is there is real value here.


oldergod commented 6 years ago

I'd be happy to hear your opinions @malmstein, @sockeqwe, @kanawish

sockeqwe commented 6 years ago

Well, if you are just concerned about timing: This is how I do timing related stuff in MVI: http://hannesdorfmann.com/android/mosby3-mvi-7

I think it's ok but

1 .Boilerplate... Having to create a new intent/action/result sounds like an overkill. Might feel better if this happens a lot and could make this logic generic.

I assume that the user can trigger the intent / action manually by dismissing the Snackbar manually (swipe to left or right), so it's not more "boilerplate" since I use the action directly

  1. Ideally I don't want the rendering function to talk to the intents emitting one. In the architecture graph, they only communicate through the user() and I think it has meanings and respecting this barrier helps make a better app.

You could add a startWith() operator into your business logic that first emits showSnackBar=true and then immediately showSnackBar=false then you don't want the rendering function to talk to the intents (Snackbar is not a good example here because it is time based, I think the provided link shows a better solution for time based things like Snackbar).

The business logic is now coupled with the UI which is never good.

I think it is ok if you do it Presenter / ViewModel layer and not really deep in your business logic. Presenter / ViewModel are couled to the View somehow in the sense that they are "working" for the view to optimize things. Therefore it's ok to that in Presenter / ViewModel imho.

if (event.within10Seconds() show()

It's exactly the same coupling as emitting 2 consecutive events, just a little bit uglier 😄

oldergod commented 6 years ago

Thank you Hannes. Nice timing for your blog post! It's like the second approach "consecutive events" but you also manage the displayed duration inside the viewmodel.

ps: I don't think or didn't know Snackbar were swipeable (could only be dismissed by the user via snackbar's action).

malmstein commented 6 years ago

I like this approach more than the SingleLiveEvent. One can easily adapt this to a Dialog, BottomSheet, etc... and doesn't hide the underlying problem. Good stuff @sockeqwe, now I only need a way to do all this without RxJava and I'll be happy ever after 💃

sockeqwe commented 6 years ago

Why without RxJava ? Then kotlin coroutines ...

On Sa., 16. Sep. 2017, 08:54 David González notifications@github.com wrote:

I like this approach more than the SingleLiveEvent. One can easily adapt this to a Dialog, BottomSheet, etc... and doesn't hide the underlying problem. Good stuff @sockeqwe https://github.com/sockeqwe, now I only need a way to do all this without RxJava and I'll be happy ever after 💃

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oldergod/android-architecture/issues/6#issuecomment-329950672, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnroksC4UY0rwBEuA61ay0S28NC2ghks5si3C3gaJpZM4PVdlf .

malmstein commented 6 years ago

Nothing against the framework, but it's not my cup of tea. My biggest concern right now is that the whole codebase gets polluted with it, which makes it really hard to move away from it.

I'm looking at coroutines and how to achieve the unidirectional data flow. Hopefully I'll have something decent soon.

oldergod commented 6 years ago

With Kotlin, that could be done without much harm, coroutine to avoid callback hell and the Kotlin libs to process collections nicely. Write everything reactively is still a problem though.

On Sep 16, 2017 16:20, "David González" notifications@github.com wrote:

Nothing against the framework, but it's not my cup of tea. My biggest concern right now is that the whole codebase gets polluted with it, which makes it really hard to move away from it.

I'm looking at coroutines and how to achieve the unidirectional data flow. Hopefully I'll have something decent soon.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/oldergod/android-architecture/issues/6#issuecomment-329951718, or mute the thread https://github.com/notifications/unsubscribe-auth/ABr49TRrE5tlvsZiyDTJiHnXCO8sp4E2ks5si3bAgaJpZM4PVdlf .

oldergod commented 6 years ago

Ended up doing something similar to Hannes' solution

https://github.com/oldergod/android-architecture/blob/6f6688a128a435554e8dca4d0d0df84cf8e9702a/todoapp/app/src/main/java/com/example/android/architecture/blueprints/todoapp/util/ObservableUtils.java#L12-L17