Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncCommand #3

Open
sjb-sjb opened this issue Jul 5, 2016 · 5 comments
Open

AsyncCommand #3

sjb-sjb opened this issue Jul 5, 2016 · 5 comments
Assignees

Comments

@sjb-sjb
Copy link

sjb-sjb commented Jul 5, 2016

I have done a bit of work relating to AsyncCommand which I am hoping the community can benefit from.

I had the following objectives and approaches:

  1. To provide built-in support for cancellation, i.e. a bindable Cancel command is provided automatically as part of the AsyncCommand, if a CancellationToken is accepted by the execution delegate.
  2. To provide built-in support for progress notification. This was accomplished using the bindable ProgressReport class referenced in my other post.
  3. To allow a variety of execution delegates -- whether async or sync, and allowing any combination of CancellationToken, ProgressReport, or command parameter object as an input to the execution delegate. This was achieved by having a number of constructors which take the various possible execution delegates and convert them privately into a unified form.
  4. To be able to execute the delegate either on the current thread (await execution(...)) or on a different thread (await Task.Run(()=>execution(...))). The latter is suitable for executing CPU-bound synchronous tasks in the background. It is captured by a boolean property, RunOnDifferentThread.
  5. To move away from the CanExecute paradigm toward something more up-to-date, i.e. consistent with property notifications. This was accomplished by allowing the user to bind a boolean function (updated by property notifications) with the meaning that the command can execute only if the function is true. Naturally the method in AsyncCommand to set this function is called CanExecuteOnlyIf( boundFunction). What this does is move us from an async paradigm that requires a lot of special events and handlers to an approach based on binding.
  6. Note: The solution to this problem (5) involved creating a BoundFunction class which defines a function and binds properties to the inputs of the function. When a bound input is updated then the BoundFunction recalculates and provides a PropertyChanged notification for the result. This is a general-purpose class which is useful, for example, in view-models to combine a number of bindable inputs into a bindable output. (The old way of doing this was to use a MultiBinding, which in addition to being awkward also resulted in moving view-model concepts into the view). To support the type-safe binding of properties in code and to have the compiler check the names of property paths, an additional method Property.Path is needed to extract property paths from lambda expressions.
  7. To have a simple way of ensuring that by default a command cannot be executed if it is already executing. This was achieved by creating a boolean property CanExecuteWhileExecuting. An extension of this is to have a way to make a set of commands mutually exclusive, which is captured by calling a method, AreMutuallyExclusive, and passing in the list of mutually exclusive commands. A typical use would be to make, for example, FileNew, FileSave, FileOpen all mutually exclusive.
  8. While the resulting class is somewhat complex, it also provides a high level of functionality. The public footprint is not excessive:
    public bool CanExecuteWhileExecuting { get; set; }
    public static void AreMutuallyExclusive( IEnumerable commands)
    public void CanExecuteOnlyIf(BoundFunction boundFunction)
    public void CanExecuteOnlyIf( Tsrc source, Expression<Func<Tsrc,bool>> propertyPath)
    public NotifyTask Execution
    public bool IsExecuting { get; }
    override public async void Execute(object parameter = null)
    public async Task ExecuteAsync(object parameter)
    public bool RunOnDifferentThread { get; set; }
    public ProgressReport ProgressReport { get; }
    public ICommand CancelCommand { get; }

I am providing source code that accomplishes all this, and I am hereby entering it into the public domain for anyone to use without restriction. Please also refer to my other post discussing NotifyTask and ProgressReport.

sjb

AsyncCommand.txt
BoundFunction.txt
BoundAction.txt
Property.txt

@StephenCleary StephenCleary self-assigned this Jul 5, 2016
@StephenCleary
Copy link
Owner

Closing: Too complex to maintain.

I've gone down this path with AsyncCommand before. In my article on the subject, I go far enough to support cancellation built-in, and in my own projects I've gone further still with more "useful" options, including progress reports. The type followed a similar approach to your code.

The problem is, adding options to the AsyncCommand class quickly causes maintainability problems. For this reason, with Mvvm.Async, I've split out CancelCommand from AsyncCommand in an attempt to make the types smaller and more maintainable while retaining functionality.

A large part of this decision is my personal preference of writing small, composable classes rather than semantically complex classes controlled with flags. However, this is the third (or fourth?) iteration of AsyncCommand, and I'm not pretending it's perfect yet.

To address each of the points individually:

Cancellation is enabled by combining AsyncCommand with CancelCommand. Earlier versions of AsyncCommand did have CancelCommand built-in, but I think it's cleaner split out.

Progress is best done using the standard IProgress API.

Running on a background thread is better with an explicit call to Task.Run in the application code, rather than as a flag.

CanExecute is definitely a horrible design, in particular because some genius decided to violate the standard C# event pattern. However, it is way too late to fix it now. Introducing a different paradign is an appropriate step for an MVVM framework, but not for a library like Mvvm.Async which is intended for use with any MVVM app.

