-
Notifications
You must be signed in to change notification settings - Fork 498
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
Added parentFragmentViewModel() #247
Conversation
): Lazy<VM> where T : Fragment, T : MvRxView = lifecycleAwareLazy(this) { | ||
requireNotNull(parentFragment) { "There is no parent fragment!" } | ||
val factory = MvRxFactory { error("No ViewModel for this Fragment.") } | ||
var fragment: Fragment? = 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.
Should it be var fragment: Fragment? = parentFragment
? or it will always throw "There is no parent fragment!"
in the first iteration.
/** | ||
* Gets or creates a ViewModel scoped to a parent fragment. This delegate will walk up the parentFragment hierarchy | ||
* until it finds a Fragment that can provide the correct ViewModel. If no parent fragments can provide the ViewModel, | ||
* a new one will be created in the direct parent of the curent Fragment. |
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.
It doesn't feel quite right that an existing viewmodel can exist anywhere in the parent fragment hierarchy, but creating a new one also puts it in the direct parent.
What if instead by default it creates it in the top level parent fragment? That seems less likely to cause issues
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.
@elihart @BenSchwab and I chatted about this yesterday with the same concern. Do you think the top would be a more sane default than the first parent?
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.
Yeah, feels safer since any other child fragment is guaranteed to be able to reach the same viewmodel.
This could be important in an edge case like process recreation where we are starting with a nested child fragment that wouldn't normally be created first
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.
@elihart would that be the case for restoration? The ViewModel won't be created until ON_CREATE. Would the child onCreate be called before the parent? That seems impossible because onAttach is called before onCreate.
09157a9
to
6191938
Compare
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.
@gpeal lgtm, just a question about the target fragment case
} | ||
|
||
// ViewModel was not found. Create a new one in the top-most parent. | ||
var topParentFragment = parentFragment |
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.
won't fragment
already be the top parent fragment at this point? since the above while loop only breaks when there is no parent fragment?
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.
seems like fragment
would be null
, but if seems like we could eliminate this loop if we track parentFragment in the above loop
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.
@BenSchwab @elihart Yes, it would be null. I wrote it that way but the code was harder to read despite being shorter. Writing it this way made the intention of each section more explicit and easier to grok.
} | ||
|
||
/** | ||
* Gets or creates a ViewModel scoped to a target fragment. Throws [IllegalStateException] if there is no target fragment. |
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.
this use case seems weird to me, because the lifecycle of the target fragment isn't necessarily tied to the fragment that is calling this. Couldn't the target fragment be destroyed, and the viewmodel cleared, while this fragment is still using it?
is it that bad to share viewmodels via an activity scoped viewmodel in this case?
I'm not particularly opposed to it, but am curious to hear more about the need. Once we have these new delegates it would be great if we updated the wiki with a little guide about when we recommend using each
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.
@elihart I think it alleviates some of the potential ambiguity in the parentFragment case because it allows you to be explicit what the ViewModel will be scoped to.
I haven't actually needed it myself but @littleGnAl suggested it and it seems reasonable and was simple enough.
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.
@gpeal @elihart In our project, the targetFragment
is generally used with DialogFragment
, but MvRx does not officially support DialogFragment
, I think this is not necessary.
If someone needs to get the ViewModel
of the targetFragment
, they can explicitly cast the targetFragment
such as (targetFragment as MyTargetFragment).myTargetViewModel
.
But if this function is needed, I think the logic should be similar to the logic of parentFragmentViewModel
, because the targetFragment
's ViewModel
maybe already exist.
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.
@littleGnAI, ah, the DialogFragment case makes sense to me
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.
Nice, thanks for adding this!
try { | ||
return@lifecycleAwareLazy ViewModelProviders.of(fragment, factory).get(key, viewModelClass.java) | ||
.apply { subscribe(this@parentFragmentViewModel, subscriber = { postInvalidate() }) } | ||
} catch (e: IllegalStateException) { |
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.
maybe we should throw our custom exception in the factory -- IllegalStateException
might be caused by something else during VM creation, and this would swallow it
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.
@BenSchwab I did this by matching the exception name now. Otherwise, we'd have to expose a new type since this is an inline function
} | ||
|
||
// ViewModel was not found. Create a new one in the top-most parent. | ||
var topParentFragment = parentFragment |
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.
seems like fragment
would be null
, but if seems like we could eliminate this loop if we track parentFragment in the above loop
Fixes #227