Refining RaiseCanExecuteChanged implementation

[Laurent Bugnion]

Hey guys,

 

I am having a couple of issues with my RelayCommand implementation in WPF. This is a modified version of Josh Smith’s original version which is quite well known. However there is a slightly annoying issue which I am trying to solve, in relation with the CanExecuteChanged event.

 

The well-known implementation is to use the CommandManager in WPF to raise the RequerySuggested event directly. This is a suitable implementation for RoutedCommands, however some other commands may be used for non-UI related tasks, which causes an issue. The well-known implementation in WPF looks like this:

 

public event EventHandler CanExecuteChanged

{

    add

    {

        if (_canExecute != null)

        {

            CommandManager.RequerySuggested += value;

        }

    }

    remove

    {

        if (_canExecute != null)

        {

            CommandManager.RequerySuggested -= value;

        }

    }

}

public void RaiseCanExecuteChanged()

{

    CommandManager.InvalidateRequerySuggested();

}

 

In Silverlight, we don’t have the CommandManager so instead, we raise the CanExecuteChanged event manually:

 

public event EventHandler CanExecuteChanged;

public void RaiseCanExecuteChanged()

{

    var handler = CanExecuteChanged;

    if (handler != null)

    {

        handler(this, EventArgs.Empty);

    }

}

 

The WPF implementation has the advantage to call CanExecute automagically when the user interacts with the UI. I am not very fond of this actually (I prefer explicit actions rather than magic, and also the CanExecute method gets called a LOT of times) but many people rely on this feature in WPF. However if anyone else subscribes to the CanExecuteChanged event, they will not be notified because the event is directly wired to the CommandManager (only).

 

In consequence, I am thinking of the following implementation that combines both the WPF and SL implementations. However I am not 100% sure about it. Won’t it cause the CanExecute method to be executed too often (even more often than what the CommandManager does)? Thoughts?

 

private EventHandler _canExecuteChanged;

public event EventHandler CanExecuteChanged

{

    add

    {

        if (_canExecute != null)

        {

            CommandManager.RequerySuggested += value;

            _canExecuteChanged = value;

        }

    }

    remove

    {

        if (_canExecute != null)

        {

            CommandManager.RequerySuggested -= value;

            _canExecuteChanged = null;

        }

    }

}

public void RaiseCanExecuteChanged()

{

    CommandManager.InvalidateRequerySuggested();

    var handler = _canExecuteChanged;

    if (handler != null)

    {

        handler(this, EventArgs.Empty);

    }

}

 

Cheers,

Laurent

[Josh Smith]

Laurent,

This is something that bothered me since I wrote RelayCommand years ago. There are two reasons why I delegated the CanExecuteChanged to CommandManager.

1) RelayCommands would be evaluated for execution availability at the same time as "built-in" commands.

2) There was (is?) a bug in WPF where the CanExecuteChanged event of a command was hooked but never unhooked by standard ICommandSources, causing memory leaks. Back then, according to folks in the know at MSFT, the CommandManager was implemented in such a way (via weak eventing, I believe) that the bug was avoided.

In hindsight, I don't know if reason #1 was significant enough to relinquish control to CommandManager. I'm not sure if #2 was ever fixed.

Josh

[Brian Noyes]

As far as I know #2 is still there, in fact verified with the team about a year ago when we were working on Prism 4. As I understand it, a WPF Binding will take a strong ref to both the command object itself and to the delegate object exposed for the CanExecuteChanged handling. Silverlight does not do the second part. As a result, we have a precompiler #if in Prism to use a weak ref in WPF but not in Silverlight.

On Sunday, October 23, 2011, Josh Smith <flappl...@gmail.com> wrote:
> Laurent,
> This is something that bothered me since I wrote RelayCommand years ago. There are two reasons why I delegated the CanExecuteChanged to CommandManager.
> 1) RelayCommands would be evaluated for execution availability at the same time as "built-in" commands.
> 2) There was (is?) a bug in WPF where the CanExecuteChanged event of a command was hooked but never unhooked by standard ICommandSources, causing memory leaks. Back then, according to folks in the know at MSFT, the CommandManager was implemented in such a way (via weak eventing, I believe) that the bug was avoided.
> In hindsight, I don't know if reason #1 was significant enough to relinquish control to CommandManager. I'm not sure if #2 was ever fixed.
> Josh
>
> On Sun, Oct 23, 2011 at 12:59 PM, Laurent Bugnion <lau...@galasoft.ch> wrote:
>
> Hey guys,
>
>  
>
> I am having a couple of issues with my RelayCommand implementation in WPF. This is a modified version of Josh Smith’s original version which is quite well known. However there is a slightly annoying issue which I am trying to solve, in relation with the CanExecuteChanged event.
>
>  
>
> The well-known implementation is to use the CommandManager in WPF to raise the RequerySuggested event directly. This is a suitable implementation for RoutedCommands, however some other commands may be used for non-UI related tasks, which causes an issue. The well-known implementation in WPF looks like this:
>
>  
>
> public event EventHandler CanExecuteChanged
>
> {
>
>     add
>
>     {
>
>         if (_canExecute != null)
>
>         {
>
>             CommandManager.RequerySuggested += value;
>
>         }
>
>     }
>
>     remove
>
>     {
>
>         if (_canExecute != null)
>
>         {
>
>             

--
-----------------------------------------
Brian Noyes
Chief Architect, IDesign Inc
Microsoft Regional Director / MVP
http://www.idesign.net
+1 703-447-3712
-----------------------------------------

[Philipp Sumi]

Hi Laurent

Alternatively, you could just delegate the actual event subscription strategy to a dedicated component of your DelegateCommand. Like this, you could have either auto-wired commands via CommandManager, manual events, or another custom strategy you could inject into your commands. Depending on how you create your API, this is not too intrusive, as you may rely on a default strategy for your commands (see snippet below)

Cheers,
Philipp

ps:I've been silent for a long - too long -, but I'll be hopefully back to blogging soon. I've been doing mostly NDA work in 2010/2011, but am finding myself playing with UI tech again after quite a while. Yay!

using System;
using System.Windows.Input;
 
namespace WpfApplication1
{
    public class DelegateCommand<T> : ICommand
    {
        private readonly Func<T, bool> canExecuteFunc;
        private readonly Action<T> executeAction;
        private readonly ICommandRequeryStrategy requeryStrategy;
 
 
        public DelegateCommand(Action<T> executeAction, RequeryStrategy requeryStrategy = RequeryStrategy.CommandManager)
            : this(executeAction, val => true, requeryStrategy)
        {
        }
 
        public DelegateCommand(Action<T> executeAction, Func<T, bool> canExecuteFunc, RequeryStrategy requeryStrategy = RequeryStrategy.CommandManager)
        {
            this.executeAction = executeAction;
            this.canExecuteFunc = canExecuteFunc;
 
            //determine command requery strategy
            switch(requeryStrategy)
            {
                case RequeryStrategy.CommandManager:
                    this.requeryStrategy = new CommandManagerRequeryStrategy();
                    break;
                case RequeryStrategy.Manual:
                    this.requeryStrategy = new ManualRequeryStrategy(this);
                    break;
                default:
                    throw new ArgumentOutOfRangeException("requeryStrategy");
            }
        }
        
        /// <summary>
        /// Triggers the <see cref="CanExecuteChanged"/> event.
        /// </summary>
        public void RaiseCanExecuteChangedEvent()
        {
            requeryStrategy.SuggestRequery();
        }
 
        /// <summary>
        /// Fires when changes occur that affect whether
        /// or not the command should execute.
        /// </summary>
        /// <remarks>Listeners are forwarded to the active
        /// <see cref="ICommandRequeryStrategy"/>.</remarks>
        public event EventHandler CanExecuteChanged
        {
            add { requeryStrategy.AddRequeryListener(value); }
            remove { requeryStrategy.RemoveRequeryListener(value); }
        }
 
        public bool CanExecute(object parameter)
        {
            return canExecuteFunc((T)parameter);
        }
 
        public void Execute(object parameter)
        {
            executeAction((T)parameter);
        }
    }
 
    public enum RequeryStrategy
    {
        CommandManager,
        Manual
    }
 
    public interface ICommandRequeryStrategy
    {
        void AddRequeryListener(EventHandler listener);
        void RemoveRequeryListener(EventHandler listener);
        void SuggestRequery();
    }
 
    public class ManualRequeryStrategy : ICommandRequeryStrategy
    {
        private event EventHandler requerySuggested;
        private readonly ICommand sender;
 
        public ManualRequeryStrategy(ICommand sender)
        {
            this.sender = sender;
        }
 
        public void AddRequeryListener(EventHandler listener)
        {
            requerySuggested += listener;
        }
 
        public void RemoveRequeryListener(EventHandler listener)
        {
            requerySuggested -= listener;
        }
 
        public void SuggestRequery()
        {
            if (requerySuggested != null) requerySuggested(sender, EventArgs.Empty);
        }
    }
 
    public class CommandManagerRequeryStrategy : ICommandRequeryStrategy
    {
        public void AddRequeryListener(EventHandler listener)
        {
            CommandManager.RequerySuggested += listener;
        }
 
        public void RemoveRequeryListener(EventHandler listener)
        {
            CommandManager.RequerySuggested -= listener;
        }
 
        public void SuggestRequery()
        {
            CommandManager.InvalidateRequerySuggested();
        }
    }
}

[Peter O'Hanlon]

That's a pretty neat solution there mate. I like the idea of being to use a strategy to control this.

---

From: Philipp Sumi
Sent: 24 October 2011 09:04
To: wpf-di...@googlegroups.com
Subject: Re: [WPF Disciples] Refining RaiseCanExecuteChanged implementation

[Mark Smith]

This is similar to what I have in MvvmHelpers.  I track a separate internal event and allow you to turn on "auto" requery support through the CommandManager, or to deliberately raise it yourself pushing the option off to the user of the class.  I like the strategy pattern below, I don't allow you to provide your own implementation so this is quite nice.

mark

[Michael Brown]

I would say instead of passing in an enum, have the constructor take an ICommandRequeryStrategy. You get the added benefit of configuring the strategy from an IoC provider (if you choose). You don’t have to remember to add a new type to the enum and a new case to the switch/case statement. This is the Open-Closed Principle at its core. You can extend the functionality of your DelegateCommand to support a new RequeryStrategy, without modifying its internals.

[Mark Smith]

Agreed.  I didn't look close enough, I thought that's what he was doing :-) 

[Philipp Sumi]

This was just a quick snippet, but I guess if I'd put this into my repo, I'd probably provide both options. The enum has the advantage of hiding the actual API from the user and thus reducing complexity. Dependencies on the enum are restricted to that constructor though, so it wouldn't be an issue to add an overload that takes an instance a strategy implementation.

[Peter O'Hanlon]

I thought that was what he said. I can't see him violating this in reality.

---

From: Mark Smith
Sent: 24 October 2011 15:30

[Philipp Sumi]

Thanks, Pete!

BUT ACTUALLY .... ;)
...I might violate this in reality by only providing the enum option UNTIL i see the need to open the API and allow custom strategy injections. However, the whole "architecture" is prepared for hose - it only takes me a constructor overload.

However, from a production API perspective, i really don't see this being the case too soon. I've VERY rarely fired the requery events manually, but  lazily relied on CommandManager, let alone needed a third strategy, so immediately requiring users of my commands to construct commands with strategies might be a case of over-engineering. I might reconsider from a testing perspective though, as we could have mocked strategies injected into our nested commands. But even then, I'd either keep the enum overload, or delegate the whole Command creation to a builder class.

[Peter O'Hanlon]

Actually, I think providing both would be more confusing. I would stick with the interface injection if I were you.

---

From: Philipp Sumi
Sent: 24 October 2011 15:39

To: wpf-di...@googlegroups.com
Subject: Re: [WPF Disciples] Refining RaiseCanExecuteChanged implementation

This was just a quick snippet, but I guess if I'd put this into my repo, I'd probably provide both options. The enum has the advantage of hiding the actual API from the user and thus reducing complexity. Dependencies on the enum are restricted to that constructor though, so it wouldn't be an issue to add an overload that takes an instance a strategy implementation.

On Mon, Oct 24, 2011 at 4:30 PM, Mark Smith <ma...@julmar.com> wrote:

[Laurent Bugnion]

Just to get back to the original question, what do you guys think about doing both, i.e. using the CommandManager AND manually raising the event. This would do the trick of both updating the UI and also handling non-UI commands, but what would the bad side effects be (if any)? It’s kind of hard to be simpler, and given how many events the CommandManager raises anyway, one more from time to time is not such a big deal… or is it?

 

Cheers,

Laurent

[Mark Smith]

This is what I allow. It's optional, i.e. you can choose each form, but I've not had issues with it. 

Mark

Sent from my iPhone

[Brian Noyes]

As long as there is a way to easily turn it off and just do the manual command handling if that is what you want.

[Michael Brown]

You could provide a default value. I can already think of one additional Strategy. RequeryOnPropertyChangedStrategy would subscribe to INPC and fire RequerySuggested whenever a propertychanged event occurs. Less chatty than command manager but doesn’t require as much interaction from the user.

 

        public DelegateCommand(Action<T> executeAction, ICommandRequeryStrategy requeryStrategy = new CommandManagerRequeryStrategy())

[Brian Noyes]

We explicitly decided to avoid any interplay with the routed commands in Prism because of the complexities and weirdness that can result from focus interplay that I wrote about here as a follow on to my MSDN Magazine article after getting some late tech review comments from the guys on the WPF team:

http://briannoyes.net/2008/11/09/TheTruthAboutRoutedCommandsRouting.aspx

 

as well as for the excessive re-polling that happens with every focus change and repainting.

 

I think it is better to not tie into CommandManager at all. RaiseCanExecuteChanged at the appropriate times when the things that are tied to the command change. Simple as that. Yes it puts a little more burden on the developer to understand what their command enablement is coupled to, but if they are not capable of figuring that out, they are probably not qualified to be doing the job. J

 

My 2c.

Brian