Preventing commands from concurrently executing is already supported by AsyncCommand. By default, multiple executions of the same command are not allowed, but the user may override this by specifying a custom implementation of CanExecute (and calling OnCanExecuteChanged when the return value may change).

@sjb-sjb
Copy link
Author

sjb-sjb commented Aug 18, 2016

Hi Stephen,

Thanks for your comments here and in the other post, which I will respond to separately.

I think you do have to be more careful as a framework designer than I need to be as an application developer.

I don't want to argue against your approach. Having said that, since I've put the effort into the code already I may as well make a few comments.

On cancellation, when I first looked at the Mvvm.Async version, I had not read up on cancellation generally. I looked at it and didn't immediately see how to use it in connection with AsyncCommand. Of course now I realize that it is by handing the token in to the command delegate, which is what I do in the more complex version. The point is that simpler classes can sometimes be more complex to use. Because of this, I'm actually not fully convinced that simpler classes make the total application easier to maintain. Say we have two simple classes and maybe 3 lines of code are needed to use them together, vs one complex class that has the three lines built in, or maybe it takes 5 extra lines instead of 3 to get the single-class version. Say I use it 10 times in my application, then I've got 30 lines sprinkled around my application in the "simple" version vs 5 built into the "complex" class. Saying that I could always just make a third class containing the tie between the two simple classes doesn't really answer this conundrum, because that third class would basically be equal to what the more "complex" solution is: the two 'simple' classes just being subclasses defined within the 'complex' class. Just because the end class delivered to the user is complex doesn't mean that it has to be poorly designed internally -- it should define private subclasses when that is appropriate for good design. So the question is not how much overall maintenance there is, but where does the maintenance effort lie -- in the framework or in the application code.

What matters, in my view, isn't how simple or complex the individual pieces of code are, but whether they collectively deliver the functionality that is needed, and how much work the user has to do to access that functionality. The risk in delivering larger chunks of code as the end product isn't that the solution is more complex in aggregate -- as just discussed, it isn't more complex. The risk is that if the larger chunk of code doesn't hit the target then it will be harder for the user to reconfigure it into a different delivery package. If you're comfortable that you've delivered what is needed then the larger solution is better because it delivers more integrated functionality, eliminating the need for the user to tie the smaller classes together over and over again.

About preventing commands from executing simultaneously, I wanted to be able to have mutual exclusion between different commands, such as FileOpen, FileClose, FileSave. Of course I can roll it myself by defining all those CanExecutes and calling OnCanExecuteChanged everywhere. But that doesn't lead me at all toward a simpler application, especially if I have to do it repeatedly throughout my application -- see the comments above.

On Progress and IProgress, I took another look and they seem almost as retrograde as CanCancel. Specifically, they don't really tie in to the property notification system. On a more philosophical note, I always get a "?" in my head when I see a class that seems to be all structure with no content. I'm of the view that classes should focus on delivering content; the structure of the code should be the means not the end.

All the best & happy coding
Steve

@StephenCleary
Copy link
Owner

I looked at it and didn't immediately see how to use it in connection with AsyncCommand

I am planning to write docs specifically for this, since it will definitely be a common use case.

I am also considering adding a creation method that will create an AsyncCommand/CancelCommand pair. I believe only a static method would be necessary, but a wrapper class may be easier. Note that there's a big difference in maintainability between merging CancelCommand into AsyncCommand (difficult to maintain) and keeping them separate and providing an AsyncCommand.CreateCancelableAsyncCommand() method or a CancelableAsyncCommand wrapper type (both easier to maintain).

On a side note, keeping classes smaller and more focused can increase usability. For example, if I wanted two or three AsyncCommands to be triggered independently, but have a single CancelCommand to cancel them all.

I wanted to be able to have mutual exclusion between different commands

I think that's a very interesting use case! I'll play around with your AreMutuallyExclusive approach - it'd be great to have a solution that does use CanExecute and friends (since they're standard, not because they're good), and also be sufficiently generic.

Specifically, they don't really tie in to the property notification system.

Neither of those types are specifically for MVVM apps. I do have some other IProgress<T> implementations that do support property notification, that I am planning to add to this library shortly.

@StephenCleary StephenCleary reopened this Aug 20, 2016
@sjb-sjb
Copy link
Author

sjb-sjb commented Aug 28, 2016

Well, I agree with your comment about the value of having a CancellableAsyncCommand wrapper type. I suppose I could have, or perhaps should have, designed my version of AsyncCommand so that it had two subclasses rather than one.

