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: How to set CanExecute to a delegate which includes AsyncCommand.Executing #11

Open
cmeeren opened this issue Nov 10, 2016 · 6 comments
Assignees

Comments

@cmeeren
Copy link

cmeeren commented Nov 10, 2016

I have a viewmodel for a sign-in form (in Xamarin.Forms). I want the following:

  1. Submit button disabled when text fields are empty or when busy signing in
  2. Text fields deactivated when signing in

Simplified, the viewmodel (which doesn't work properly) is defined like this:

    // skipping INotifyPropertyChanged implementation for user/pass for brevity
    public string Username { get; set; }
    public string Password { get; set; }

    public IAsyncCommand SignInCommand { get; set; }

    public SignInPageModel()
    {
        SignInCommand = new AsyncCommand(SignInAsync, SignInCommandCanExecute);
        PropertyChanged += delegate { ((AsyncCommand) SignInCommand).OnCanExecuteChanged(); };
    }

    private async Task SignInAsync(object _)
    {
        // actual sign-in process here
        await DoSomeWork();
    }

    private bool SignInCommandCanExecute(object _)
    {
        return !string.IsNullOrEmpty(Username) &&
               !string.IsNullOrEmpty(Password) &&
               !((AsyncCommand SignInCommand).Executing;
    }

What I'm mainly wondering is how I should change this to implement requirement 1. When the command is invoked, there is no way I can call OnCanExecuteChanged so that SignInCommandCanExecute returns the correct value. If I call OnCanExecuteChanged just before and after await DoSomeWork() (which is the earliest and latest I can do it AFAIK, at least without mucking around in the UI codebehind), then the command (and the button that binds to it) is disabled just a tad bit late, and not re-enabled at all (because it's still executing as long as SignInAsync is running).

Unless I have missed an obvious solution, I suggest you fix this by calling OnCanExecuteChanged during the command execution process whether or not CanExecute is set when instantiating the AsyncCommand (i.e., https://github.com/StephenCleary/Mvvm.Async/blob/master/src/Nito.Mvvm.Async/AsyncCommand.cs#L126-L127 and later in that method).

Furthermore, for requirement 2, the obvious solution is to bind the text fields' IsEnabled to SignInCommand.Executing. However, the command is of type IAsyncCommand and needs to be cast to AsyncCommand in order to access the Executing property. Do I really have to make a ValueConverter just for this, or is there a simpler/more elegant way?

@StephenCleary StephenCleary self-assigned this Nov 10, 2016
@StephenCleary
Copy link
Owner

I am working on an easier way to work in this scenario, but for now the best solution is to have your own SignInExecuting property, set and unset inside SignInAsync:

    // skipping INotifyPropertyChanged implementation for user/pass for brevity
    public string Username { get; set; }
    public string Password { get; set; }
    bool SignInExecuting { get; set; }

    public IAsyncCommand SignInCommand { get; set; }

    public SignInPageModel()
    {
        SignInCommand = new AsyncCommand(SignInAsync, SignInCommandCanExecute);
        PropertyChanged += delegate { ((AsyncCommand) SignInCommand).OnCanExecuteChanged(); };
    }

    private async Task SignInAsync(object _)
    {
        SignInExecuting = true;
        // actual sign-in process here
        await DoSomeWork();
       SignInExecuting = false;
    }

    private bool SignInCommandCanExecute(object _)
    {
        return !string.IsNullOrEmpty(Username) &&
               !string.IsNullOrEmpty(Password) &&
               !SignInExecuting;
    }

Regarding IAsyncCommand.Execute, the IAsyncCommand is literally just an async equivalent of ICommand, so I really avoid adding any more requirements than that. If you want to data-bind to Executing, I'd recommend making your command property of type AsyncCommand (it's possible to cast in a binding expression, but it's not pretty).

@cmeeren
Copy link
Author

cmeeren commented Nov 10, 2016

for now the best solution is to have your own SignInExecuting property, set and unset inside SignInAsync

That is, in fact, exactly what I'm currently doing. :)

How do you envision this working in the future? And do you have an (order-of-magnitude) ETA?

@StephenCleary
Copy link
Owner

One part of it is that I'm planning to enhance CalculatedProperties so that it can take regular (NotifyChanged-style) property changes as input to a calculated expression.

Another part of it is a fixing of the (horrible) CanExecute pattern with a true CanExecute property. This part would need to be nicely convertible to the existing pattern, and it should work equally well for synchronous and asynchronous commands.

You'd still have to change the command type to AsyncCommand, which I think is the right solution. You would also still need your own SignInExecuting, which I'm not sure is the right solution. I'm definitely open to persuasion on that one.

@cmeeren
Copy link
Author

cmeeren commented Nov 10, 2016

So if I understand correctly, you want something like a CanExecute calculated property, i.e., a CanExecute property whose value is determined by a delegate? How is this different from the current CanExecute pattern, where the value is also determined by a delegate?

In short, can you give an example (based on the above) of how would a developer would use AsyncCommand with the new functionality?

Note that I have only actually worked as a software developer (and with C#) for a few months, so I might be missing stuff that's obvious to more experienced developers.

@sjb-sjb
Copy link

sjb-sjb commented Jan 22, 2017

Another possibility might be to use the bound functions from issue #5, or more generally to call CanExecuteChanged from a property notification callback.

@sjb-sjb
Copy link

sjb-sjb commented Jan 22, 2017

Oops issue #3 not 5.

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

3 participants