-
Notifications
You must be signed in to change notification settings - Fork 494
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
Fix subscriptionLifecycleOwner to use viewLifecycleOwner in Fragment's onCreateView #533
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.
@@ -211,6 +212,16 @@ interface MvRxView : MavericksView { | |||
).toDisposable() | |||
} | |||
|
|||
private fun Job.toDisposable() = Disposables.fromAction { | |||
cancel() | |||
private class JobDisposable(job: Job) : AtomicReference<Job>(job), Disposable { |
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.
could you add a unit test for this? 🙏
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.
Added a few simple unit tests here: 8e00379.
@@ -70,7 +70,11 @@ interface MavericksView : LifecycleOwner { | |||
* By default [subscriptionLifecycleOwner] is the same as the MvRxView's standard lifecycle owner. | |||
*/ | |||
val subscriptionLifecycleOwner: LifecycleOwner | |||
get() = (this as? Fragment)?.viewLifecycleOwnerLiveData?.value ?: this | |||
get() = try { | |||
(this as? Fragment)?.viewLifecycleOwner ?: this |
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.
could you add a comment explaining that this is necessary for the view lifecycle to be accessible in onCreateView?
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 realized the old doc comment was still suggesting an incorrect fragment implementation so I rephrased it overall and included the how/why of the new implementation: 98afd3f
Tried to still keep it brief there, let me know what you think.
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 great, thanks!
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.
Thanks for the improvements!
@@ -70,7 +70,11 @@ interface MavericksView : LifecycleOwner { | |||
* By default [subscriptionLifecycleOwner] is the same as the MvRxView's standard lifecycle owner. | |||
*/ | |||
val subscriptionLifecycleOwner: LifecycleOwner | |||
get() = (this as? Fragment)?.viewLifecycleOwnerLiveData?.value ?: this | |||
get() = try { | |||
(this as? Fragment)?.viewLifecycleOwner ?: this |
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 great, thanks!
As discussed in #530,
subscriptionLifecycleowner
wasn't using a fragment'sviewLifecycleOwner
for subscriptions made duringonCreateView
. The previous implementation usingviewLifecycleOwnerLiveData
doesn't work there due to the LiveData's value remainingnull
until the view has been returned. New solution usesviewLifecycleOwner
directly when available - exception for accessing too early is caught.Also included is a new wrapper for
Job
inmvrx-rxjava2
to better reflect the status of the underlying job in the returnedDisposable
'sisDisposed()
.