11

I am making some custom ICommand implementation of my own and I see A LOT of implementations going like this:

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

protected void RaiseCanExecuteChanged()
{        
    CommandManager.InvalidateRequerySuggested();
}

As far as I can see this is poorly optimized code since calling RaiseCanExecuteChanged() will trigger ALL commands in the UI to check their ICommand.CanExecute status, when usually we just want one of them to verify it.

I think I read once this is the main code of some WPF ICommands like RoutedCommand, and for them it makes sense because they want to revalidate all ICommands automatically once some control loses focus and things like this, but still I don't understand why people repeat this pattern for their own ICommand implementations.

The code I have in mind is a simple event invocation such as:

public event EventHandler CanExecuteChanged;

protected void RaiseCanExecuteChanged()
{        
    CanExecuteChanged?.Invoke(this, EventArgs.Empty);
}

I tested and this works, so Why all the examples on the web don't implement something as simple as this? Am I missing something?

I have read about memory leak problems using strong references in regular events, where the CommandManager only uses WeakReferences, which is good in case the View is Garbage Collected, but still, aren't there any solutions that won't compromise performance over memory footprint?

Michel Feinstein
  • 10,189
  • 11
  • 68
  • 144
  • http://stackoverflow.com/questions/2281566/is-josh-smiths-implementation-of-the-relaycommand-flawed – ASh Nov 14 '16 at 11:12

4 Answers4

6

Why all the examples on the web don't implement something as simple as this? Am I missing something?

I'm guessing it's mostly due to laziness... What you propose is indeed a better (more efficient) implementation. However, it's not complete: you still need to subscribe to CommandManager.RequerySuggested to raise CanExecuteChanged on the command.

Thomas Levesque
  • 270,447
  • 59
  • 580
  • 726
  • Why? I tested this and it works... Which case scenarios am I missing? – Michel Feinstein Nov 14 '16 at 10:28
  • @mFeinstein, "it works" in the cases you're tested, but calling `CommandManager.InvalidateRequerySuggested()` won't raise the `CanExecuteChanged` on your command. – Thomas Levesque Nov 14 '16 at 17:32
  • @mFeinstein, but it should. That's the whole point of calling `CommandManager.InvalidateRequerySuggested()`: to ensure the `CanExecute()` of all commands is re-evaluated. You're right that the command itself shouldn't call it, but other code can (and does) call it. – Thomas Levesque Nov 14 '16 at 17:46
  • I thought "if I raise the CanExecuteChanged" so the command will look for its "CanExecute" status... But not that this is the only way of making the command reevaluate it. I thought the CommandManager holds a reference to the command and it will reevaluate, but not necessarily with the event bring triggered. – Michel Feinstein Nov 14 '16 at 17:48
  • "I thought the CommandManager holds a reference to the command": no, it doesn't; it only has a (weak) reference to the event handler if you subscribe to the `RequerySuggested ` event. – Thomas Levesque Nov 14 '16 at 23:06
  • "it will reevaluate, but not necessarily with the event bring triggered": how, then? If the command is bound to a control, the control needs to know that it needs to update its enabled/disabled state. It does so by subscribing to the `CanExecuteChanged` event. – Thomas Levesque Nov 14 '16 at 23:07
  • Well, yes, that's the obvious answer, but I am not familiar with the inner WPF engine, there could be a lot more to it, specially on the CommandManager side of things. I usually try to not make any assumptions before I know more about a system. – Michel Feinstein Nov 14 '16 at 23:09
  • Ideally, a command should both react to `RequerySuggested`, and expose a `RaiseCanExecuteChanged` method. This way it can be refreshed globally or individually. – Thomas Levesque Nov 14 '16 at 23:10
  • So `RaiseCanExecuteChanged` should be implemented with a `WeakReference` to avoid memory leaks and be more performant than `CommandManager.InvalidateRequerySuggested()`, right? – Michel Feinstein Nov 14 '16 at 23:13
  • @mFeinstein `CommandManager` uses a weak reference because it's long-lived (static event), but it's not the case for an individual command, so it doesn't need to use a weak reference. – Thomas Levesque Nov 15 '16 at 08:56
  • 3
    For what it's worth, here's my typical implementation: https://gist.github.com/thomaslevesque/13c3e69f6c5ae219b945fc07b75843fc – Thomas Levesque Nov 15 '16 at 08:57
  • Well, if a viewmodel triggers some UI reconstruction the strong reference could be an issue I guess and something easy to forget and not pay the proper attention. – Michel Feinstein Nov 15 '16 at 14:28
  • Your code looks just like mine, but I didn't made the CommandManager part, good job! – Michel Feinstein Nov 15 '16 at 14:30
  • I was just looking your code a second time...shouldn't you unregister from the `RequerySuggested` event on the Command's destruction? – Michel Feinstein Nov 19 '16 at 23:25
  • 1
    @mFeinstein it doesn't really matter, because `RequerySuggested` only holds a weak reference. – Thomas Levesque Nov 20 '16 at 01:20
  • Oh, yeah, hahah I forgot and it's the whole point of the question hahaha – Michel Feinstein Nov 20 '16 at 01:24
4

Very simply - if you are performing heavy work in ICommand.CanExecute() then you are using Commands very badly. If you follow that rule there should in fact be no serious performance implication to calling CommandManager.InvalidateRequerySuggested().

Pragmatically, it's a much easier implementation than what you've suggested.

Personally, I rather call CommandManager.InvalidateRequerySuggested() in a particular ViewModel when a property changes so that the feedback to the user is instantaneous (i.e. enabling a button as soon as a form is completed/valid).

toadflakz
  • 7,314
  • 1
  • 21
  • 38
  • I am sorry, but I don't understand your point, how a 5 line solution can be "programmatically easier" than a 3 line solution? – Michel Feinstein Nov 14 '16 at 10:34
  • It's simpler because you don't have to have each change call `RaiseCanExecuteChanged` on every `Command` that is affected by that property. – toadflakz Nov 14 '16 at 11:34
  • 2
    In that sense it is, but IMHO it's flawed in its concept. RaiseCanExecuteChanged I believe is for the ViewModel to be able to notify that particular Command, and only him, that his CanExecute state changed. CommandManager is the one to notify all. I can't imagine any programming case where activating all objects in the purpose of activating one is considered good practice. It's like yelling at the street so everyone will open the door, instead of ringing at the door bell of the door you want.... But still, I understand the simplicity IF you kept your CanExecute simple and has few commands. – Michel Feinstein Nov 14 '16 at 15:53
  • I can. Try the text/paragraph formatting bar of a Word processing application. If your document and formatting command host are completely separate `ViewModel` classes, you would use `CommandManager.InvalidateRequerySuggested` because there is no way you would want to have dependencies to multiple `ViewModel`/`Command` classes and have to manually maintain those relationships. – toadflakz Nov 14 '16 at 16:08
  • 1
    In that case I think you would want a special kind of command that behaves like this, not that all commands should be like this. – Michel Feinstein Nov 14 '16 at 17:28
1

This question is quite old, it's 2019 now, but I have found another reason to use CommandManager.InvalidateRequerySuggested().

I have written my own custom ICommand class for a WPF application, in which I invoked CanExecuteChanged directly in the first place like this.

public void RaiseCanExecuteChanged()
{
    CanExecuteChanged?.Invoke(this, null); 
}

My WPF application makes heavy use of different threads and when the above method is called from another thread then the main UI thread, it throws no error, it is just kind of ignored. It was getting even worse, when I found out, that all code lines inside the calling method were skipped, which led to strange results.

I don't know exactly, but I guess the reason was, that CanExecuteChanged led to changes in my UI, which must not be changed from another thread.

However - the moment when I changed my ICommand to CommandManager.InvalidateRequerySuggested(), there was no more problem. It seems that CommandManager.InvalidateRequerySuggested() can be called from any thread and the UI still gets updated.

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

public void RaiseCanExecuteChanged()
{
    CommandManager.InvalidateRequerySuggested();
}

I thought this could be a valueable answer, because I debugged this issue for 3 hours, before I came to this solution. The problem finding this issue was, that there were no errors thrown while debugging. The code was just skipped. Very strange behaviour.

Michael
  • 876
  • 1
  • 10
  • 26
1

This is an answer to this answer. It is true that the CanExecuteChanged?.Invoke(this, null); has to be called by the main UI thread.

Simply write it like follows:

public void RaiseCanExecuteChanged()
{
    Application.Current.Dispatcher.Invoke(() => CanExecuteChanged?.Invoke(this, null)); 
}

This solves your problem and you can requery just one command. It is however true that you should nevertheless make your CanExecute-Method as fast as possible, as it will anyways be periodically executed. It is best to have CanExecute just consist of a single return foo; where foo is a field you can set right before you call CommandManager.InvalidateRequerySuggested();.

jonzbonz
  • 61
  • 3