Open michael-hawker opened 4 years ago
Carrying forward discussion from https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3571#issuecomment-738972627 - We probably want a specific document as well for WPF to call out this reasoning/explanation for the sample and what the recommended practice is to achieve harmony and performance.
FYI @thomasclaudiushuber
Thanks @michael-hawker. I looked through the existing samples on the different branches, and I just write down some thoughts here, so that we don't forget about it. I also include @Sergio0694 here as he is the initial sample creator, but he might read this anyway. :)
For WPF, we have an issue/discussion here regarding CommandManager, which is WPF specific: https://github.com/windows-toolkit/WindowsCommunityToolkit/issues/3571
Right now, the samples don't use the NotifyCanExecuteChanged method of the command implementations, as there is no use case in the sample that needs to call that method (@Sergio0694 please correct me if I'm wrong, I haven't found one). We need to think about a use case that requires to call that method to enable/disable a Button, because that method call is what is necessary without WPF's CommandManager. Or in other words: Calling that method shows devs who used CommandManager in the past what they have to do to invalidate their commands.
A typical use case for calling NotifyCanExecuteChanged could be a SaveCommand for a Save Button on a data entry form:
In WPF with CommandManager, all this would work automatically, and when it doesn't work automatically, developers would call CommandManager.InvalidateRequerySuggested().
So, I think the samples should show how to call NotifyCanExecuteChanged on a command to refresh a button in the UI. And it must be at a place where a call to NotifyCanExecuteChanged is necessary.
If you do it also in WPF with NotifyCanExecuteChanged (instead of CommandManager), your ViewModels will work on all platforms, which means you can move them to a .NET Standard library (which is already done in the Uno sample).
For the WPF sample, we could show in addition to that NotifyCanExecuteChanged option how to use the MVVM framework with CommandManager. That part would mean that the ViewModels become WPF-specific and can't live in a .NET Standard library anymore. I wouldn't recommend to use CommandManager, but maybe there are developers who want to use it, as they know it from MVVMLight. So, we could have a WPF sample that shows what you need to do to have CommandManager support (Implementing your own ICommand with CommandManager support, maybe we find a great way how the MVVM framework Commands can be extended that allows this).
@thomasclaudiushuber That's correct, I haven't used that in the sample app so far.
I think this issue is highlighting yet another way the ICommand
support for WPF is inferior compared to UWP/WinUI. There, you often don't even need to call NotifyCanExecuteChanged
manually at all - the binding for the command parameter will automatically call CanExecute
on the command whenever the parameter is changed.
To make an example, consider this simple setup:
Here you don't need to ever call NotifyCanExecuteChanged
at all - the binding to CommandParameter
takes care of everything for you, and it'll also be pretty efficient, as the CanExecute
property is only even re-evaluated when the input binding is changed (and not just whenever the user interacts with the app at all or does any sort of input on the window).
Without support for this from the framework (which imho should be part of any framework that claims to fully support MVVM), users would manually have to invoke NotifyCanExecuteChanged
whenever the target property changes, yes. In theory they could just add a handler to the same property the command is binding to, and manually call that method in the view when that happens. If we wanted to make life easier for developers, for instance I imagine this could be doable with some sort of attached property that binds to the same input parameter and the target command, and invokes the method whenever the parameter changes. As you said though, that'd require a WPF specific extension 🤔
Or alternatively, users could mirror the property in their viewmodels, binding to that two way, and then call NotifyCanExecuteChanged
from there whenever the value changes. Though this would probably be even more verbose.
@Sergio0694 Thanks for your quick reply. :)
I think the whole CommandManager stuff is not about CommandParameter, it's about Commands in general. I think the CommandParameter stuff like shown in your sample snippet ties the selection logic to the UI, you wouldn't be able to Unit Test the selection and the SearchCommand, as you have done the selection logic in XAML via the CommandParameter property bound directly to the other UI control, in this case ComboControl.
Imo a better way would be to make a SelectedItem property in the ViewModel, as this would allow unit testing of selection and SearchCommand. The ComboBox would look like this:
<ComboBox x:Name="ComboControl"
ItemsSource="{x:Bind ViewModel.Items}"
SelectedItem="{x:Bind ViewModel.SelectedItem,Mode=TwoWay}"/>
Then the Button would look just like below without CommandParameter property set:
<Button
Grid.Row="1"
Content="CLICK ME!"
Command="{x:Bind ViewModel.SearchCommand}"/>
Now you have the logic in the ViewModel that the SearchCommand should only be enabled when the SelectedItem property of the ViewModel is true, and you have all that logic there, which is great. When the SelectedItem property of the ViewModel is set, you have to raise the SearchCommand's CanExecute changed event. I think this does also not work in UWP nor in any other XAML framework without raising the CanExecuteChanged event on the SearchCommand when the ViewModel's SelectedItem property is changed, it's not just in WPF the case.
And that's now the point where WPF's CommandManager would come in. It has imo nothing to do with the CommandParameters.
Update: Maybe what I described here with the SelectedItem property in the ViewModel is also what you meant with your last sentence:
Or alternatively, users could mirror the property in their viewmodels, binding to that two way, and then call NotifyCanExecuteChanged from there whenever the value changes. Though this would probably be even more verbose.
Maybe what I described here with the SelectedItem property in the ViewModel is also what you meant with your last sentence
@thomasclaudiushuber Yup, that's what I meant there 🙂
I didn't use that as the first example as that might be a bit verbose and overkill in some simpler scenarios. I agree that especially in more complex apps also with a set of unit tests for viewmodels etc. that might actually be much better. In that case users would have to manually do something like this in their SelectedItem
property then:
private string selectedItem;
public string SelectedItem
{
get => selectedItem;
set
{
if (SetProperty(ref selectedItem, value))
SearchCommand.NotifyCanExecuteChanged();
}
}
If I understand this correctly, this would make it work on WPF as well, without the need for platform specific extensions?
Yup, that's what I meant there 🙂
@Sergio0694 That's good. Now we know we're on the same page. Sorry, didn't get that last sentence when I read it the first time. But the problem was not the sentence, the problem was me. :-)
If I understand this correctly, this would make it work on WPF as well, without the need for platform specific extensions?
Yes, exactly.
This is also the way how I did it in all WPF apps in the past decade.
WPF CommandManager
Now, if you use an ICommand implementation that forwards its CanExecute
event to the WPF-CommandManager
's RequerySuggested
event, the setter of the SelectedItem
property could look like below, without the SearchCommand.NotifyCanExecuteChanged();
call:
private string selectedItem;
public string SelectedItem
{
get => selectedItem;
set => SetProperty(ref selectedItem, value);
}
So, with CommandManager this would work. But the CommandManager has drawbacks:
CommandManager
fires RequerySuggested
on all kind of Input events => all CanExecute
methods of your Commands are called, not the most performant way, especially important in bigger applications CommandManager.RequerySuggested
event can cause memory leaksCommandManager
is WPF specific.I think ViewModels should be independent from the used XAML framework, so I'm not a fan of CommandManager
.
But we could think about a smart way of how developers could extend the existing ICommand implementations of the MVVM framework, so that they could hook up CommandManager if they want. I think this is what could be shown in a WPF sample.
So, there are two things I have on my mind
a) show MVVM "the proper/recommended way" (= the way without CommandManager) for WPF like we discussed it here with calling NotifyCanExecuteChanged()
in the setter
b) show maybe in a little standalone sample how to hook up WPF's CommandManager
, so that those devs who want to use it know how they can use it.
I wrote a WPF app using the community toolkit MVVM functionality. Source code is at https://github.com/markolbert/GeoProcessor.
If it helps anyone in the meantime I have a sample app using this for WPF here. https://github.com/SimonGeering/AdminAssistant/tree/dev Please note that it is very early in development at the time of writing this.