ReactiveCircus / FlowBinding

Kotlin Coroutines Flow binding APIs for Android's platform and unbundled UI widgets, inspired by RxBinding.
https://reactivecircus.github.io/FlowBinding/
Apache License 2.0
901 stars 42 forks source link

Issue with conflate() operator in bindings #83

Closed svenjacobs closed 4 years ago

svenjacobs commented 4 years ago

I use the events() binding in my application. More precisely I use it to listen for view lifecycle events in a Fragment like viewLifecycleOwner.lifecycle.events().

However the ON_START event is swallowed for some reason. For example I do receive ON_CREATE and ON_RESUME but not ON_START. It seems like this is a race conditions as ON_START does occur depending of where/when I listen for these events.

I noticed that you apply a conflate() on the Flow. I copied the code of events() and just removed conflate(). With my version I receive ON_START all the time. So I wonder why do you apply conflate()? It seems you add this to all bindings? Is there a specific reason?

ychescale9 commented 4 years ago

In general when the consumer of UI events cannot catchup with the emission of these events, the consumer only cares about the latest event and it doesn't make sense to replay all the emissions (e.g. multiple click events). conflate() (similar to BackpressureStrategy.LATEST in RxJava) does just that.

Event emission   | 1   2   3   4   5
Event collection | process...event...1... | process event 5..... |

I suspect the reason you are missing ON_START is that your collector is doing some work that takes longer than the emission of those lifecycle events.

override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
    super.onViewCreated(view, savedInstanceState)
    viewLifecycleOwner.lifecycle.events()
        .onEach {
            Timber.d("LifecycleEvent: $it")
            delay(50)
        }
        .launchIn(lifecycleScope)

Output:

LifecycleEvent: ON_CREATE
LifecycleEvent: ON_RESUME

The timeline looks like this:

Event emission   | ON_CREATE   ON_START   ON_RESUME
Event collection | process.......ON_CREATE..........| process ON_RESUME..... |

However the lifecycle events are different to the other UI events in nature as they are time-sensitive and if you care about observing them you probably don't want to miss any!

I'm happy to remove the conflate() for the lifecyle.events() binding. But since these events are not emitted in real-time as they occur but rather just being replayed, I don't see many benefits in using FlowBinding for this.

Again happy to do it as end of the day the library shouldn't care too much about how it's used.

svenjacobs commented 4 years ago

Thanks for the explanation! That makes sense. But as you said lifecycle events are different than UI events so I would be glad if you remove conflate() from this binding.

But since these events are not emitted in real-time as they occur but rather just being replayed, I don't see many benefits in using FlowBinding for this.

Could you please elaborate on this? What do you mean by "not real-time, rather replayed"? If you mean that these events do not occur at the same time the related callback methods are called (onCreate(), onStart(), etc) this does not matter for my use case. It should be a difference of nano- or milliseconds, right?

ychescale9 commented 4 years ago

I was referring to the initial few events which are always cached and replayed. For example if you register the a LifecycleObserver in onResume() you'll still get ON_CREATE, ON_START, ON_RESUME all at the same time (in the right order) which I'm struggling to see the use case for - why would you want to handle previous lifecycle events which have already happened?

svenjacobs commented 4 years ago

Right, I didn't have this on the radar since I'm filtering a specific event. But if your code depends on certain lifecycle states, I think it does make sense to replay previous events so that nothing is missed. Of course it does not make sense to have multiple event flows in a single Fragment. However with this behaviour, it does not make a difference whether you subscribe to the events in onCreateView() or in onResume().

I wonder if there is a Lifecycle concept that only notifies about events from the time events are observed, not replaying previous events?

ychescale9 commented 4 years ago

I wonder if there is a Lifecycle concept that only notifies about events from the time events are observed, not replaying previous events?

Not that I'm aware of 🤷‍♂. Anyway I'll remove conflate() to respect the behavior of LifecycleObserver.

ychescale9 commented 4 years ago

Released in 0.10.2.