search

gnosis / safe-android-kouban

2 stars 2 forks source link

LivaData not suited for UI updates #96

Open jpalvarezl opened 4 years ago

jpalvarezl commented 4 years ago

MutableLiveData and LiveData are not suited for UI updates.

postValue and emit methods only emit the latest value if they are called in a very short interval. Therefore the pattern "emit data then emit loading state false" will only emit "loading state false", there is no sense of backpressure, or rather, the backpressure strategy is simple emit the latest value.

This renders this completely useless for UI state changes, in which we can't afford to lose any emission. There needs to be backpressure and none of the objects can be dropped.

https://developer.android.com/reference/androidx/lifecycle/MutableLiveData

elgatovital commented 4 years ago

This statement about LIveData is exaggerated. In most cases, we are interested only in the latest state when dealing with UI changes. LiveData mostly used to update UI on the main thread. Excessive updates will only waste & block UI thread. If used as intended, LiveData does its job. Why would we need backpressure for UI updates in our project? This sounds like an artificial problem for me

jpalvarezl commented 4 years ago

No Vitaly, it is not a fabricated problem. I logged it because I experienced it yesterday. If you want I can give you a live demo so you believe me :)

rmeissner commented 4 years ago

You could also change the view state object in a way that it always contains the complete state (see safe-android-authenticator).

jpalvarezl commented 4 years ago

I am not against reducing state changes, but then we would have to ensure that all the paths in a method call lead to a single call of postValue. Code reviews would be the only way to enforce this, which may be a bit much considering the cognitive load of a code review.

rmeissner commented 4 years ago

That highly depends on your architecture :D (little bit offtopic discussion). E.g. in the authenticator prototype you have this livedata as part of the base viewmodel and this has a method updateState (https://github.com/gnosis/safe-android-authenticator/blob/master/app/src/main/java/io/gnosis/safe/authenticator/ui/base/BaseViewModel.kt#L49) which handels all this. So in the implementation you only check that you call updateState which is quite easy to review imo.

Again this hightly depends on the architecture and I don't think it makes sense to change the current prototype right now.

rmeissner commented 4 years ago

We could probably implement our own lifedata with backpressure 🤔

jpalvarezl commented 4 years ago

That looks nice actually, encapsulating that logic in a super class. Having a BaseViewModel makes total sense. I opened this ticket more as a reminder to have this discussion in the future or at any point in time :) But yeah, this is future talk for future prototypes or refactorings ahead.

Either way, having a liveData with backpressure would definitely come in handy. If what's in the authenticator is thoroughly tested, I wouldn't be against adopting it.