2
Vote

Changeset 60539 breaks DelegateCommand's RaiseCanExecuteChanged() notification

description

The attached repro solution for Silverlight 4 implements a simple Navigation scheme with two Frame elements that allow navigation through three Page controls each.
Navigation works through Buttons (Back/Forward/Next) whose Commands are bound to DelegateCommand<T> instances that call into the NavigationService's GoForward()/GoBack() methods and CanGoForward/CanGoBack properties.
Page navigation steps lead to an update of the Commands' CanExecute Property via RaiseCanExecuteChanged() in the Page.OnNavigated() methods.
 
When built against MEFedMVVM.SL build 60280 it is possible to navigate back and forth for as long as you like.
Using MEFedMVVM.SL build 60539 instead leads to the Forward and Back buttons getting permanently disabled (or permanently enabled) after a couple of steps back and forth (at least next/next/next/back/forward/back, behavior is not entirely deterministic).
The reason for this appears to be that the DelegateCommand<T> instances lose the entries in their _canExecuteChangedHandlers collections. As a result the navigation Buttons do not get their CanExecute status pdated by RaiseCanExecuteChanged() calls any more.

file attachments

comments

marlongrech wrote Oct 14, 2010 at 8:10 AM

DelegateCommand (previous version) had a memory leak, now it uses WeakReferences... probably this is causing it... I will have a look at the project you posted... So sorry i am answering so late, I know its horrible but I am squashed... sorry again

DrJ wrote Oct 15, 2010 at 7:42 AM

No problem man, I know what you're talking about.
I'd try to dig up the root of this problem myself if I had more time - and if I had a clue about WeakReferences :-)

DrJ wrote Oct 20, 2010 at 9:02 AM

Findings of further analysis:

The DelegateCommand<T> implementation from changeset 60539 cannot actually work when used with Silverlight Buttons, because the code inside ButtonBase.OnCommandChanged does not keep the required hard reference to its EventHandler (see below).
This means that all Buttons attached to a WeakReference-based DelegateCommand<T> via a standard XAML binding can be expected to eventually lose their Command association when the EventArgs instance is garbage collected.

Marlon, this needs fixing otherwise DelegateCommand<T> is useless.

/ System.Windows.Controls.Primitives.ButtonBase /
private void OnCommandChanged(DependencyPropertyChangedEventArgs e)
{
ICommand oldValue = e.OldValue as ICommand;
ICommand newValue = e.NewValue as ICommand;
if (oldValue != null)
{
    oldValue.CanExecuteChanged -= new EventHandler(this.CanExecuteChanged);
}
if (newValue != null)
{
    newValue.CanExecuteChanged += new EventHandler(this.CanExecuteChanged);
}
this.UpdateCanExecute();
}

DrJ wrote Oct 20, 2010 at 9:14 AM

BTW: This is different from WPF's ButtonBase which does keep a hard link to its EventHandler before passing it to an attached command.
So the approach from changeset 60539 will work on WPF but not on Silverlight 4.
If you look into the latest Prism 4 drop for Silverlight 4 you'll find that its DelegateCommand does not implement the WeakReference pattern, either.

wrote Oct 20, 2010 at 1:21 PM

wrote Feb 28, 2011 at 2:35 PM

marlongrech wrote Sep 10, 2011 at 12:15 PM

is this still the case with the latest version? I could not reproduce

wrote Feb 14, 2013 at 6:47 PM