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

[ghost]

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! 🎉

[michael-hawker]

After talking to @Sergio0694 we're just going to provide some guidance in our Samples/Docs. I'm going to move that to that repo for tracking over there.

[PeskovV]

Hi everyone! I read current discussion and understood that i should call NotifyCanExecuteChanged() in setter for correct work CanExecute method (i use RelayCommand from Microsoft.Toolkit.Mvvm)

If I'm wrong, can you correct me?

[Sergio0694]

That is correct, by default RelayCommand will not automatically update its visual state on WPF due to how the interface is managed there and its tight coupling with the CommandManager class, which we couldn't fully support in the MVVM Toolkit given that it's platform agnostic, and that type is not part of the standard BCL but it's just WPF specific.

You can either manually handle that, or use an extension to enable the auto-updating of that, like this one I wrote.

The idea with that is that you'd use it to enable the auto-update for each element individually, like this:

<Button
    xmlns:input="using:Microsoft.Toolkit.Mvvm.Wpf.Input"
    Command="{Binding MyCommand}"
    input:RelayCommandExtensions.IsCommandUpdateEnabled="True"/>

Let me know if it works for you!

NOTE: that extension type is just an example I put together, it's not official and it's not part of the MVVM Toolkit. It's just provided as-is if you want to try it out, but if you have issues with it, please don't report that in the actual WCT repo 🙂

[PeskovV]

@Sergio0694 thank you very much! Your extension looks like a good way for me.

I tried to use it and it has a bug, i think. When control with command and extension is initializing, condition of 97 line is false (because obj doesn't have a Command)

        /// <summary>
        /// Toggles the <see cref="IsCommandUpdateEnabled"/> logic for a target <see cref="DependencyObject"/> instance.
        /// </summary>
        /// <param name="obj">The target <see cref="DependencyObject"/> to toggle the property for.</param>
        private static void ToggleIsCommandUpdateEnabled(DependencyObject obj)
        {
            // Before doing anything, ensure previous handlers are removed
            if (handlers.TryGetValue(obj, out var data))
            {
                _ = handlers.Remove(obj);

                CommandManager.RequerySuggested -= data.CommandManagerHandler;
                data.Command.CanExecuteChanged -= data.CommandHandler;
            }

            // Register the new handlers, if requested
            if (((ICommandSource)obj).Command is IRelayCommand command &&
                GetIsCommandUpdateEnabled(obj))
            {
                object dummy = new();

                EventHandler
                    commandManagerHandler = (_, _) => CommandManager_RequerySuggested(command, dummy),
                    commandHandler = (_, _) => Command_CanExecuteChanged(dummy);

                handlers.Add(obj, new SubscriptionData(command, dummy, commandManagerHandler, commandHandler));

                CommandManager.RequerySuggested += commandManagerHandler;
                command.CanExecuteChanged += commandHandler;
            }
        }

[Sergio0694]

Yeah that can often happen, pretty sure I had added some code to account for that though? 🤔 I'm referring to this code here (lines 68 to 76):

// Ensure a notifier is enabled so that we can monitor for changes to the command
// property in the future. This is also needed in case this attached property is
// set before the binding engine has actually resolved the target command instance.
if (!notifiers.TryGetValue(obj, out _))
{
    PropertyChangeNotifier notifier = new(obj, nameof(ButtonBase.Command));

    notifier.PropertyChanged += (_, _) => ToggleIsCommandUpdateEnabled(obj);
}

So even though you were seeing the property being false there, it should automatically update it when the Command binding is updated by the frameword. I do remember testing this and it seemed to be working fine for me. Is this actually not working at all in your application? If that's the case I could see if I can find my original sample and try it out again I guess, let me know!

[PeskovV]

You're right and i agree with you, but after creating command

DownloadCommand = new AsyncRelayCommand(DownloadCommandExecute, DownloadCommandCanExecute);

private IAsyncRelayCommand _downloadCommand;
public IAsyncRelayCommand DownloadCommand
{
    get => _downloadCommand;
    set
    {
        _downloadCommand = value;
        OnPropertyChanged(nameof(DownloadCommand));
    }
}

event handler ToggleIsCommandUpdateEnabled doesn't work :-(

I'm trying to figure out what's happend

[Sergio0694]

Potentially unrelated, but why is your command raising a notification? Ideally it'd just be an init-only property (or a get-only autoproperty) that's just set in the constructor. There's really no reason to make the whole property observable like that if you're never going to change its value anyway (which is generally the case) 🙂

[PeskovV]

I agree) I test why ToggleIsCommandUpdateEnabled doesn't work and made this property observable In generally case i use command like this:

