Windowscommunitytoolkit: MVVM Toolkit - WPF CommandManager RelayCommand `canExecute` Issue

[michael-hawker]

If you migrate a WPF application and swap the using Galasoft.MvvmLight.CommandWpf; to using Microsoft.Toolkit.Mvvm.Input; all your commands using canExecute will stop working due to the specific handling of CommandManager missing in Windows Community Toolkit MVVM framework!

_Originally posted by @laurentkempe in https://github.com/windows-toolkit/MVVM-Samples/pull/14#r523771777_

[msftbot[bot]]

Hello michael-hawker, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[michael-hawker]

@Sergio0694 @jamesmcroft opening this here for tracking from the comment @laurentkempe raised in the sample repo and on Twitter: https://twitter.com/laurentkempe/status/1327707975532810243

@sonnemaf I know you wrote a WPF sample a while back here: https://github.com/sonnemaf/ToolkitMvvmDemo - Is this something you encountered as well or did you not have the same setup for this scenario?

@deanchalk I know you may have done some early poking with WPF as well?

[sonnemaf]

My WPF sample doesn't use the CommandManager. It's something I always try to avoid. When I used the MvvmLight framework (long time ago) I only used the Galasoft.MvvmLight.Command not the Galasoft.MvvmLight.CommandWpf. Maybe because I had a Silverlight & UWP background. I never liked the WPF implementation.

Can't help you with this one, sorry.

[laurentkempe]

Maybe because I had a Silverlight & UWP background. I never liked the WPF implementation.

Yeah, this problem is only a WPF one!

[michael-hawker]

@sonnemaf I saw here that you called the NotifyCanExecuteChanged directly:

vm.RaiseSalaryCommand.NotifyCanExecuteChanged();
vm.DeleteCommand.NotifyCanExecuteChanged();

Was this the workaround you did to get them to be enabled at the start of the application? In your scenario, you're just never dynamically changing their state of being able to be executed, eh?

[michael-hawker]

Seems like an issue was a start to this type of discussion in the WPF repo here: https://github.com/dotnet/wpf/issues/434

Could be worth filing a separate issue over there to formalize some sort of proposal specifically for a change to how CommandManager in WPF works to improve this experience to be more inline with how it's been simplified in UWP?

@dotMorten I know you've looked at the WPF codebase a bit. Do you have any good tips or know of good starting points for anyone who wanted to investigate this?

[dotMorten]

@michael-hawker Sorry no I haven't dug into that part of WPF. But my blogpost here might be helpful: https://xaml.dev/post/Compiling-and-debugging-WPF

[sonnemaf]

@michael-hawker I call the NotifyCanExecuteChanged from the ListViewEmployees_SelectionChanged events in WPF. It is not only about the Start of the app. It is necessary to detect ListView selection changes. The CommandParameter changes do not trigger the NotifyCanExecuteChanged in WPF.

private void ListViewEmployees_SelectionChanged(object sender, SelectionChangedEventArgs e) {
    var vm = MainViewModel.Current;
    vm.RaiseSalaryCommand.NotifyCanExecuteChanged();
    vm.DeleteCommand.NotifyCanExecuteChanged();
}

The CommandParameter of the RaiseSalary and Delete buttons are databound to the SelectedItem of the ListBox.

<StackPanel Orientation="Horizontal">
    <Button Margin="4,0"
            Padding="4"
            Command="{Binding RaiseSalaryCommand, Mode=OneTime}"
            CommandParameter="{Binding ElementName=listViewEmployees, Path=SelectedItem, Mode=OneWay}"
            Content="Raise Salary" />
    <Button Command="{Binding DeleteCommand, Mode=OneTime}"
            CommandParameter="{Binding ElementName=listViewEmployees, Path=SelectedItem, Mode=OneWay}"
            Content="Delete" />
</StackPanel>

The Commands of my ViewModel have a CanExecute which is dependent of the passed Employee parameter.

RaiseSalaryCommand = new RelayCommand<Employee>(OnRaiseSalary, emp => emp is object && emp.Salary < 5500);
DeleteCommand = new AsyncRelayCommand<Employee>(OnDelete, emp => emp is object);

In UWP the CanExecute is re-evaluated everytime the CommandParameter of a Button changes. WPF doesn't do this. So I had call the NotifyCanExecuteChanged manually. This is off course ugly. So I just updated my sample project. Introduced an NotifyCommandParameterChanges attached property for this. Not sure if this is the best property name though.

<StackPanel Orientation="Horizontal">
    <Button Margin="4,0"
            Padding="4"
            local:MvvmHelper.NotifyCommandParameterChanges="true"
            Command="{Binding RaiseSalaryCommand, Mode=OneTime}"
            CommandParameter="{Binding ElementName=listViewEmployees, Path=SelectedItem, Mode=OneWay}"
            Content="Raise Salary" />
    <Button local:MvvmHelper.NotifyCommandParameterChanges="true"
            Command="{Binding DeleteCommand, Mode=OneTime}"
            CommandParameter="{Binding ElementName=listViewEmployees, Path=SelectedItem, Mode=OneWay}"
            Content="Delete" />
