uno.extensions icon indicating copy to clipboard operation
uno.extensions copied to clipboard

[Reactive] Commands: enable CanExecute customization and async

Open weitzhandler opened this issue 2 years ago • 5 comments

What would you like to be added:

Better CanExecute support

  1. Enable async CanExecute
  2. Implement Command.Create<T>(builder => builder.When(AsyncFunc<T>))
  3. Enable MVUX generated-command's CanExecute

Why is this needed:

MVUX supports generation of async commands in the following way:

public ValueTask Move(int myCommandParameter)
{
}

Gets generated to an async command named MyCommand in the bindable model accepting a CommandParameter of int.

What currently gets generated is:

image

It makes sense to also enable customizing the CanExecute if it's prefixed with Can:

public ValueTask<bool> CanMove(int myCommandParameter)
{
}

*Async required because some times need to evaluate and compare with data from feed(s).

weitzhandler avatar Apr 21 '23 02:04 weitzhandler

We can definitely add an attribute to tag the Can<DoSomething> on method like:

[CanExecute(nameof(CanSave))] public void Save(Profile profile) {};
private bool CanSave(Profile profile) => profile.Username is { Length: >3 };

We can even imagine to have a naming convention to do this auto-magically.

However about the async version, I less convinced:

  • The can execute is sync in the view, and is based on a the CommandParameter provided by the view. Making it async will be challenging and might drive to some flickers.
  • If you want to validate using data from feeds, you should then use that feeds in the Given. It's the only way to ensure consistency of the data: you have the insurance that the value you get in the Execute is the same as the value used in the CanExecute.
  • Modern apps are actually not really using the CanExecuteanymore, they prefer to keep the button enabled and display validation results inline or show dialog with a proper message. Not sure it worth the investment.

dr1rrb avatar Apr 21 '23 13:04 dr1rrb

@dr1rrb

After rethinking, I do think there's a valid case for async can-execute, thought not in an async way.

Let's say we have these two feeds in our model:

public IFeed<uint> CurrentPage => ...
public IFeed<uint> PageCount => ...

Then we have the following method to be generateed as command:

public async ValueTask Move(
    uint currentPage,
    uint pageCount,
    int direction,
    CancellationToken ct)

where currentPage and pageCount will be taken from the feeds, and direction is the CommandParameter from the UI.

In a case as this one, since all parameters' source are known, we can listen to their changes and only reevaluate their can-execute state on-demand, if any of the dependent feeds change, otherwise use a cached result. Obviously sticking to the standard intended use of Commanding is preferred, especially given that there will be no need to write custom feeds for each IsEnabled of each button.

Considering this information, I think we should consider a CanXXX prefixed synchronous method, which will be fed with the most-recent feed values, if their signature matches their XXX counterpart. Since we're already standardizing methods-to-commands generation, Can prefix is a very natural candidate for auto generation. Attributes can be considered as well, but I have an even stronger vote for naming convention.

So in our previous example, the following method should be matched:

public /* sync */ bool CanMove(
    /* latest feed value */ uint currentPage,
    /* latest feed value */ uint pageCount,
    /* View value */ int direction
    /* no support for cancellation needed */)

This is what's generated for Move above, the same can be used for CanMove:

image

The above generated code (line 69) should be changed to return vm.CanMove(... like in line 82. If I understand correctly, these arguments are there already.

weitzhandler avatar Apr 24 '23 04:04 weitzhandler

Considering this information, I think we should consider a CanXXX prefixed synchronous method, which will be fed with the most-recent feed values, if their signature matches their XXX counterpart.

Yes it's exact;y what I was thinking at :)

dr1rrb avatar Apr 24 '23 13:04 dr1rrb

Just to clear, the builder allows for the CanExecute to be specified, right?

I get that we could have a conventipon that adds it by convention with CanXYZ(parameter) based on a "local" feed like we have for the method, but to me it's low priority for now.

francoistanguay avatar May 08 '23 23:05 francoistanguay

Just to clear, the builder allows for the CanExecute to be specified, right?

Yes :)

dr1rrb avatar May 09 '23 21:05 dr1rrb