SaveCommand = new RelayCommand(SaveCommandExecute, SaveCommandCanExecute); // in constructor

public IRelayCommand SaveCommand { get; private set; }

[Sergio0694]

You can ditch that private set, it's not needed in that case 🙂 Get-only autoproperty will already be settable from a constructor.

Anyway I'll see if I can repro the issue in the next few days 👍 In the meantime there's also that other variant of that class that Fons shared here: https://github.com/windows-toolkit/MVVM-Samples/issues/41#issuecomment-737070043. You can also try using that one and see if that works as expected for you (the implementation is different so it should be fine) 😄

[PeskovV]

@Sergio0694 if i create boolean property in view model public bool Test => true; and then make binding utils:RelayCommandExtensions.IsCommandUpdateEnabled="{Binding Test}", i get working version But utils:RelayCommandExtensions.IsCommandUpdateEnabled="True" doesn't work 😄

[PeskovV]

@Sergio0694 Hi! I'm sorry for the inconvenience Have you reproduced issue? You said about it in this comment: https://github.com/windows-toolkit/MVVM-Samples/issues/41#issuecomment-808928952

[cjmurph]

@Sergio0694 After hooking up the property changed event on the new notifier, it needs to be added to the notifiers collection or it will be collected and disposed so when the command property is updated, it won't fire ToggleIsCommandUpdateEnabled

        // Ensure a notifier is enabled so that we can monitor for changes to the command
        // property in the future. This is also needed in case this attached property is
        // set before the binding engine has actually resolved the target command instance.
        if (!notifiers.TryGetValue(obj, out _))
        {
            PropertyChangeNotifier notifier = new(obj, nameof(ICommandSource.Command));

            notifier.PropertyChanged += (_, _) => ToggleIsCommandUpdateEnabled(obj);

            notifiers.Add(obj, notifier);
        }

[IngoBleile]

Will there be a fix available soon?

[Sergio0694]

@IngoBleile what are you referring to? If you mean a fix for this extension, as I mentioned that's just a gist I put together as a proof of concept, you're free to take it and modify it with the suggestions above to make it work for you. If you meant a fix to make this all work out of the box on WPF, that's not planned due to the MVVM Toolkit being cross platform by design 🙂

[IngoBleile]

@Sergio0694 Yes, I meant to make it work for WPF out of the box. Sorry, I missed the point of being cross platform! :)

[Sergio0694]

No worries! 🙂 And yeah, we can't do that because that'd require the use of CommandManager, which is WPF-specific unfortunately.

[jamiehankins]

This is a surprise to me. I tried using your RelayCommand objects and all of my commands with predicates stopped working.

Having to manually trigger checks is a no go when I have conditions like this:

public bool HasCompletedOrders =>
    PersistenceService
        .OrderRepository
        .FindAll()
        .Any(o => o.IsCompleted)
    ||
    Orders
        .Any(o => o.OrderCompleted);

Yes, I know that looks heavy for a command predicate, but it's at the main menu when nothing else is happening, and it works fine when triggered by CommandManager.RequerySuggested.

I would like to remove all of my personally coded MVVM stuff and only use the Community Toolkit, but I rely on these change notifications. My app runs on modern hardware, so I'm not worried about having to run a few extra CanExecute checks.

This is how my implementation handles it:

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

Maybe what I'll do is keep a collection of IRelayCommand objects in my base class, subscribe to CommandManager.RequerySuggested, and pass on the notifications. My base class is already IDisposable, so there won't be a concern about leaking references.

I'd really like to use the source generators. I guess that means I'd need to use reflection since I wouldn't be able to know the IRelayCommand objects otherwise.

[will-scc]

I've come across this issue, and I understand I need to call NotifyCanExecuteChanged manually for each command in the relevant property setters, but I just want to double check I've understood fully before I commit to working on these changes.

This is how the commands are declared:

///Uses PropertyChanged.Fody to inject NotifyPropertyChanged
public string SomeValue { get; set; }

private ICommand addCmd;

public ICommand AddCmd => this.addCmd ?? (this.addCmd = new RelayCommand(() =>
{
    //Do some stuff
}, this.AddRecordCmd_CanExecute));

