Skip to content
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

Merged
merged 2 commits into from
Jan 8, 2018

Conversation

mmallozzi
Copy link
Contributor

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.

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2018

CLA assistant check
All committers have signed the CLA.

@ZacSweers ZacSweers self-requested a review January 4, 2018 00:22
Copy link
Collaborator

@ZacSweers ZacSweers left a 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.");
Copy link
Collaborator

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
Copy link
Collaborator

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()
Copy link
Collaborator

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)
Copy link
Collaborator

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>()
Copy link
Collaborator

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>() }

Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perf <3

@ZacSweers ZacSweers merged commit 9317294 into uber:master Jan 8, 2018
@mmallozzi mmallozzi deleted the viewholder_lifecycle branch January 8, 2018 18:34
@ZacSweers ZacSweers mentioned this pull request Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants