fix: windows-common ViewModelViewHost creating the old view again whe… #2447
Conversation
…n switching to a new view model (#2258) The way it was written before, a change of either ViewModelProperty or ViewContractObservableProperty resulted in both firing their corresponding Observables. Because of CombineLatest this resulted in the old ViewModel being run through ResolveViewForViewModel again, when the ViewModelProperty was changed.
| var contractChanged = _updateViewModel.Select(_ => ViewContractObservable).Switch(); | ||
| var viewModelChanged = _updateViewModel.Select(_ => ViewModel); | ||
| // Observable that fires when ViewModel or ViewContractObservable change, or ViewContractObservable fires | ||
| var vmAndContract = Observable.CombineLatest( |
There was a problem hiding this comment.
This is going to cause memory leaks unfortunately due to the way dependency properties work. Eg WhenAnyValue will call into the static manager, and it'll hold an instance of the class until you dispose the WhenAnyValue.
This is the reason why the SomethingChanged exists since it doesn't suffer from this problem.
Happy to take a solution that keeps the SomethingChanged approach.
There was a problem hiding this comment.
Did not know that could be an issue. Is that not a problem in https://github.com/reactiveui/ReactiveUI/blob/main/src/ReactiveUI.XamForms/ViewModelViewHost.cs as well then?
There was a problem hiding this comment.
Xamarin Forms is different since they have BindableProperty invoke a INotifyPropertyChanged so have a non-memory leaking way of getting alerted of property changes.
glennawatson
left a comment
glennawatson
left a comment
There was a problem hiding this comment.
Will just need the requested changes done.
|
Also in note regards to integrating the Xamarin Forms/Windows version, we can't do the xamarin forms version on windows due to the fact Xamarin Forms will use INotifyPropertyChanged approach for alerting you when BindableProperty's change. |
…n switching to a new view model (#2258) Reverted my first pull request, and added a new subject for ViewContract to handle that separately
|
Thanks for the quick update. Looks heaps better thanks. |
|
So, I reverted my first commit, and simply added a new Subject for ViewContract changes and separate PropertyChangedCallbacks. |
|
The build error is due to a recent change in Splat, so will fix it. Thanks. |
The ViewContractObservable was previously set before the CombineLatest operator. That also means value change callback on ViewContractObservableProperty fires before CombineLatest. This meant, if we never set ViewContract afterwards, CombineLatest would never emit a value, since _updateViewContract would notify of a change before anyone was listening. This commit fixes that by simply moving the ViewContractObservable setup to the end. Alternatively, we could also make _updateViewContract into a ReplaySubject(1).
|
After doing some tests with my project, I found a subtle bug changing ViewContract change notification caused. |
|
I'll do a release by the end of the week coming. Thanks for getting this in and testing it for me. Just want to get some events bugs fixed first before the release. You can use the GitHub package nuget server if you need it sooner though. |
|
You're welcome. Also, thank you for being so responsive. |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
3 participants
fix: windows-common ViewModelViewHost creating the old view again when switching to a new view model (#2258)
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
fixes #2258
windows-common ViewModelViewHost creates the old view again when switching to a new view model:
The way it was written before, a change of either ViewModelProperty or ViewContractObservableProperty resulted in both firing their corresponding Observables. Because of CombineLatest this resulted in the old ViewModel being run through ResolveViewForViewModel again, when the ViewModelProperty was changed. This created the old View for a moment again, which is a waste of resources, and it results in animation flickering (and other issues with DI lifetime management which I use).
What is the new behavior?
Now each Dependency property only fires their own Observable, instead of both at the same time.
The implementation has been mostly copied from XamForms ViewModelViewHost
What might this PR break?
It's a simple change, that has been taken from another part of ReactiveUI, so I find it unlikely to break anything.
Please check if the PR fulfills these requirements
I cannot run the ReactiveUI.Tests, because I am missing build tools for all the multitude of test targets. I do not think this class even has any tests anyhow.
I would not know, where to look for that. Besides, this bug fix makes ViewModelViewHost ViewModelProperty follow expected behavior, so no documentation changes should be necessary.
Other information:
I suggest ReactiveUI's team to do an audit of the 3 ViewModelViewHost implementations, with the goal of putting shared code in a common class. Currently, there is a lot of repetition and a showcase, how to write the same thing in 3 different ways (ViewLocator part of the code being the biggest offender).