Fix BindCommand passing incorrect parameter after assigning new ViewModel to View#4324
Fix BindCommand passing incorrect parameter after assigning new ViewModel to View#4324xackus wants to merge 4 commits intoreactiveui:mainfrom
Conversation
| { | ||
| ArgumentExceptionHelper.ThrowIfNull(vmProperty); | ||
| ArgumentExceptionHelper.ThrowIfNull(controlProperty); | ||
| ArgumentExceptionHelper.ThrowIfNull(withParameter); |
There was a problem hiding this comment.
Before this change null was rejected by withParameter.ToObservable(viewModel) anyway.
|
I found dead code that did the correct thing. I think this was regressed by #2766. |
|
Thanks for the PR. Will review over the next day or so. Won't be straight away. From the quick glance so far looks good though. |
There was a problem hiding this comment.
Pull request overview
Fixes a BindCommand inconsistency where the command parameter was being computed from the originally supplied ViewModel instead of the view’s current IViewFor.ViewModel, which shows up when reusing views and reassigning ViewModels.
Changes:
- Update
CommandBinderImplementation.BindCommand(..., Expression withParameter, ...)to derive the parameter observable viaReflection.ViewModelWhenAnyValue(...)(i.e., from the currentview.ViewModel). - Remove an internal mixin overload that previously built the parameter observable from the initially passed ViewModel.
- Add a WPF regression test and update WinForms tests to call the binder implementation directly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/ReactiveUI/Bindings/Command/CommandBinderImplementation.cs | Computes command parameter from the current view.ViewModel instead of the originally passed ViewModel. |
| src/ReactiveUI/Bindings/Command/CommandBinderImplementationMixins.cs | Removes the internal overload that created a stale parameter observable from the initially supplied ViewModel. |
| src/tests/ReactiveUI.Wpf.Tests/Wpf/WpfCommandBindingImplementationTests.cs | Adds a regression test for parameter correctness after ViewModel reassignment. |
| src/tests/ReactiveUI.WinForms.Tests/winforms/CommandBindingImplementationTests.cs | Updates tests to call BindCommand on the binder implementation instance (no longer using the removed mixin overload). |
src/tests/ReactiveUI.Wpf.Tests/Wpf/WpfCommandBindingImplementationTests.cs
Outdated
Show resolved
Hide resolved
src/tests/ReactiveUI.Wpf.Tests/Wpf/WpfCommandBindingImplementationTests.cs
Show resolved
Hide resolved
src/tests/ReactiveUI.Wpf.Tests/Wpf/WpfCommandBindingImplementationTests.cs
Outdated
Show resolved
Hide resolved
|
They @xackus if you feel up to it would appreciate it you can evaluate copilot comments. It definitely gets things wrong at times so feel free to write a comment and resolve Sometimes it has some good suggestions though. Just a public holiday here so visiting family so limited availability. |
…ng to non-default event
|
Copilot helped uncover another bug by pointing out an issue in my test. |
49d030b to
659cfab
Compare
|
There were some copies of the docs that were wrong / out of date. |
What kind of change does this PR introduce?
Fixes #3970.
What is the current behavior?
The command parameter is derived from the original ViewModel passed into
BindCommand.What is the new behavior?
The command parameter is derived from the current ViewModel (
IViewFor.ViewModel).What might this PR break?
Theoretically this could break code that relies on the stale command parameter value.
Please check if the PR fulfills these requirements
Other information: