Why is CanExecute called after removing the command source from the user interface?

Question

I am trying to understand why CanExecute is called in a command source that has been removed from the user interface. Here is a simplified demo program:

<Window x:Class="WpfApplication1.MainWindow" xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" Height="350" Width="525"> <StackPanel> <ListBox ItemsSource="{Binding Items}"> <ListBox.ItemTemplate> <DataTemplate> <StackPanel> <Button Content="{Binding Txt}" Command="{Binding Act}" /> </StackPanel> </DataTemplate> </ListBox.ItemTemplate> </ListBox> <Button Content="Remove first item" Click="Button_Click" /> </StackPanel> </Window> 

Code for:

 public partial class MainWindow : Window { public class Foo { static int _seq = 0; int _txt = _seq++; RelayCommand _act; public bool Removed = false; public string Txt { get { return _txt.ToString(); } } public ICommand Act { get { if (_act == null) { _act = new RelayCommand( param => { }, param => { if (Removed) Console.WriteLine("Why is this happening?"); return true; }); } return _act; } } } public ObservableCollection<Foo> Items { get; set; } public MainWindow() { Items = new ObservableCollection<Foo>(); Items.Add(new Foo()); Items.Add(new Foo()); Items.CollectionChanged += new NotifyCollectionChangedEventHandler(Items_CollectionChanged); DataContext = this; InitializeComponent(); } void Items_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e) { if (e.Action == NotifyCollectionChangedAction.Remove) foreach (Foo foo in e.OldItems) { foo.Removed = true; Console.WriteLine("Removed item marked 'Removed'"); } } void Button_Click(object sender, RoutedEventArgs e) { Items.RemoveAt(0); Console.WriteLine("Item removed"); } } 

When I click the "Delete first item" button once, I get this output:

 Removed item marked 'Removed' Item removed Why is this happening? Why is this happening? 

"Why is this happening?" It is constantly printed every time I click on some empty part of the window.

Why is this happening? And what can or should be done to prevent the use of CanExecute in remote command sources?

Note: RelayCommand can be found here .

Answers to Michael Edenfield's questions:

Q1: Callstack when CanExecute is called on a remote button:

WpfApplication1.exe! WpfApplication1.MainWindow.Foo.get_Act.AnonymousMethod__1 (object parameter) Line 30 WpfApplication1.exe! WpfApplication1.RelayCommand.CanExecute (object parameter) String 41 + 0x1a bytes PresentationFramework.dll! MS.Internal.Commands.CommandHelpers.CanExecuteCommandSource (System.Windows.Input.ICommandSource commandSource) + 0x8a bytes PresentationFramework.dll! System.Windows.Controls.Primitives.ButtonBase.UpdateCanExecute () + 0x18 bytes PresentationFramework.dll! System.Windows.Controls.Primitives.ButtonBase.OnCanExecuteChanged (object sender, System.EventArgs e) + 0x5 bytes PresentationCore.dll! System.Windows.Input.CommandManager.CallWeakReferenceHandlers (System.Collections.Generic.List handlers) + 0xac bytes PresentationCore.dll! System.Windows.Input.CommandManager.RaiseRequerySposed (obj object) + 0xf bytes

Q2: In addition, this happens if you remove all the buttons from the list (and not just the first?)

Yes.

Answer 1

The problem is that the command source (that is, the button) does not unsubscribe from the CanExecuteChanged command to which it is attached, so when CommandManager.RequerySuggested is triggered, CanExecute fires long after the command source has failed.

To solve this problem, I implemented IDisposable on RelayCommand and added the necessary code so that whenever the model object is deleted and therefore removed from the user interface, Dispose () is called on all its RelayCommand .

This is a modified RelayCommand (original here ):

 public class RelayCommand : ICommand, IDisposable { #region Fields List<EventHandler> _canExecuteSubscribers = new List<EventHandler>(); readonly Action<object> _execute; readonly Predicate<object> _canExecute; #endregion // Fields #region Constructors public RelayCommand(Action<object> execute) : this(execute, null) { } public RelayCommand(Action<object> execute, Predicate<object> canExecute) { if (execute == null) throw new ArgumentNullException("execute"); _execute = execute; _canExecute = canExecute; } #endregion // Constructors #region ICommand [DebuggerStepThrough] public bool CanExecute(object parameter) { return _canExecute == null ? true : _canExecute(parameter); } public event EventHandler CanExecuteChanged { add { CommandManager.RequerySuggested += value; _canExecuteSubscribers.Add(value); } remove { CommandManager.RequerySuggested -= value; _canExecuteSubscribers.Remove(value); } } public void Execute(object parameter) { _execute(parameter); } #endregion // ICommand #region IDisposable public void Dispose() { _canExecuteSubscribers.ForEach(h => CanExecuteChanged -= h); _canExecuteSubscribers.Clear(); } #endregion // IDisposable } 

Wherever I use this, I keep track of all the instantiated RelayCommands created, so I can call Dispose() when the time comes:

 Dictionary<string, RelayCommand> _relayCommands = new Dictionary<string, RelayCommand>(); public ICommand SomeCmd { get { RelayCommand command; string commandName = "SomeCmd"; if (_relayCommands.TryGetValue(commandName, out command)) return command; command = new RelayCommand( param => {}, param => true); return _relayCommands[commandName] = command; } } void Dispose() { foreach (string commandName in _relayCommands.Keys) _relayCommands[commandName].Dispose(); _relayCommands.Clear(); } 

Answer 2

There is a known issue with using lambda expressions and events that you fire. I hesitate to call it a “mistake” because I don’t understand the internal details enough to know if this is the intended behavior, but it certainly seems intriguing to me.

The key indicator here is this part of the call stack:

 PresentationCore.dll!System.Windows.Input.CommandManager.CallWeakReferenceHandlers( System.Collections.Generic.List handlers) + 0xac bytes 

Weak events are a way to connect events that do not support a live object; it is used here because you pass the lamba expression as an event handler, so the "object" containing this method is an anonymous object created internally. The problem is that the object passed to the add handler for your event is not the same instance of the expression as the one that is passed to the remove event, it is just a functionally identical object, so it is not an unsubscribe from your event.

There are several workarounds as described in the following questions:

Weak event handler model for use with lambdas

UnHooking Events with Lambdas in C #

Can using lambdas as event handlers cause a memory leak?

For your case, the easiest way is to move the CanExecute and Execute code to real methods:

 if (_act == null) { _act = new RelayCommand(this.DoCommand, this.CanDoCommand); } private void DoCommand(object parameter) { } private bool CanDoCommand(object parameter) { if (Removed) Console.WriteLine("Why is this happening?"); return true; } 

Alternatively, if you can organize your object to create Action<> and Func<> delegates from lambdas once, save them in variables and use them when creating RelayCommand , this will force you to use the same instance, IMO, for your case, probably more complicated than it should be.