Closed humblehacker closed 1 year ago
Hi, thanks for reporting this! While the current crash is fixable it will just be replaced with another crash since such a setup would try to wrap the KMMViewModel multiple times. Will have to take a deeper look at this to see if we can support such cases.
Thanks for looking into this! Do you think this is something you'll be working on soon? I'm not trying to rush you. Rather I need to plan how much time I should spend looking for my own workaround.
I was actually just looking at this 😁. So the real problem is that the ObservableViewModel
(which is the ObservableObject
wrapping the KMMViewModel
) is being deallocated.
I have update the logic to provide a better error message for this, but that won't fix the problem you are facing.
We need something to keep a strong reference to the ObservableViewModel
.
Currently RootViewModel
is holding a strong reference to ChildViewModel
.
When isExpanded
is first set to true
your ChildViewModel
will be wrapped in an ObservableViewModel
because of the @ObservedViewModel
property wrapper.
But once isExpanded
is set to false
the reference to this ObservableViewModel
is lost.
I don't think there is a way to keep a strong reference in KMM-ViewModel since ObservableViewModel
keeps a strong reference to the KMMViewModel
already, which would result in a circular reference preventing both objects from being deallocated.
I guess our best option would be to try and remove the restriction where only a single ObservableViewModel
can be used. Let me see if that is possible.
I noticed the same thing, and changing the associated object from ASSIGN to RETAIN avoided the crash but replaced it with a leak.
Looking forward to what you come up with - let me know if I can help in any way.
So removing the restriction on a single ObservableViewModel
per KMMViewModel
isn't going to work.
The deallocation of the ObservableViewModel
is used to cleanup the KMMViewModel
(it calls onCleared
and cancels the CoroutineScope
).
Does the childViewModel
value ever change in your real project? I am assuming it is since it is a StateFlow
.
If so, when does it change? And does that somehow relate to the isExpanded
state?
I mean something like the following should solve the issue, but I am not sure if that fits your requirements:
open class RootViewModel : KMMViewModel() {
@NativeCoroutinesState
val childViewModel: MutableStateFlow<ChildViewModel?> = MutableStateFlow(viewModelScope, ChildViewModel("Child"))
fun toggleChildViewModel() {
childViewModel.value = if (childViewModel.value == null) ChildViewModel("Child") else null
}
}
struct RootScreen: View {
@StateViewModel var viewModel = RootViewModel()
var body: some View {
HStack {
Button(
action: { viewModel.toggleChildViewModel() },
label: { Image(systemName: viewModel.childViewMode != nil ? "chevron.right" : "chevron.left") }
)
if let childViewModel = viewModel.childViewModel {
ChildView(viewModel: childViewModel)
}
Spacer()
}
.padding()
}
}
Yeah, we need to keep the instance of child view model around. This example is a very simplistic version of the code where we discovered the issue. We use nested view models extensively in our code, with parent view models sometimes holding lists of child view models -- all of which need to retain their state until the parent is disposed of.
Alright, how about only hiding the body
of the ChildView
instead of the whole view?
That would keep the @ObservedViewModel
property in the view hierarchy (including the strong reference to the ObservableViewModel
.
I think it would be better if we didn't have to apply special case logic when using KMMViewModel's analog property wrappers -- that is, hopefully @ObservedViewModel
would be a drop-in replacement for @ObservedObject
.
I did some poking around in your source and came up with something that works, but I'm not sure if there might be some unintended consequences of such a change. If you find this to be a workable solution, let me know and I'll clean up the PR.
PR #40
FYI support for child view models has been added in v1.0.0-ALPHA-10.
I have a
KMMViewModel
subclass that contains a child view model that's also aKMMViewModel
subclass. The child view model is passed to a child view that is collapsible. I can collapse the view fine, but the app crashes when I try to expand it again.Relevant stack trace excerpt
Kotlin view models:
Swift Views
Sample project to demonstrate
https://github.com/humblehacker/KMMViewModelCrashExample
steps:
>
to collapse the viewComments
It looks like when the view is collapsed, the child view model is destroyed. So when it's expanded again it crashes when we try to set the associated object on a dangling pointer.
Is there a better way to model this kind of relationship that might avoid this problem?