</StackPanel>

Does this answer your question?

[thomasclaudiushuber]

This is about raising CanExecuteChanged automatically, which is done in WPF by implementing ICommand and implementing the CanExcecuteChanged event like this:

   public event EventHandler CanExecuteChanged
    {
        add { CommandManager.RequerySuggested += value; }
        remove { CommandManager.RequerySuggested -= value; }
    }

The CommandManager.RequerySuggested is fired whenever an input event happens that might invalidate commands. It's described here: https://docs.microsoft.com/en-us/dotnet/api/system.windows.input.commandmanager.requerysuggested?view=net-5.0

So, the question is: how to hook up the CanExecuteChanged event of the ICommand implementation to automatically use CommandManager.RequerySuggested behind the scenes if used in a WPF app.

I think, by default it should not be hooked up, like it is now. Because CommandManager.RequerySuggested fires all the time when you click, press keys etc., which has a performance impact in bigger applications. It's always better to fire by hand when needed from a performance point of view, but means more work for a developer, also for smaller apps, where performance might not be different.

I suggest we find a way to turn this on with a single line of code at the application level. But as the CommandManager class is only available in WPF, there needs to be some extension to call into that WPF specific code.

Sometimes the CommandManager also does not find out when to invalidate (when you change ViewModel properties in code). Then you have to call the static CommandManager.InvalidateRequerySuggested() method to invalidate all commands in your app.

I'm looking at it right now, but I'm not sure if we should really implement that in the MVVM lib. I never used CommandManager in all the WPF apps in the past, but invalidated manually. But let's see what options we have.

[michael-hawker]

Thanks for the extra insights @sonnemaf and @thomasclaudiushuber. Makes a lot more sense now.

@thomasclaudiushuber think something like @sonnemaf's helper attached property makes sense to have in one of our samples or are you thinking of something more specific than that? What have you done in the past when you "invalidated manually" in terms of the typical pattern?

[thomasclaudiushuber]

@michael-hawker I don't think the issue is about the command parameter like described by @sonnemaf, I think it's about commands in general. In UWP/Silverlight etc., you have to raise the CanExecute manually. In WPF you can use an ICommand implementation that hooks up to CommandManager.RequerySuggested to raise it automatically on user input, like shown in the snippet in my comment above.

But I also didn't use CommandManager in WPF in the past 10 years (but before I did), as I found out that raising it manually is better for performance, and attaching directly to the static CommandManager.RequerySuggested event is also something to re-consider regarding memory leaks.

In one project we implemented an ICommand class that was able to retrieve property names to subscribe to these. I think something similar is also done in Prism's ICommand implementation, but I have to look that up. That would also be a great way to say at the Command level when it has to invalidate, and not having to call RaiseCanExecuteChanged at several property setters in your ViewModel. But it would still require more code for the developer than CommandManager, which "magically" just works for most cases.

[michael-hawker]

Thanks @thomasclaudiushuber.

attaching directly to the static CommandManager.RequerySuggested event is also something to re-consider regarding memory leaks.

I wasn't sure if this was meant as a re-consider in terms of doing this or not doing this? I think based on the past discussion you're saying it's better to handle manually to avoid memory leaks compared to trying to hook into the events of the CommandManager.RequerySuggested? Or are you saying that you're thinking of re-considering now and that it may be better to leverage the CommandManager.RequerySuggested to avoid accidental memory leaks from trying to manage it manually?

If the former (avoiding CommandManager.RequerySuggested), do you think if we have a solid WPF example in our sample repo of how to best manage this manually or with some light-weight helper that'd be enough? Then we could potentially show folks how to get better performance in their apps and avoid the issue?

[thomasclaudiushuber]

@michael-hawker Sorry for my not so precise sentence. It was meant as a re-consider of not doing this.

I think based on the past discussion you're saying it's better to handle manually to avoid memory leaks compared to trying to hook into the events of the CommandManager.RequerySuggested?

Yes, exactly.

If the former (avoiding CommandManager.RequerySuggested), do you think if we have a solid WPF example in our sample repo of how to best manage this manually or with some light-weight helper that'd be enough? Then we could potentially show folks how to get better performance in their apps and avoid the issue?

I think this is a great idea, and I'm happy to help out there.

[michael-hawker]

@thomasclaudiushuber that'd be great. We haven't started an official WPF sample in our MVVM Toolkit Sample Repo.

I've actually been a bit behind on getting PRs reviewed there. ☹ We have a Uno one which is close which splits the ViewModels out to .NET Standard projects (as they should be) here: https://github.com/windows-toolkit/MVVM-Samples/pull/16 and then a Xamarin PR based off of that one: https://github.com/windows-toolkit/MVVM-Samples/pull/33

Let me start working on getting those reviewed (there's some build failures I should bubble-up), but if you wanted to get started, feel free to fork off of https://github.com/windows-toolkit/MVVM-Samples/pull/33 or just start tinkering with some thoughts in this space that we could integrate later. Whatever works best for you! 🎉