On the mutually exclusive commands, here is how I actually use it (and also how I use the property notifications for controlling CanExecute):

    public FileCommands()

    {

        this.ReopenCommand = new AsyncCommand( this.XX.FileReopenAsync);

        this.FileOpenCommand = new AsyncCommand( this.XX.FileOpenAsync);

        this.FileNewCommand = new AsyncCommand( this.XX.FileNewAsync);

        this.FileCloseCommand = new AsyncCommand( this.XX.FileCloseAsync);



        this.FileNewCommand.CanExecuteOnlyIf(

            BoundFunction.Create(

                this, 

                (canInitialize, fileIsOpen) => (canInitialize && !fileIsOpen),

                (@this) => @this.XX.CanFileInitialize,

                (@this) => @this.XX.FileIsOpen

            )

        );

        this.FileOpenCommand.CanExecuteOnlyIf(

            BoundFunction.Create(

               this,

                (fileIsOpen) => !fileIsOpen,

                (@this) => @this.XX.FileIsOpen

            )

        );

        this.FileCloseCommand.CanExecuteOnlyIf(

            this, (@this) => @this.XX.FileIsOpen

        );



        AsyncCommand.AreMutuallyExclusive(

            new AsyncCommand[] { ReopenCommand, FileOpenCommand, FileNewCommand, FileCloseCommand }

        );



        this.ExecutingCommand = BoundFunction.Create( 

            this,

            (reopenIsExecuting, fileOpenIsExecuting, fileNewIsExecuting, fileCloseIsExecuting) => (IAsyncCommand)(reopenIsExecuting? this.ReopenCommand: (fileNewIsExecuting? this.FileNewCommand: (fileOpenIsExecuting? this.FileOpenCommand: (fileCloseIsExecuting? this.FileCloseCommand: null)))),

            (@this) => @this.ReopenCommand.IsExecuting,

            (@this) => @this.FileOpenCommand.IsExecuting, 

            (@this) => @this.FileNewCommand.IsExecuting,

            (@this) => @this.FileCloseCommand.IsExecuting

        );



        this.ReopenCommand.Execute();

    }

From: Stephen Cleary [mailto:[email protected]]
Sent: August 20, 2016 8:14 AM
To: StephenCleary/Mvvm.Async
Cc: sjb; Author
Subject: Re: [StephenCleary/Mvvm.Async] AsyncCommand (#3)

I looked at it and didn't immediately see how to use it in connection with AsyncCommand

I am planning to write docs specifically for this, since it will definitely be a common use case.

I am also considering adding a creation method that will create an AsyncCommand/CancelCommand pair. I believe only a static method would be necessary, but a wrapper class may be easier. Note that there's a big difference in maintainability between merging CancelCommand into AsyncCommand (difficult to maintain) and keeping them separate and providing an AsyncCommand.CreateCancelableAsyncCommand() method or a CancelableAsyncCommand wrapper type (both easier to maintain).

On a side note, keeping classes smaller and more focused can increase usability. For example, if I wanted two or three AsyncCommands to be triggered independently, but have a single CancelCommand to cancel them all.

I wanted to be able to have mutual exclusion between different commands

I think that's a very interesting use case! I'll play around with your AreMutuallyExclusive approach - it'd be great to have a solution that does use CanExecute and friends (since they're standard, not because they're good), and also be sufficiently generic.

Specifically, they don't really tie in to the property notification system.

Neither of those types are specifically for MVVM apps. I do have some other IProgress implementations that do support property notification, that I am planning to add to this library shortly.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #3 (comment) , or mute the thread https://github.com/notifications/unsubscribe-auth/ASQzRslnk0ZxEfXSdGoBMzvzyAGvS4v3ks5qhu91gaJpZM4JEyZi .Image removed by sender.

@sjb-sjb
Copy link
Author

sjb-sjb commented Mar 4, 2017

Hi Stephen,

Thought I would give you an update on my use case. What I found was that I wanted two distinct types of functionality.

One, I wanted a really robust, general-purpose delegate wrapper. The constructor input is either an async or sync delegate, which it can then execute either sync or async (well, not sync for an async delegate) and wrapping it in NotifyTask when appropriate. It can have a return value, it can execute on a background thread or the original thread. It creates a cancellation token if needed and a progress reporter object if needed. It moves the exceptions from the sync or async execution into the progress reporter. Also, via a derived class, I keep track of all the delegates and notify the application when interesting things happen.

Two, the command. It's a delegate wrapper that then deals with the various requirements of ICommand such as CanExecute, etc. I did stick with the approach of switching CanExecute to something based on INotifyPropertyChanged. Since the cancellation token has been moved into the delegate wrapper, it doesn't have the same level of complexity as discussed earlier. True some of this complexity has just been moved elsewhere -- there's no free lunch.

Thanks again for all the great code you have out there & for your dedication to the world of async!

sjb

P.S. One other comment. For NotifyTask<TResult>, I ran into cases where I needed it to be derived from NotifyTask. This is quite easily accomplished by using "new" on the Task property in the derived class. The reason I needed this was for a non-generic interface that exposed a NotifyTask property, where the implementation was generic and actually creating a NotifyTask<TResult>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants