_canExecuteChanged = new WeakCanExecuteChanged(this) in AsyncCommand

[sjb-sjb]

I created an Mvvm.AsyncCommand and bound it as the Command of a button. I got a null pointer exception in CanExecuteChanged_add, the _canExecuteChanged variable is null.

I am not sure how to use the class. I can set the CanExecuteChanged property to assign a WeakReference-based handler. However I don't see how to trigger CanExecute=false during execaution of the command. If I call OnCanExecuteChanged before or after the command executes then it will not accomplish the objective of setting CanExcecute=false during the execution.

The class itself does not trigger CanExecuteChanged as part of Execute, so it does not set CanExecute=false during Execute.

Looking at the derived class, the use of "new" for OnCanExecuteChanged is striking. While there are some situations where "new" is a necessary design element, in most cases it should be avoided. This is because it means that a different method will be called if a derived pointer is used vs if a base pointer to the very same object is used. That behavior is not what one usually expects -- normally there is only one version of a given method for a given object.

Steve

PS I find it confusing is that there is another AsyncCommand, with a very different implementation, in StephenCleary / AsyncEx. It would be a lot easier if each class were in just one package.

Having said that, thanks for some fantastic code!!

[StephenCleary]

Thank you for opening this issue! This bug in AsyncCommand has now been fixed (as of 1.0.0-beta-2).

The normal usage of AsyncCommand is to only provide an "execution" delegate (the first constructor parameter), and not a "canExecute" delegate (the second constructor parameter). In this situation, the AsyncCommand will return false from CanExecute while the canExecute delegate is executing. I believe this is the behavior you're looking for.

I agree, the new is highly unusual, and it may change before release. The intent in this case is to have the base class provide a protected method to its derived classes, and the AsyncCommand wants to expose that method publicly. So new is more of a workaround because override can't expand visibility.

AsyncCommand has quite a history. It started in an MSDN article a couple years ago (with 3 or so differing implementations) and then moved into my AsyncEx library (another implementation). With the .NET Core library, I revisited the APIs to a lot of my AsyncEx types, and this one was high on my list for refactoring; the one in this library is the newest, and I think it's the cleanest (factoring out CancelCommand was key).

[sjb-sjb]

Those are interesting comments. Since you say that factoring out CancelCommand was key, I guess you have already been where I was planning to go, which is a cancellation command attached to the AsyncCommand.

I think there is a trade off between having a simple class and having one that provides a lot of service. Factoring out CancelCommand does make the library simple but doesn't it also reduce the service level? The user must now create the cancel command, tie it to the AsyncCommand, and notify property changed for the CancelCommand variable in the ViewModel. I would like that all done for me instead of having to do it myself.

A similar situation occurs for progress notification. I would like the command class to give my routine a progress notification tool that is bindable by the ui. The messenger approach requires that I write more code to receive the message and notify a property that I can then bind to, or else that I write code behind in my view to receive the message and update the ui. But the service level that I want is an progress report object created for me, that I can update directly from within my command execution and which I can also bind to directly in my UI.

[StephenCleary]

I had a version of AsyncCommand (never published, IIRC) that handled: asynchronous commands, cancellation commands, and progress reports. It got way to complex, especially since "disable when executing", "support cancellation", and "support progress reporting" are all optional. When I got too confused trying to use my own type, yeah... :)

[sjb-sjb]

I've given this topic some more thought. I feel that the CanExecute / CanExecuteChanged design is showing its age. What we should do is have CanExecuteIf( source, propertyPath) which binds to a boolean pity.

[StephenCleary]

I never cared for the CanExecute/CanExecuteChanged design. It always felt awkward, not the least because CanExecuteChanged cannot be implemented by a .NET event. It was a horrible design from day 1, but we're stuck with it for backcompat reasons.

[sjb-sjb]

Here is what I am doing:

    /// <summary>

    /// Creates a binding to a boolean property which indicates a condition required to hold in order for the command to 

    /// be enabled. Use this method to restrict execution of the command so that it cannot always be executed. 

    /// Use of this method is optional. 

    /// Two other conditions also need to be satisfied in order for the command to be enabled: 

    /// (1) if CanExecuteWhileExecuting = false, then the command must not already be executing; and (2) if a list 

    /// of excluding commands has been set by a call to AreMutuallyExclusive, then it must be that none of them

    /// is already executing. 

    /// </summary>

    public void CanExecuteOnlyIf<TSource>( TSource source, Expression<Func<TSource,bool>> propertyPath)

When the binding updates then we can call OnCanExecuteChanged, and the value set by the binding is accessed by CanExecute.

To complete the transition, just hide OnCanExecuteChanged and put a note saying that CanExecute is an infrastructure function not intended for end users.