CommunityToolkit / dotnet

.NET Community Toolkit is a collection of helpers and APIs that work for all .NET developers and are agnostic of any specific UI platform. The toolkit is maintained and published by Microsoft, and part of the .NET Foundation.
https://docs.microsoft.com/dotnet/communitytoolkit/?WT.mc_id=dotnet-0000-bramin
Other
3.07k stars 299 forks source link

[NotifyCanExecuteChangedFor] not work when the property changed in Async environment #776

Open lqy16 opened 1 year ago

lqy16 commented 1 year ago

Describe the bug

If a property is labeled with [ObservableProperty] and [NotifyCanExecuteChangedFor], e.g. [NotifyCanExecuteChangedFor(nameof(ClickCommand))] [ObservableProperty] public bool indicator; it should notify the property (e.g., bool CanExecute) which decides whether ClickCommand is executable when it changes. When Indicator is changed in Sync environment, all goes well. However, when Indicator is changed in Aync environment, e.g., Task.Run(() => Indicator = false);, it doesn't notify that CanExecute should change, and the executable status of ClickCommand doesn't change.

Regression

No response

Steps to reproduce

It can be easily reproduced with a simple demo. Create a new default WPF project in Visual Studio 2022 with .NET 6.0. Then

1. Modify the main content of MainWindow.xaml file into:
<Window
x:Class="NotifyCanExecuteChangedForBugTest.MainWindow"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:local="clr-namespace:NotifyCanExecuteChangedForBugTest"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
Title="MainWindow"
Width="400"
Height="100"
d:DataContext="{d:DesignInstance local:MainWindowViewModel}"
mc:Ignorable="d">
<Grid>
<StackPanel HorizontalAlignment="Center" Orientation="Horizontal">
<Button Width="100" Height="30" Margin="10,0" Command="{Binding Path=ChangeIndicatorSyncCommand}" Content="Sync" />
<Button Width="100" Height="30" Margin="10,0" Command="{Binding Path=ChangeIndicatorAsyncCommand}" Content="Async" />
<Button Width="100" Height="30" Margin="10,0" Command="{Binding Path=ClickCommand}" Content="Test" />
</StackPanel>
</Grid>
</Window>```"xaml"

2. Add File MainWindowViewModel.cs:
 System.Threading.Tasks;
using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.Input;
namespace NotifyCanExecuteChangedForBugTest
{
partial class MainWindowViewModel : ObservableRecipient
{
public MainWindowViewModel()
{
Indicator = true;
}
[NotifyCanExecuteChangedFor(nameof(ClickCommand))]
[ObservableProperty]
public bool indicator;
private bool CanClick => Indicator;
[RelayCommand(CanExecute = nameof(CanClick))]
private void Click() { }
[RelayCommand]
private void ChangeIndicatorSync()
{
Indicator = false;
}
[RelayCommand]
private void ChangeIndicatorAsync()
{
Task.Run(() => Indicator = false);
}
}
}```"csharp"

3. Add one line in MainWindow.xaml.cs
```DataContext = new MainWindowViewModel();```"csharp"

4. Run the project. (See Screenshots) Initially, the Button "Test" is executable. When "Sync" is clicked, the property Indicator is changed to False, and "Test" is not executable any more. However, when "Async" is clicked, the property Indicator is changed to False in async environment, but "Test" is still executable.

Expected behavior

when "Async" is clicked, "Test" should also become inexecutable.

Screenshots

demo

IDE and version

VS 2022

IDE version

17.5.5

Nuget packages

Nuget package version(s)

8.2.1

Additional context

No response

Help us help you

Yes, I'd like to be assigned to work on this item

lqy16 commented 1 year ago

Currently, I use IValueConverter and IsEnabled to directly bind the excutable status of the Button to the property. That works.

JochenMader commented 1 year ago

Hi there

In my opinion this is not a bug.

According to RelayCommand attribute - Asynchronous commands the command method should return a Task type for this.

[RelayCommand]
private async Task ChangeIndicatorAsync() { ... }

The command is then accessible in xaml as this:

Command="{Binding Path=ChangeIndicatorCommand}"

I also think you should use Dispatcher.BeginInvoke, to update the bounded Properties, when you do your work within another Task.

Here is my working example:

public partial class MainWindowViewModel : ObservableRecipient
{
    public MainWindowViewModel()
    {
        Counter = 1;
        Indicator = true;
    }

    [NotifyCanExecuteChangedFor(nameof(ClickCommand))]
    [ObservableProperty]
    public int counter;

    [NotifyCanExecuteChangedFor(nameof(ClickCommand))]
    [ObservableProperty]
    public bool indicator;

    [RelayCommand(CanExecute = nameof(Indicator))]
    private void Click() => Counter++;

    [RelayCommand]
    private void ChangeIndicatorSync()
    {
        Counter = 0;
        Indicator = !Indicator;
    }

    [RelayCommand]
    private async Task ChangeIndicatorAsync()
    {
        var newIndicatorValue = false;
        await Task.Run(() => newIndicatorValue = !Indicator);
        await Task.Delay(3000);
        await Dispatcher.CurrentDispatcher.BeginInvoke(() =>
        {
            Counter = 0;
            Indicator = newIndicatorValue;
        });
    }
}
<Grid>
    <StackPanel HorizontalAlignment="Center" Orientation="Horizontal">
        <Button Width="100" Height="30" Margin="10,0" Command="{Binding Path=ChangeIndicatorSyncCommand}" Content="Sync" />
        <Button Width="100" Height="30" Margin="10,0" Command="{Binding Path=ChangeIndicatorCommand}" Content="Async" />
        <Button Width="100" Height="30" Margin="10,0" Command="{Binding Path=ClickCommand}" Content="{Binding Counter}" />
    </StackPanel>
</Grid>

best regards

wfjsw commented 10 months ago

It is redundant to update an observable property in Dispatcher as WPF would automatically marshal the change to UI thread no matter where the PropertyChanged event occurs. On the other hand, the CanExecuteChanged does not have this nice feature and I think it would be great to wrap it for parity.