-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Introduced LifecycleExtensions overloads to set observation schedulers #179
Introduced LifecycleExtensions overloads to set observation schedulers #179
Conversation
mvicore-android/src/main/java/com/badoo/mvicore/android/lifecycle/LifecycleSchedulerScope.kt
Fixed
Show fixed
Hide fixed
e0917ec
to
8d1905d
Compare
fun Lifecycle.startStop(f: Binder.() -> Unit) { | ||
Binder(StartStopBinderLifecycle(this)).apply(f) | ||
} | ||
|
||
fun Lifecycle.startStop(schedulers: LifecycleBinderSchedulers, f: LifecycleSchedulerScope.() -> Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the API a little bit verbose, especially when we do not need to use it most of the time. Can we remove LifecycleBinderSchedulers
and make API more lightweight?
lifecycle.createDestroy(scheduler) {
bind(a to b using c) // implicit .observeOn scheduler
}
lifecycle.createDestroy {
bind(a to b using c observeOn scheduler) // explicit
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createDestroy(scheduler)
and createDestroy(lifecycleBinderSchedulers)
have one issue – when you start applying them to the existing code, you might change subscription order, leading to some unexpected bugs. So it might be helpful to consider only infix observeOn
usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing stops developers using observeOn
in the version that does not take a scheduler. (It's actually already in this PR).
I am hoping that we WILL use this 'verbose' api as it sets out a strict guideline, and is less boilerplate in case you want to set a schedule in a lot of places.
I could also change it so that you can also call bind outside of main
/background
and remove unbound
so it could look like
testLifecycleOwner.lifecycle.createDestroy(schedulers) {
mainThread {
bind(events to uiConsumer)
}
backgroundThread {
bind(events to analyticsConsumer)
}
bind(events to dontSwitchThreadsConsumer)
bind(events to dontSwitchThreadsConsumer observeOn mainThreadScheduler)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regards to unexpected bugs, developers would need to be aware of this change and take that into account if/when migrating. At the moment there is no way to ensure that everything is happening on the main/background threads anyway, so it's likely already an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like that I need to create a separate LifecycleBinderSchedulers
class instead of injecting schedulers directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this in my 3rd attempt at the API.
testLifecycleOwner.lifecycle.createDestroy {
withScheduler(mainScheduler) {
bind(events to uiConsumer)
}
withScheduler(backgroundScheduler) {
bind(events to analyticsConsumer)
}
bind(events to dontSwitchThreadsConsumer)
bind(events to dontSwitchThreadsConsumer observeOn mainThreadScheduler)
}
74b8d6a
to
3a10e05
Compare
f6fddd4
to
c16bb4c
Compare
c16bb4c
to
0ac913a
Compare
Description
Proposal for allowing developers to set observation scheduler for Binder and lifecycle extensions.
Check list
CHANGELOG.md
if required.