-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change sample ViewHolders to implement LifecycleScopeProvider #157
Conversation
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.
Looks good. Just a few nits and looks like you'll need to sign the CLA from your github account
case BIND: | ||
return ViewHolderEvent.UNBIND; | ||
default: | ||
throw new LifecycleEndedException("Cannot use view holder lifecycle after unbind."); |
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.
nit: ViewHolder
} | ||
|
||
override fun peekLifecycle(): ViewHolderEvent? { | ||
return lifecycleEvents.value |
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.
nit: can be single body expression
override fun peekLifecycle() = lifecycleEvents.value
} | ||
} | ||
override fun lifecycle(): Observable<ViewHolderEvent> { | ||
return lifecycleEvents.hide() |
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.
nit: can be single body expression
override fun lifecycle() = lifecycleEvents.hide()
|
||
override fun onUnbind() { | ||
emitUnbindIfPresent() | ||
unbindNotifier = null | ||
lifecycleEvents.onNext(UNBIND) |
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.
can single body this and onBind above as well
|
||
private var unbindNotifier: MaybeSubject<Any>? = null | ||
private val lifecycleEvents = BehaviorSubject.create<ViewHolderEvent>() |
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.
let's make this lazy
private val lifecycleEvents by lazy { BehaviorSubject.create<ViewHolderEvent>() }
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.
Perf <3
Description:
Change AutoDisposeViewHolder from a ScopeProvider to a LifecycleScopeProvider. This better matches the underlying programming model, and provides some safety guarantees around subscription timing, as well as helping to detect certain issues that may arise from support library version bumps and changes to internal RecyclerView/ViewHolder logic.