private bool AddCmd_CanExecute()
{
    return this.SomeValue == "Foo";
}

Since NotifyCanExecuteChanged isn't a part of ICommand interface I need to change my ICommand to RelayCommand and also convert my auto property to a full property so that I can call NotifyCanExecuteChanged(), like this:

///Uses PropertyChanged.Fody to inject NotifyPropertyChanged
private string someValue;

public string SomeValue
{ 
    get => this.someValue;
    set
    {
        this.someValue= value;
        this.AddCmd.NotifyCanExecuteChanged();
    }
}

private ICommand addCmd;

public ICommand AddCmd => this.addCmd ?? (this.addCmd = new RelayCommand(() =>
{
    //Do some stuff
}, this.AddRecordCmd_CanExecute));

private bool AddCmd_CanExecute()
{
    return this.SomeValue == "Foo";
}

Is this correct?

Changing from ICommand to RelayCommand is easy enough, it's having to update all the properties used in CanExecute methods that's unpleasant with how many commands there are and each one having to be done by hand (rather than a nice easy Replace job).

Side note: it's disappointing that the Migrating from Mvvmlight article doesn't mention this breaking change at all. :(

[pikausp]

No worries! 🙂 And yeah, we can't do that because that'd require the use of CommandManager, which is WPF-specific unfortunately.

Hello @Sergio0694 , you guys are obviously free to handle things how you see fit, but would you say MAUI, for example, is not cross platform due to it allowing you to tweak platform specific things? Personally I would say that having support for command re-query for WPF is something that many developers have use for and it takes away from the added benefit of the library. If we were talking about some obscure feature I would understand, but this seems as sort of a core functionality on one of the platforms.

Just a food for thought :)

[doxxx]

Another alternative solution is to call NotifyCanExecuteChanged on IRelayCommands when CommandManager.RequerySuggested is raised:

CommandManager.RequerySuggested += (s, e) =>
{
    foreach (var command in GetType().GetProperties()
        .Where(p => p.PropertyType.IsAssignableTo(typeof(IRelayCommand)))
        .Select(p => (IRelayCommand?)p.GetValue(this)))
    {
        command?.NotifyCanExecuteChanged();
    }
};

You'd have to do this on each type that exposes IRelayCommand properties, but you could wrap it up as a utility method that you just call from the constructor after the properties are initialized.

Beware when creating a utility method for this because RequerySuggested only keeps a weak reference to the handler, so unless you keep a strong reference to each object's handler elsewhere, your handler will mysteriously not work. I did it like so:

public static class RelayCommandHelpers
{
    private static List<EventHandler> handlers = new();

    public static void RegisterCommandsWithCommandManager(object container)
    {
        void Handler(object? s, EventArgs e)
        {
            foreach (var command in container.GetType().GetProperties()
                .Where(p => p.PropertyType.IsAssignableTo(typeof(IRelayCommand)))
                .Select(p => (IRelayCommand?)p.GetValue(container)))
            {
                command?.NotifyCanExecuteChanged();
            }
        }

        // We have to retain a reference to the handler because RequerySuggested
        // only keeps a weak reference to it.
        handlers.Add(Handler);

        CommandManager.RequerySuggested += Handler;

    }
}

Edit2: A more performant implementation which assumes that your IRelayCommand properties don't change:

public static class RelayCommandHelpers
{
    private static readonly List<WeakReference<IRelayCommand>> _commands = new();

    public static void RegisterCommandsWithCommandManager(object container)
    {
        if (_commands.Count == 0)
            CommandManager.RequerySuggested += OnRequerySuggested;

        foreach (var p in container.GetType().GetProperties())
        {
            if (p.PropertyType.IsAssignableTo(typeof(IRelayCommand)))
            {
                var command = (IRelayCommand?)p.GetValue(container);
                if (command != null)
                    _commands.Add(new WeakReference<IRelayCommand>(command));
            }
        }
    }

    private static void OnRequerySuggested(object? sender, EventArgs args)
    {
        foreach (var command in _commands)
        {
            if (command.TryGetTarget(out var c))
                c.NotifyCanExecuteChanged();
        }
    }
}

Edit3: Use weak references for the IRelayCommands to avoid memory leaks in case views are being created dynamically.

[doxxx]

I created a gist for this helper: https://gist.github.com/doxxx/911ba2af9847362e12e01a045273184e