From c3b3d035746d1721362278f0021c27bdf32710b4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 13 Aug 2020 18:32:32 +0200 Subject: [PATCH 01/31] Add initial version of ObservableValidator type --- .../ComponentModel/ObservableValidator.cs | 77 +++++++++++++++++++ .../Microsoft.Toolkit.Mvvm.csproj | 1 + 2 files changed, 78 insertions(+) create mode 100644 Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs new file mode 100644 index 00000000000..c0d58102eb0 --- /dev/null +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -0,0 +1,77 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Collections; +using System.Collections.Generic; +using System.ComponentModel; +using System.ComponentModel.DataAnnotations; + +namespace Microsoft.Toolkit.Mvvm.ComponentModel +{ + /// + /// A base class for objects implementing the interface. This class + /// also inherits from , so it can be used for observable items too. + /// + public abstract class ObservableValidator : ObservableObject, INotifyDataErrorInfo + { + /// + /// The instance used to store previous validation results. + /// + private readonly Dictionary> errors = new Dictionary>(); + + /// + public event EventHandler? ErrorsChanged; + + /// + public bool HasErrors => this.errors.Count > 0; + + /// + public IEnumerable GetErrors(string propertyName) + { + return this.errors[propertyName]; + } + + /// + /// Validates a property with a specified name and a given input value. + /// + /// The name of the property to validate. + /// The value to test for the specified property. + public void ValidateProperty(string propertyName, object value) + { + // Clear the errors for the specified property, if any + if (this.errors.TryGetValue(propertyName, out List? propertyErrors)) + { + propertyErrors.Clear(); + + ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); + } + + List results = new List(); + + // Validate the property + if (!Validator.TryValidateProperty( + value, + new ValidationContext(this, null, null) { MemberName = propertyName }, + results)) + { + // We can avoid the repeated dictionary lookup if we just check the result of the previous + // one here. If the retrieved errors collection is null, we can log the ones produced by + // the validation we just performed. We also don't need to create a new list and add them + // to that, as we can directly set the resulting list as value in the mapping. If the + // property already had some other logged errors instead, we can add the new ones as a range. + if (propertyErrors is null) + { + this.errors.Add(propertyName, results); + } + else + { + propertyErrors.AddRange(results); + } + + ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); + } + } + } +} \ No newline at end of file diff --git a/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj b/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj index 25371e3abc3..b98b400247d 100644 --- a/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj +++ b/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj @@ -28,6 +28,7 @@ + From 67908f36834a1390d1cd6e32fcad16a948236c8d Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 13 Aug 2020 18:47:05 +0200 Subject: [PATCH 02/31] Update ValidateProperty API to mirror SetProperty --- .../ComponentModel/ObservableValidator.cs | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index c0d58102eb0..e7da951eb73 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -7,6 +7,8 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; +using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; namespace Microsoft.Toolkit.Mvvm.ComponentModel { @@ -36,12 +38,19 @@ public IEnumerable GetErrors(string propertyName) /// /// Validates a property with a specified name and a given input value. /// - /// The name of the property to validate. /// The value to test for the specified property. - public void ValidateProperty(string propertyName, object value) + /// The name of the property to validate. + /// Thrown when is . + public void ValidateProperty(object value, [CallerMemberName] string? propertyName = null) { + if (propertyName is null) + { + ThrowArgumentNullExceptionForNullPropertyName(); + } + // Clear the errors for the specified property, if any - if (this.errors.TryGetValue(propertyName, out List? propertyErrors)) + if (this.errors.TryGetValue(propertyName!, out List? propertyErrors) && + propertyErrors.Count > 0) { propertyErrors.Clear(); @@ -63,7 +72,7 @@ public void ValidateProperty(string propertyName, object value) // property already had some other logged errors instead, we can add the new ones as a range. if (propertyErrors is null) { - this.errors.Add(propertyName, results); + this.errors.Add(propertyName!, results); } else { @@ -73,5 +82,13 @@ public void ValidateProperty(string propertyName, object value) ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } } + + /// + /// Throws an when a property name given as input is . + /// + private static void ThrowArgumentNullExceptionForNullPropertyName() + { + throw new ArgumentNullException("propertyName", "The input property name cannot be null when validating a property"); + } } } \ No newline at end of file From ffb54ca73568ef54f78564a7108681ec8917baa4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 13 Aug 2020 18:51:08 +0200 Subject: [PATCH 03/31] Add XML docs for thrown exceptions --- Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs | 5 +++++ Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs | 2 ++ Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs | 5 +++++ 3 files changed, 12 insertions(+) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index 61f5c41f65e..eaff311f873 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -220,6 +220,7 @@ protected bool SetProperty(T oldValue, T newValue, IEqualityComparer compa /// be used to access properties of a model that is directly stored as a property of the current instance. /// Additionally, this method can only be used if the wrapped item is a reference type. /// + /// Thrown when is not valid. protected bool SetProperty(Expression> propertyExpression, T newValue, [CallerMemberName] string? propertyName = null) { return SetProperty(propertyExpression, newValue, EqualityComparer.Default, out _, propertyName); @@ -238,6 +239,7 @@ protected bool SetProperty(Expression> propertyExpression, T newValue /// The instance to use to compare the input values. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. + /// Thrown when is not valid. protected bool SetProperty(Expression> propertyExpression, T newValue, IEqualityComparer comparer, [CallerMemberName] string? propertyName = null) { return SetProperty(propertyExpression, newValue, comparer, out _, propertyName); @@ -253,6 +255,7 @@ protected bool SetProperty(Expression> propertyExpression, T newValue /// The resulting initial value for the target property. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. + /// Thrown when is not valid. private protected bool SetProperty(Expression> propertyExpression, T newValue, IEqualityComparer comparer, out T oldValue, [CallerMemberName] string? propertyName = null) { PropertyInfo? parentPropertyInfo; @@ -328,6 +331,7 @@ parentExpression.Expression is ConstantExpression instanceExpression && /// indicates that the new value being assigned to is different than the previous one, /// and it does not mean the new instance passed as argument is in any particular state. /// + /// Thrown when is not valid. protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, [CallerMemberName] string? propertyName = null) where TTask : Task { @@ -360,6 +364,7 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express /// The and events are not raised /// if the current and new value for the target property are the same. /// + /// Thrown when is not valid. protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) where TTask : Task { diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs index ca847383b87..5c4ba934e60 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs @@ -247,6 +247,7 @@ protected bool SetProperty(T oldValue, T newValue, IEqualityComparer compa /// If , will also be invoked. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. + /// Thrown when is not valid. protected bool SetProperty(Expression> propertyExpression, T newValue, bool broadcast, [CallerMemberName] string? propertyName = null) { return SetProperty(propertyExpression, newValue, EqualityComparer.Default, broadcast, propertyName); @@ -268,6 +269,7 @@ protected bool SetProperty(Expression> propertyExpression, T newValue /// If , will also be invoked. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. + /// Thrown when is not valid. protected bool SetProperty(Expression> propertyExpression, T newValue, IEqualityComparer comparer, bool broadcast, [CallerMemberName] string? propertyName = null) { bool propertyChanged = SetProperty(propertyExpression, newValue, comparer, out T oldValue, propertyName); diff --git a/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs b/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs index 7546d684da8..f8567c0de57 100644 --- a/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs +++ b/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs @@ -54,6 +54,7 @@ public sealed class Ioc : IServiceProvider private volatile ServiceProvider? serviceProvider; /// + /// Thrown if the current instance has not been configured yet. object? IServiceProvider.GetService(Type serviceType) { // As per section I.12.6.6 of the official CLI ECMA-335 spec: @@ -81,6 +82,7 @@ public sealed class Ioc : IServiceProvider /// Initializes the shared instance. /// /// The configuration delegate to use to add services. + /// Thrown if the current instance has already been configured. public void ConfigureServices(Action setup) { ConfigureServices(setup, new ServiceProviderOptions()); @@ -91,6 +93,7 @@ public void ConfigureServices(Action setup) /// /// The configuration delegate to use to add services. /// The instance to configure the service provider behaviors. + /// Thrown if the current instance has already been configured. public void ConfigureServices(Action setup, ServiceProviderOptions options) { var collection = new ServiceCollection(); @@ -104,6 +107,7 @@ public void ConfigureServices(Action setup, ServiceProviderO /// Initializes the shared instance. /// /// The input instance to use. + /// Thrown if the current instance has already been configured. public void ConfigureServices(IServiceCollection services) { ConfigureServices(services, new ServiceProviderOptions()); @@ -114,6 +118,7 @@ public void ConfigureServices(IServiceCollection services) /// /// The input instance to use. /// The instance to configure the service provider behaviors. + /// Thrown if the current instance has already been configured. public void ConfigureServices(IServiceCollection services, ServiceProviderOptions options) { ServiceProvider newServices = services.BuildServiceProvider(options); From f429a78ad89f078e9d5dfc4b1dab90209312853f Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Tue, 18 Aug 2020 13:19:12 +0200 Subject: [PATCH 04/31] Removed unnecessary using directive --- Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index e7da951eb73..5bf505fc6dc 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -7,7 +7,6 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; -using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; namespace Microsoft.Toolkit.Mvvm.ComponentModel From 1cb4df26de0ed40ff3c3ad3993daf7618ae8ff2a Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 19 Aug 2020 13:45:47 +0200 Subject: [PATCH 05/31] Speed/memory improvements to SetPropertyAndNotifyOnCompletion --- .../ComponentModel/ObservableObject.cs | 58 +++++++------------ .../Input/AsyncRelayCommand.cs | 2 +- .../Input/AsyncRelayCommand{T}.cs | 2 +- .../Mvvm/Test_ObservableObject.cs | 2 +- 4 files changed, 24 insertions(+), 40 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index eaff311f873..4e5eb57ecaa 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -312,27 +312,22 @@ parentExpression.Expression is ConstantExpression instanceExpression && /// public Task MyTask /// { /// get => myTask; - /// private set => SetAndNotifyOnCompletion(ref myTask, () => myTask, value); + /// private set => SetAndNotifyOnCompletion(() => ref myTask, value); /// } /// /// /// The type of to set and monitor. - /// The field storing the property's value. - /// - /// An returning the field to update. This is needed to be - /// able to raise the to notify the completion of the input task. - /// + /// The instance to access the backing field for the property. /// The property's value after the change occurred. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. /// - /// The and events are not raised if the current - /// and new value for the target property are the same. The return value being only - /// indicates that the new value being assigned to is different than the previous one, + /// The and events are not raised if the current and new + /// value for the target property are the same. The return value being only indicates that the + /// new value being assigned to field provided by is different than the previous one, /// and it does not mean the new instance passed as argument is in any particular state. /// - /// Thrown when is not valid. - protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, [CallerMemberName] string? propertyName = null) + protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fieldAccessor, TTask? newValue, [CallerMemberName] string? propertyName = null) where TTask : Task { // We invoke the overload with a callback here to avoid code duplication, and simply pass an empty callback. @@ -341,21 +336,19 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express // instance. This will result in no further allocations after the first time this method is called for a given // generic type. We only pay the cost of the virtual call to the delegate, but this is not performance critical // code and that overhead would still be much lower than the rest of the method anyway, so that's fine. - return SetPropertyAndNotifyOnCompletion(ref field, fieldExpression, newValue, _ => { }, propertyName); + return SetPropertyAndNotifyOnCompletion(fieldAccessor, newValue, _ => { }, propertyName); } /// /// Compares the current and new values for a given field (which should be the backing /// field for a property). If the value has changed, raises the /// event, updates the field and then raises the event. - /// This method is just like , + /// This method is just like , /// with the difference being an extra parameter with a callback being invoked /// either immediately, if the new task has already completed or is , or upon completion. /// /// The type of to set and monitor. - /// The field storing the property's value. - /// - /// An returning the field to update. + /// The instance to access the backing field for the property. /// The property's value after the change occurred. /// A callback to invoke to update the property value. /// (optional) The name of the property that changed. @@ -364,10 +357,12 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express /// The and events are not raised /// if the current and new value for the target property are the same. /// - /// Thrown when is not valid. - protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) + protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fieldAccessor, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) where TTask : Task { + // Invoke the accessor once to get a field reference for the synchronous part + ref TTask? field = ref fieldAccessor(); + if (ReferenceEquals(field, newValue)) { return false; @@ -398,16 +393,6 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express return true; } - // Get the target field to set. This is needed because we can't - // capture the ref field in a closure (for the async method). - if (!((fieldExpression.Body as MemberExpression)?.Member is FieldInfo fieldInfo)) - { - ThrowArgumentExceptionForInvalidFieldExpression(); - - // This is never executed, as the method above always throws - return false; - } - // We use a local async function here so that the main method can // remain synchronous and return a value that can be immediately // used by the caller. This mirrors Set(ref T, T, string). @@ -427,7 +412,7 @@ async void MonitorTask() { } - TTask? currentTask = (TTask?)fieldInfo.GetValue(this); + TTask? currentTask = fieldAccessor(); // Only notify if the property hasn't changed if (ReferenceEquals(newValue, currentTask)) @@ -444,19 +429,18 @@ async void MonitorTask() } /// - /// Throws an when a given is invalid for a property. + /// A custom that returns a reference to a backing field to a property. /// - private static void ThrowArgumentExceptionForInvalidPropertyExpression() - { - throw new ArgumentException("The given expression must be in the form () => MyModel.MyProperty"); - } + /// The type of reference to return. + /// A reference to the backing field of a property. + protected delegate ref T FieldAccessor(); /// - /// Throws an when a given is invalid for a property field. + /// Throws an when a given is invalid for a property. /// - private static void ThrowArgumentExceptionForInvalidFieldExpression() + private static void ThrowArgumentExceptionForInvalidPropertyExpression() { - throw new ArgumentException("The given expression must be in the form () => field"); + throw new ArgumentException("The given expression must be in the form () => MyModel.MyProperty"); } } } \ No newline at end of file diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs index ee03b23a1f0..b2c74b300db 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs @@ -58,7 +58,7 @@ public Task? ExecutionTask get => this.executionTask; private set { - if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, () => this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) + if (SetPropertyAndNotifyOnCompletion(() => ref this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) { OnPropertyChanged(nameof(IsRunning)); } diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs index 14d2c65d4b7..3d8d733ddcd 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs @@ -58,7 +58,7 @@ public Task? ExecutionTask get => this.executionTask; private set { - if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, () => this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) + if (SetPropertyAndNotifyOnCompletion(() => ref this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) { OnPropertyChanged(nameof(IsRunning)); } diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs index 89e70171139..82f64fffbdd 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs @@ -226,7 +226,7 @@ public class SampleModelWithTask : ObservableObject public Task Data { get => data; - set => SetPropertyAndNotifyOnCompletion(ref data, () => data, value); + set => SetPropertyAndNotifyOnCompletion(() => ref data, value); } } } From 06cc52062cf2b7e7555da43dd83c96f5a3dbadca Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 22 Aug 2020 00:45:31 +0200 Subject: [PATCH 06/31] Added cancelation support for async commands --- .../Input/AsyncRelayCommand.cs | 65 ++++++++++++++++++- .../Input/AsyncRelayCommand{T}.cs | 65 ++++++++++++++++++- .../Input/Interfaces/IAsyncRelayCommand.cs | 19 ++++++ 3 files changed, 145 insertions(+), 4 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs index b2c74b300db..c46a3c174bd 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs @@ -4,6 +4,7 @@ using System; using System.Runtime.CompilerServices; +using System.Threading; using System.Threading.Tasks; using Microsoft.Toolkit.Mvvm.ComponentModel; @@ -20,13 +21,25 @@ public sealed class AsyncRelayCommand : ObservableObject, IAsyncRelayCommand /// /// The to invoke when is used. /// - private readonly Func execute; + private readonly Func? execute; + + /// + /// The cancelable to invoke when is used. + /// + /// Only one between this and is not . + private readonly Func? cancellableExecute; /// /// The optional action to invoke when is used. /// private readonly Func? canExecute; + /// + /// The instance to use to cancel . + /// + /// This is only used when is not . + private CancellationTokenSource? cancellationTokenSource; + /// public event EventHandler? CanExecuteChanged; @@ -42,6 +55,15 @@ public AsyncRelayCommand(Func execute) /// /// Initializes a new instance of the class that can always execute. /// + /// The cancelable execution logic. + public AsyncRelayCommand(Func cancellableExecute) + { + this.cancellableExecute = cancellableExecute; + } + + /// + /// Initializes a new instance of the class. + /// /// The execution logic. /// The execution status logic. public AsyncRelayCommand(Func execute, Func canExecute) @@ -50,6 +72,17 @@ public AsyncRelayCommand(Func execute, Func canExecute) this.canExecute = canExecute; } + /// + /// Initializes a new instance of the class. + /// + /// The cancelable execution logic. + /// The execution status logic. + public AsyncRelayCommand(Func cancellableExecute, Func canExecute) + { + this.cancellableExecute = cancellableExecute; + this.canExecute = canExecute; + } + private Task? executionTask; /// @@ -65,6 +98,12 @@ private set } } + /// + public bool CanBeCanceled => !(this.cancellableExecute is null); + + /// + public bool IsCancellationRequested => this.cancellationTokenSource?.IsCancellationRequested == true; + /// public bool IsRunning => ExecutionTask?.IsCompleted == false; @@ -92,10 +131,32 @@ public Task ExecuteAsync(object? parameter) { if (CanExecute(parameter)) { - return ExecutionTask = this.execute(); + // Non cancelable command delegate + if (!(this.execute is null)) + { + return ExecutionTask = this.execute(); + } + + // Cancel the previous operation, if one is pending + this.cancellationTokenSource?.Cancel(); + + var cancellationTokenSource = this.cancellationTokenSource = new CancellationTokenSource(); + + OnPropertyChanged(nameof(IsCancellationRequested)); + + // Invoke the cancelable command delegate with a new linked token + return ExecutionTask = this.cancellableExecute!(cancellationTokenSource.Token); } return Task.CompletedTask; } + + /// + public void Cancel() + { + this.cancellationTokenSource?.Cancel(); + + OnPropertyChanged(nameof(IsCancellationRequested)); + } } } \ No newline at end of file diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs index 3d8d733ddcd..cd2162ca46b 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs @@ -4,6 +4,7 @@ using System; using System.Runtime.CompilerServices; +using System.Threading; using System.Threading.Tasks; using Microsoft.Toolkit.Mvvm.ComponentModel; @@ -18,13 +19,23 @@ public sealed class AsyncRelayCommand : ObservableObject, IAsyncRelayCommand< /// /// The to invoke when is used. /// - private readonly Func execute; + private readonly Func? execute; + + /// + /// The cancelable to invoke when is used. + /// + private readonly Func? cancellableExecute; /// /// The optional action to invoke when is used. /// private readonly Func? canExecute; + /// + /// The instance to use to cancel . + /// + private CancellationTokenSource? cancellationTokenSource; + /// public event EventHandler? CanExecuteChanged; @@ -41,6 +52,16 @@ public AsyncRelayCommand(Func execute) /// /// Initializes a new instance of the class that can always execute. /// + /// The cancelable execution logic. + /// See notes in . + public AsyncRelayCommand(Func cancellableExecute) + { + this.cancellableExecute = cancellableExecute; + } + + /// + /// Initializes a new instance of the class. + /// /// The execution logic. /// The execution status logic. /// See notes in . @@ -50,6 +71,18 @@ public AsyncRelayCommand(Func execute, Func canExecute) this.canExecute = canExecute; } + /// + /// Initializes a new instance of the class. + /// + /// The cancelable execution logic. + /// The execution status logic. + /// See notes in . + public AsyncRelayCommand(Func cancellableExecute, Func canExecute) + { + this.cancellableExecute = cancellableExecute; + this.canExecute = canExecute; + } + private Task? executionTask; /// @@ -65,6 +98,12 @@ private set } } + /// + public bool CanBeCanceled => !(this.cancellableExecute is null); + + /// + public bool IsCancellationRequested => this.cancellationTokenSource?.IsCancellationRequested == true; + /// public bool IsRunning => ExecutionTask?.IsCompleted == false; @@ -113,7 +152,21 @@ public Task ExecuteAsync(T parameter) { if (CanExecute(parameter)) { - return ExecutionTask = this.execute(parameter); + // Non cancelable command delegate + if (!(this.execute is null)) + { + return ExecutionTask = this.execute(parameter); + } + + // Cancel the previous operation, if one is pending + this.cancellationTokenSource?.Cancel(); + + var cancellationTokenSource = this.cancellationTokenSource = new CancellationTokenSource(); + + OnPropertyChanged(nameof(IsCancellationRequested)); + + // Invoke the cancelable command delegate with a new linked token + return ExecutionTask = this.cancellableExecute!(parameter, cancellationTokenSource.Token); } return Task.CompletedTask; @@ -124,5 +177,13 @@ public Task ExecuteAsync(object? parameter) { return ExecuteAsync((T)parameter!); } + + /// + public void Cancel() + { + this.cancellationTokenSource?.Cancel(); + + OnPropertyChanged(nameof(IsCancellationRequested)); + } } } \ No newline at end of file diff --git a/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs index ea56dfae4e3..39d979ec303 100644 --- a/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs @@ -18,6 +18,16 @@ public interface IAsyncRelayCommand : IRelayCommand, INotifyPropertyChanged /// Task? ExecutionTask { get; } + /// + /// Gets a value indicating whether running operations for this command can be canceled. + /// + public bool CanBeCanceled { get; } + + /// + /// Gets a value indicating whether a cancelation request has been issued for the current operation. + /// + public bool IsCancellationRequested { get; } + /// /// Gets a value indicating whether the command currently has a pending operation being executed. /// @@ -30,5 +40,14 @@ public interface IAsyncRelayCommand : IRelayCommand, INotifyPropertyChanged /// The input parameter. /// The representing the async operation being executed. Task ExecuteAsync(object? parameter); + + /// + /// Communicates a request for cancelation. + /// + /// + /// If the underlying command is not running, or if it does not support cancelation, this method will perform no action. + /// Note that even with a successful cancelation, the completion of the current operation might not be immediate. + /// + void Cancel(); } } From 06ca18615ce7b693fac63974bf3c9f63bcc26991 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 22 Aug 2020 00:57:46 +0200 Subject: [PATCH 07/31] Fixed spelling to follow conventions --- .../Input/AsyncRelayCommand.cs | 22 +++++++++---------- .../Input/AsyncRelayCommand{T}.cs | 20 ++++++++--------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs index c46a3c174bd..f9b41df775d 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs @@ -27,7 +27,7 @@ public sealed class AsyncRelayCommand : ObservableObject, IAsyncRelayCommand /// The cancelable to invoke when is used. /// /// Only one between this and is not . - private readonly Func? cancellableExecute; + private readonly Func? cancelableExecute; /// /// The optional action to invoke when is used. @@ -35,9 +35,9 @@ public sealed class AsyncRelayCommand : ObservableObject, IAsyncRelayCommand private readonly Func? canExecute; /// - /// The instance to use to cancel . + /// The instance to use to cancel . /// - /// This is only used when is not . + /// This is only used when is not . private CancellationTokenSource? cancellationTokenSource; /// @@ -55,10 +55,10 @@ public AsyncRelayCommand(Func execute) /// /// Initializes a new instance of the class that can always execute. /// - /// The cancelable execution logic. - public AsyncRelayCommand(Func cancellableExecute) + /// The cancelable execution logic. + public AsyncRelayCommand(Func cancelableExecute) { - this.cancellableExecute = cancellableExecute; + this.cancelableExecute = cancelableExecute; } /// @@ -75,11 +75,11 @@ public AsyncRelayCommand(Func execute, Func canExecute) /// /// Initializes a new instance of the class. /// - /// The cancelable execution logic. + /// The cancelable execution logic. /// The execution status logic. - public AsyncRelayCommand(Func cancellableExecute, Func canExecute) + public AsyncRelayCommand(Func cancelableExecute, Func canExecute) { - this.cancellableExecute = cancellableExecute; + this.cancelableExecute = cancelableExecute; this.canExecute = canExecute; } @@ -99,7 +99,7 @@ private set } /// - public bool CanBeCanceled => !(this.cancellableExecute is null); + public bool CanBeCanceled => !(this.cancelableExecute is null); /// public bool IsCancellationRequested => this.cancellationTokenSource?.IsCancellationRequested == true; @@ -145,7 +145,7 @@ public Task ExecuteAsync(object? parameter) OnPropertyChanged(nameof(IsCancellationRequested)); // Invoke the cancelable command delegate with a new linked token - return ExecutionTask = this.cancellableExecute!(cancellationTokenSource.Token); + return ExecutionTask = this.cancelableExecute!(cancellationTokenSource.Token); } return Task.CompletedTask; diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs index cd2162ca46b..83d25155cf6 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs @@ -24,7 +24,7 @@ public sealed class AsyncRelayCommand : ObservableObject, IAsyncRelayCommand< /// /// The cancelable to invoke when is used. /// - private readonly Func? cancellableExecute; + private readonly Func? cancelableExecute; /// /// The optional action to invoke when is used. @@ -32,7 +32,7 @@ public sealed class AsyncRelayCommand : ObservableObject, IAsyncRelayCommand< private readonly Func? canExecute; /// - /// The instance to use to cancel . + /// The instance to use to cancel . /// private CancellationTokenSource? cancellationTokenSource; @@ -52,11 +52,11 @@ public AsyncRelayCommand(Func execute) /// /// Initializes a new instance of the class that can always execute. /// - /// The cancelable execution logic. + /// The cancelable execution logic. /// See notes in . - public AsyncRelayCommand(Func cancellableExecute) + public AsyncRelayCommand(Func cancelableExecute) { - this.cancellableExecute = cancellableExecute; + this.cancelableExecute = cancelableExecute; } /// @@ -74,12 +74,12 @@ public AsyncRelayCommand(Func execute, Func canExecute) /// /// Initializes a new instance of the class. /// - /// The cancelable execution logic. + /// The cancelable execution logic. /// The execution status logic. /// See notes in . - public AsyncRelayCommand(Func cancellableExecute, Func canExecute) + public AsyncRelayCommand(Func cancelableExecute, Func canExecute) { - this.cancellableExecute = cancellableExecute; + this.cancelableExecute = cancelableExecute; this.canExecute = canExecute; } @@ -99,7 +99,7 @@ private set } /// - public bool CanBeCanceled => !(this.cancellableExecute is null); + public bool CanBeCanceled => !(this.cancelableExecute is null); /// public bool IsCancellationRequested => this.cancellationTokenSource?.IsCancellationRequested == true; @@ -166,7 +166,7 @@ public Task ExecuteAsync(T parameter) OnPropertyChanged(nameof(IsCancellationRequested)); // Invoke the cancelable command delegate with a new linked token - return ExecutionTask = this.cancellableExecute!(parameter, cancellationTokenSource.Token); + return ExecutionTask = this.cancelableExecute!(parameter, cancellationTokenSource.Token); } return Task.CompletedTask; From b8610abd5d281e91680aeaf838abf858ded49d60 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 22 Aug 2020 01:16:29 +0200 Subject: [PATCH 08/31] Added unit tests for cancelable async commands --- .../Mvvm/Test_AsyncRelayCommand.cs | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_AsyncRelayCommand.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_AsyncRelayCommand.cs index 4a10690c6b2..09ff1d35a90 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_AsyncRelayCommand.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_AsyncRelayCommand.cs @@ -28,6 +28,9 @@ public async Task Test_AsyncRelayCommand_AlwaysEnabled() Assert.IsTrue(command.CanExecute(null)); Assert.IsTrue(command.CanExecute(new object())); + Assert.IsFalse(command.CanBeCanceled); + Assert.IsFalse(command.IsCancellationRequested); + (object, EventArgs) args = default; command.CanExecuteChanged += (s, e) => args = (s, e); @@ -75,6 +78,9 @@ public void Test_AsyncRelayCommand_WithCanExecuteFunctionTrue() Assert.IsTrue(command.CanExecute(null)); Assert.IsTrue(command.CanExecute(new object())); + Assert.IsFalse(command.CanBeCanceled); + Assert.IsFalse(command.IsCancellationRequested); + command.Execute(null); Assert.AreEqual(ticks, 1); @@ -100,6 +106,9 @@ public void Test_AsyncRelayCommand_WithCanExecuteFunctionFalse() Assert.IsFalse(command.CanExecute(null)); Assert.IsFalse(command.CanExecute(new object())); + Assert.IsFalse(command.CanBeCanceled); + Assert.IsFalse(command.IsCancellationRequested); + command.Execute(null); Assert.AreEqual(ticks, 0); @@ -108,5 +117,34 @@ public void Test_AsyncRelayCommand_WithCanExecuteFunctionFalse() Assert.AreEqual(ticks, 0); } + + [TestCategory("Mvvm")] + [TestMethod] + public async Task Test_AsyncRelayCommand_WithCancellation() + { + TaskCompletionSource tcs = new TaskCompletionSource(); + + var command = new AsyncRelayCommand(token => tcs.Task); + + Assert.IsTrue(command.CanExecute(null)); + Assert.IsTrue(command.CanExecute(new object())); + + Assert.IsTrue(command.CanBeCanceled); + Assert.IsFalse(command.IsCancellationRequested); + + command.Execute(null); + + Assert.IsFalse(command.IsCancellationRequested); + + command.Cancel(); + + Assert.IsTrue(command.IsCancellationRequested); + + tcs.SetResult(null); + + await command.ExecutionTask!; + + Assert.IsTrue(command.IsCancellationRequested); + } } } From 38680eaab223e2680fa5de1325a63e09970cce50 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sun, 23 Aug 2020 11:50:33 +0200 Subject: [PATCH 09/31] Removed unnecessary visibility modifiers --- Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs index 39d979ec303..c3f663feb20 100644 --- a/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/Interfaces/IAsyncRelayCommand.cs @@ -21,12 +21,12 @@ public interface IAsyncRelayCommand : IRelayCommand, INotifyPropertyChanged /// /// Gets a value indicating whether running operations for this command can be canceled. /// - public bool CanBeCanceled { get; } + bool CanBeCanceled { get; } /// /// Gets a value indicating whether a cancelation request has been issued for the current operation. /// - public bool IsCancellationRequested { get; } + bool IsCancellationRequested { get; } /// /// Gets a value indicating whether the command currently has a pending operation being executed. From cfb26287273e4e2f5c303400255a641343d19b4c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Mon, 31 Aug 2020 22:35:32 +0200 Subject: [PATCH 10/31] Temporary reverted FieldAccessor APIs Regression to unblock UWP for now, see https://github.com/dotnet/corert/issues/8273 --- .../ComponentModel/ObservableObject.cs | 56 ++++++++++++------- .../Input/AsyncRelayCommand.cs | 2 +- .../Input/AsyncRelayCommand{T}.cs | 2 +- .../Mvvm/Test_ObservableObject.cs | 2 +- 4 files changed, 38 insertions(+), 24 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index 4e5eb57ecaa..a4b786cf668 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -312,22 +312,26 @@ parentExpression.Expression is ConstantExpression instanceExpression && /// public Task MyTask /// { /// get => myTask; - /// private set => SetAndNotifyOnCompletion(() => ref myTask, value); + /// private set => SetAndNotifyOnCompletion(ref myTask, () => myTask, value); /// } /// /// /// The type of to set and monitor. - /// The instance to access the backing field for the property. + /// The field storing the property's value. + /// + /// An returning the field to update. This is needed to be + /// able to raise the to notify the completion of the input task. + /// /// The property's value after the change occurred. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. /// - /// The and events are not raised if the current and new - /// value for the target property are the same. The return value being only indicates that the - /// new value being assigned to field provided by is different than the previous one, + /// The and events are not raised if the current + /// and new value for the target property are the same. The return value being only + /// indicates that the new value being assigned to is different than the previous one, /// and it does not mean the new instance passed as argument is in any particular state. /// - protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fieldAccessor, TTask? newValue, [CallerMemberName] string? propertyName = null) + protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, [CallerMemberName] string? propertyName = null) where TTask : Task { // We invoke the overload with a callback here to avoid code duplication, and simply pass an empty callback. @@ -336,19 +340,21 @@ protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fie // instance. This will result in no further allocations after the first time this method is called for a given // generic type. We only pay the cost of the virtual call to the delegate, but this is not performance critical // code and that overhead would still be much lower than the rest of the method anyway, so that's fine. - return SetPropertyAndNotifyOnCompletion(fieldAccessor, newValue, _ => { }, propertyName); + return SetPropertyAndNotifyOnCompletion(ref field, fieldExpression, newValue, _ => { }, propertyName); } /// /// Compares the current and new values for a given field (which should be the backing /// field for a property). If the value has changed, raises the /// event, updates the field and then raises the event. - /// This method is just like , + /// This method is just like , /// with the difference being an extra parameter with a callback being invoked /// either immediately, if the new task has already completed or is , or upon completion. /// /// The type of to set and monitor. - /// The instance to access the backing field for the property. + /// The field storing the property's value. + /// + /// An returning the field to update. /// The property's value after the change occurred. /// A callback to invoke to update the property value. /// (optional) The name of the property that changed. @@ -357,12 +363,9 @@ protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fie /// The and events are not raised /// if the current and new value for the target property are the same. /// - protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fieldAccessor, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) + protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) where TTask : Task { - // Invoke the accessor once to get a field reference for the synchronous part - ref TTask? field = ref fieldAccessor(); - if (ReferenceEquals(field, newValue)) { return false; @@ -393,6 +396,16 @@ protected bool SetPropertyAndNotifyOnCompletion(FieldAccessor fie return true; } + // Get the target field to set. This is needed because we can't + // capture the ref field in a closure (for the async method). + if (!((fieldExpression.Body as MemberExpression)?.Member is FieldInfo fieldInfo)) + { + ThrowArgumentExceptionForInvalidFieldExpression(); + + // This is never executed, as the method above always throws + return false; + } + // We use a local async function here so that the main method can // remain synchronous and return a value that can be immediately // used by the caller. This mirrors Set(ref T, T, string). @@ -412,7 +425,7 @@ async void MonitorTask() { } - TTask? currentTask = fieldAccessor(); + TTask? currentTask = (TTask?)fieldInfo.GetValue(this); // Only notify if the property hasn't changed if (ReferenceEquals(newValue, currentTask)) @@ -428,13 +441,6 @@ async void MonitorTask() return true; } - /// - /// A custom that returns a reference to a backing field to a property. - /// - /// The type of reference to return. - /// A reference to the backing field of a property. - protected delegate ref T FieldAccessor(); - /// /// Throws an when a given is invalid for a property. /// @@ -442,5 +448,13 @@ private static void ThrowArgumentExceptionForInvalidPropertyExpression() { throw new ArgumentException("The given expression must be in the form () => MyModel.MyProperty"); } + + /// + /// Throws an when a given is invalid for a property field. + /// + private static void ThrowArgumentExceptionForInvalidFieldExpression() + { + throw new ArgumentException("The given expression must be in the form () => field"); + } } } \ No newline at end of file diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs index f9b41df775d..471fff34169 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs @@ -91,7 +91,7 @@ public Task? ExecutionTask get => this.executionTask; private set { - if (SetPropertyAndNotifyOnCompletion(() => ref this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) + if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, () => this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) { OnPropertyChanged(nameof(IsRunning)); } diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs index 83d25155cf6..2a265f0752e 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs @@ -91,7 +91,7 @@ public Task? ExecutionTask get => this.executionTask; private set { - if (SetPropertyAndNotifyOnCompletion(() => ref this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) + if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, () => this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) { OnPropertyChanged(nameof(IsRunning)); } diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs index 82f64fffbdd..89e70171139 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs @@ -226,7 +226,7 @@ public class SampleModelWithTask : ObservableObject public Task Data { get => data; - set => SetPropertyAndNotifyOnCompletion(() => ref data, value); + set => SetPropertyAndNotifyOnCompletion(ref data, () => data, value); } } } From 0bd1f92ad00dff22afbc832d197322e22055668c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Mon, 31 Aug 2020 23:46:42 +0200 Subject: [PATCH 11/31] Removed Ioc class and MS.Ex.DI dependency --- .../DependencyInjection/Ioc.cs | 150 ----------------- .../Microsoft.Toolkit.Mvvm.csproj | 4 +- UnitTests/UnitTests.Shared/Mvvm/Test_Ioc.cs | 154 ------------------ .../UnitTests.Shared.projitems | 1 - 4 files changed, 2 insertions(+), 307 deletions(-) delete mode 100644 Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs delete mode 100644 UnitTests/UnitTests.Shared/Mvvm/Test_Ioc.cs diff --git a/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs b/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs deleted file mode 100644 index f8567c0de57..00000000000 --- a/Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs +++ /dev/null @@ -1,150 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using System.Threading; -using Microsoft.Extensions.DependencyInjection; - -#nullable enable - -namespace Microsoft.Toolkit.Mvvm.DependencyInjection -{ - /// - /// A type that facilitates the use of the type. - /// The provides the ability to configure services in a singleton, thread-safe - /// service provider instance, which can then be used to resolve service instances. - /// The first step to use this feature is to declare some services, for instance: - /// - /// public interface ILogger - /// { - /// void Log(string text); - /// } - /// - /// - /// public class ConsoleLogger : ILogger - /// { - /// void Log(string text) => Console.WriteLine(text); - /// } - /// - /// Then the services configuration should then be done at startup, by calling one of - /// the available overloads, like so: - /// - /// Ioc.Default.ConfigureServices(services => - /// { - /// services.AddSingleton<ILogger, Logger>(); - /// }); - /// - /// Finally, you can use the instance (which implements ) - /// to retrieve the service instances from anywhere in your application, by doing as follows: - /// - /// Ioc.Default.GetService<ILogger>().Log("Hello world!"); - /// - /// - public sealed class Ioc : IServiceProvider - { - /// - /// Gets the default instance. - /// - public static Ioc Default { get; } = new Ioc(); - - /// - /// The instance to use, if initialized. - /// - private volatile ServiceProvider? serviceProvider; - - /// - /// Thrown if the current instance has not been configured yet. - object? IServiceProvider.GetService(Type serviceType) - { - // As per section I.12.6.6 of the official CLI ECMA-335 spec: - // "[...] read and write access to properly aligned memory locations no larger than the native - // word size is atomic when all the write accesses to a location are the same size. Atomic writes - // shall alter no bits other than those written. Unless explicit layout control is used [...], - // data elements no larger than the natural word size [...] shall be properly aligned. - // Object references shall be treated as though they are stored in the native word size." - // The field being accessed here is of native int size (reference type), and is only ever accessed - // directly and atomically by a compare exchange instruction (see below), or here. We can therefore - // assume this read is thread safe with respect to accesses to this property or to invocations to one - // of the available configuration methods. So we can just read the field directly and make the necessary - // check with our local copy, without the need of paying the locking overhead from this get accessor. - ServiceProvider? provider = this.serviceProvider; - - if (provider is null) - { - ThrowInvalidOperationExceptionForMissingInitialization(); - } - - return provider!.GetService(serviceType); - } - - /// - /// Initializes the shared instance. - /// - /// The configuration delegate to use to add services. - /// Thrown if the current instance has already been configured. - public void ConfigureServices(Action setup) - { - ConfigureServices(setup, new ServiceProviderOptions()); - } - - /// - /// Initializes the shared instance. - /// - /// The configuration delegate to use to add services. - /// The instance to configure the service provider behaviors. - /// Thrown if the current instance has already been configured. - public void ConfigureServices(Action setup, ServiceProviderOptions options) - { - var collection = new ServiceCollection(); - - setup(collection); - - ConfigureServices(collection, options); - } - - /// - /// Initializes the shared instance. - /// - /// The input instance to use. - /// Thrown if the current instance has already been configured. - public void ConfigureServices(IServiceCollection services) - { - ConfigureServices(services, new ServiceProviderOptions()); - } - - /// - /// Initializes the shared instance. - /// - /// The input instance to use. - /// The instance to configure the service provider behaviors. - /// Thrown if the current instance has already been configured. - public void ConfigureServices(IServiceCollection services, ServiceProviderOptions options) - { - ServiceProvider newServices = services.BuildServiceProvider(options); - - ServiceProvider? oldServices = Interlocked.CompareExchange(ref this.serviceProvider, newServices, null); - - if (!(oldServices is null)) - { - ThrowInvalidOperationExceptionForRepeatedConfiguration(); - } - } - - /// - /// Throws an when the property is used before initialization. - /// - private static void ThrowInvalidOperationExceptionForMissingInitialization() - { - throw new InvalidOperationException("The service provider has not been configured yet"); - } - - /// - /// Throws an when a configuration is attempted more than once. - /// - private static void ThrowInvalidOperationExceptionForRepeatedConfiguration() - { - throw new InvalidOperationException("The default service provider has already been configured"); - } - } -} diff --git a/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj b/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj index b98b400247d..d9e4a16cdea 100644 --- a/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj +++ b/Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj @@ -16,8 +16,9 @@ UWP Toolkit Windows MVVM MVVMToolkit observable Ioc dependency injection services extensions helpers - + + @@ -27,7 +28,6 @@ - diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_Ioc.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_Ioc.cs deleted file mode 100644 index 44fbf61301e..00000000000 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_Ioc.cs +++ /dev/null @@ -1,154 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. -// See the LICENSE file in the project root for more information. - -using System; -using Microsoft.Extensions.DependencyInjection; -using Microsoft.Toolkit.Mvvm.ComponentModel; -using Microsoft.Toolkit.Mvvm.DependencyInjection; -using Microsoft.Toolkit.Mvvm.Messaging; -using Microsoft.VisualStudio.TestTools.UnitTesting; - -namespace UnitTests.Mvvm -{ - [TestClass] - public class Test_Ioc - { - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_ServicesNotConfigured() - { - var ioc = new Ioc(); - - Assert.ThrowsException(() => ioc.GetService()); - } - - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_LambdaInitialization() - { - var ioc = new Ioc(); - - ioc.ConfigureServices(services => - { - services.AddSingleton(); - }); - - var service = ioc.GetRequiredService(); - - Assert.IsNotNull(service); - Assert.IsInstanceOfType(service, typeof(AliceService)); - } - - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_LambdaInitialization_ConcreteType() - { - var ioc = new Ioc(); - - ioc.ConfigureServices(services => - { - services.AddSingleton(); - }); - - var service = ioc.GetRequiredService(); - - Assert.IsNotNull(service); - Assert.IsInstanceOfType(service, typeof(AliceService)); - } - - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_LambdaInitialization_ConstructorInjection() - { - var ioc = new Ioc(); - var messenger = new Messenger(); - - ioc.ConfigureServices(services => - { - services.AddSingleton(); - services.AddSingleton(messenger); - services.AddTransient(); - }); - - var service = ioc.GetRequiredService(); - - Assert.IsNotNull(service); - Assert.IsInstanceOfType(service, typeof(MyRecipient)); - Assert.IsNotNull(service.NameService); - Assert.IsInstanceOfType(service.NameService, typeof(AliceService)); - Assert.IsNotNull(service.MessengerService); - Assert.AreSame(service.MessengerService, messenger); - } - - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_CollectionInitialization() - { - var ioc = new Ioc(); - - var services = new ServiceCollection(); - - services.AddSingleton(); - - ioc.ConfigureServices(services); - - var service = ioc.GetRequiredService(); - - Assert.IsNotNull(service); - Assert.IsInstanceOfType(service, typeof(AliceService)); - } - - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_RepeatedLambdaInitialization() - { - var ioc = new Ioc(); - - ioc.ConfigureServices(services => { }); - - Assert.ThrowsException(() => ioc.ConfigureServices(services => { })); - } - - [TestCategory("Mvvm")] - [TestMethod] - public void Test_Ioc_RepeatedCollectionInitialization() - { - var ioc = new Ioc(); - - var services = new ServiceCollection(); - - ioc.ConfigureServices(services); - - Assert.ThrowsException(() => ioc.ConfigureServices(services)); - } - - public interface INameService - { - string GetName(); - } - - public class BobService : INameService - { - public string GetName() => "Bob"; - } - - public class AliceService : INameService - { - public string GetName() => "Alice"; - } - - public class MyRecipient : ObservableRecipient - { - public MyRecipient(INameService nameService, IMessenger messengerService) - : base(messengerService) - { - NameService = nameService; - } - - public INameService NameService { get; } - - public IMessenger MessengerService => Messenger; - } - } -} diff --git a/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems b/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems index 14147cede7e..2422170a482 100644 --- a/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems +++ b/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems @@ -23,7 +23,6 @@ - From 4144f60dcc8eca6e1529b1f146db9b9de61d341d Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 2 Sep 2020 17:50:52 +0200 Subject: [PATCH 12/31] Removed repeated ErrorsChanged event raise --- .../ComponentModel/ObservableValidator.cs | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 5bf505fc6dc..153a7ded239 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -47,13 +47,15 @@ public void ValidateProperty(object value, [CallerMemberName] string? propertyNa ThrowArgumentNullExceptionForNullPropertyName(); } + bool errorsChanged = false; + // Clear the errors for the specified property, if any if (this.errors.TryGetValue(propertyName!, out List? propertyErrors) && propertyErrors.Count > 0) { propertyErrors.Clear(); - ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); + errorsChanged = true; } List results = new List(); @@ -78,6 +80,14 @@ public void ValidateProperty(object value, [CallerMemberName] string? propertyNa propertyErrors.AddRange(results); } + errorsChanged = true; + } + + // Only raise the event once if needed. This happens either when the target property + // had existing errors and is now valid, or if the validation has failed and there are + // new errors to broadcast, regardless of the previous validation state for the property. + if (errorsChanged) + { ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } } From fed7051317a98e44fb361d722ccdca80980b2133 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 2 Sep 2020 17:56:07 +0200 Subject: [PATCH 13/31] Added null/"" handling in ObservableValidator.GetErrors --- .../ComponentModel/ObservableValidator.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 153a7ded239..09fb2728ec9 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -29,9 +29,15 @@ public abstract class ObservableValidator : ObservableObject, INotifyDataErrorIn public bool HasErrors => this.errors.Count > 0; /// - public IEnumerable GetErrors(string propertyName) + public IEnumerable GetErrors(string? propertyName) { - return this.errors[propertyName]; + if (!(propertyName is null) && + this.errors.TryGetValue(propertyName, out List errors)) + { + return errors; + } + + return null!; } /// From b0f6b719cc66fa936e72057210b763b766158802 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Wed, 2 Sep 2020 23:24:31 +0200 Subject: [PATCH 14/31] Added TaskAccessor API, code refactoring --- .../ComponentModel/ObservableObject.cs | 88 +++++++++++-------- .../Input/AsyncRelayCommand.cs | 4 +- .../Input/AsyncRelayCommand{T}.cs | 4 +- .../Mvvm/Test_ObservableObject.cs | 4 +- 4 files changed, 56 insertions(+), 44 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index a4b786cf668..d43e0fd8316 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -301,37 +301,37 @@ parentExpression.Expression is ConstantExpression instanceExpression && /// Compares the current and new values for a given field (which should be the backing /// field for a property). If the value has changed, raises the /// event, updates the field and then raises the event. - /// The behavior mirrors that of , with the difference being that this method - /// will also monitor the new value of the property (a generic ) and will also + /// The behavior mirrors that of , with the difference being that + /// this method will also monitor the new value of the property (a generic ) and will also /// raise the again for the target property when it completes. /// This can be used to update bindings observing that or any of its properties. + /// This method and its overload specifically rely on the type, which needs + /// to be used in the backing field for the target property. The field doesn't need to be + /// initialized, as this method will take care of doing that automatically. The + /// type also includes an implicit operator, so it can be assigned to any instance directly. /// Here is a sample property declaration using this method: /// - /// private Task myTask; + /// private TaskAccessor<Task> myTask; /// /// public Task MyTask /// { /// get => myTask; - /// private set => SetAndNotifyOnCompletion(ref myTask, () => myTask, value); + /// private set => SetAndNotifyOnCompletion(ref myTask, value); /// } /// /// /// The type of to set and monitor. - /// The field storing the property's value. - /// - /// An returning the field to update. This is needed to be - /// able to raise the to notify the completion of the input task. - /// + /// The field accessor to modify. /// The property's value after the change occurred. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. /// /// The and events are not raised if the current /// and new value for the target property are the same. The return value being only - /// indicates that the new value being assigned to is different than the previous one, + /// indicates that the new value being assigned to is different than the previous one, /// and it does not mean the new instance passed as argument is in any particular state. /// - protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, [CallerMemberName] string? propertyName = null) + protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? taskAccessor, TTask? newValue, [CallerMemberName] string? propertyName = null) where TTask : Task { // We invoke the overload with a callback here to avoid code duplication, and simply pass an empty callback. @@ -340,21 +340,19 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express // instance. This will result in no further allocations after the first time this method is called for a given // generic type. We only pay the cost of the virtual call to the delegate, but this is not performance critical // code and that overhead would still be much lower than the rest of the method anyway, so that's fine. - return SetPropertyAndNotifyOnCompletion(ref field, fieldExpression, newValue, _ => { }, propertyName); + return SetPropertyAndNotifyOnCompletion(ref taskAccessor, newValue, _ => { }, propertyName); } /// /// Compares the current and new values for a given field (which should be the backing /// field for a property). If the value has changed, raises the /// event, updates the field and then raises the event. - /// This method is just like , + /// This method is just like , /// with the difference being an extra parameter with a callback being invoked /// either immediately, if the new task has already completed or is , or upon completion. /// /// The type of to set and monitor. - /// The field storing the property's value. - /// - /// An returning the field to update. + /// The field accessor to modify. /// The property's value after the change occurred. /// A callback to invoke to update the property value. /// (optional) The name of the property that changed. @@ -363,10 +361,13 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express /// The and events are not raised /// if the current and new value for the target property are the same. /// - protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Expression> fieldExpression, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) + protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? taskAccessor, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) where TTask : Task { - if (ReferenceEquals(field, newValue)) + // Initialize the task accessor, if none is present + var localAccessor = taskAccessor ??= new TaskAccessor(); + + if (ReferenceEquals(taskAccessor.Value, newValue)) { return false; } @@ -379,7 +380,7 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express OnPropertyChanging(propertyName); - field = newValue; + localAccessor.Value = newValue; OnPropertyChanged(propertyName); @@ -396,16 +397,6 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TTask? field, Express return true; } - // Get the target field to set. This is needed because we can't - // capture the ref field in a closure (for the async method). - if (!((fieldExpression.Body as MemberExpression)?.Member is FieldInfo fieldInfo)) - { - ThrowArgumentExceptionForInvalidFieldExpression(); - - // This is never executed, as the method above always throws - return false; - } - // We use a local async function here so that the main method can // remain synchronous and return a value that can be immediately // used by the caller. This mirrors Set(ref T, T, string). @@ -425,10 +416,8 @@ async void MonitorTask() { } - TTask? currentTask = (TTask?)fieldInfo.GetValue(this); - // Only notify if the property hasn't changed - if (ReferenceEquals(newValue, currentTask)) + if (ReferenceEquals(newValue, localAccessor.Value)) { OnPropertyChanged(propertyName); } @@ -442,19 +431,42 @@ async void MonitorTask() } /// - /// Throws an when a given is invalid for a property. + /// A wrapping class that can hold a value. /// - private static void ThrowArgumentExceptionForInvalidPropertyExpression() + /// The type of value to store. + protected sealed class TaskAccessor + where TTask : Task { - throw new ArgumentException("The given expression must be in the form () => MyModel.MyProperty"); + /// + /// Initializes a new instance of the class. + /// + internal TaskAccessor() + { + } + +#pragma warning disable SA1401 // Fields should be private + /// + /// Gets or sets the wrapped value. + /// + internal TTask? Value; +#pragma warning restore SA1401 + + /// + /// Unwraps the value stored in the current instance. + /// + /// The input instance. + public static implicit operator TTask?(TaskAccessor? accessor) + { + return accessor?.Value; + } } /// - /// Throws an when a given is invalid for a property field. + /// Throws an when a given is invalid for a property. /// - private static void ThrowArgumentExceptionForInvalidFieldExpression() + private static void ThrowArgumentExceptionForInvalidPropertyExpression() { - throw new ArgumentException("The given expression must be in the form () => field"); + throw new ArgumentException("The given expression must be in the form () => MyModel.MyProperty"); } } } \ No newline at end of file diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs index 471fff34169..2ba5598f5b8 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs @@ -83,7 +83,7 @@ public AsyncRelayCommand(Func cancelableExecute, Func? executionTask; /// public Task? ExecutionTask @@ -91,7 +91,7 @@ public Task? ExecutionTask get => this.executionTask; private set { - if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, () => this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) + if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) { OnPropertyChanged(nameof(IsRunning)); } diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs index 2a265f0752e..b9e04fb2be4 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs @@ -83,7 +83,7 @@ public AsyncRelayCommand(Func cancelableExecute, Fun this.canExecute = canExecute; } - private Task? executionTask; + private TaskAccessor? executionTask; /// public Task? ExecutionTask @@ -91,7 +91,7 @@ public Task? ExecutionTask get => this.executionTask; private set { - if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, () => this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) + if (SetPropertyAndNotifyOnCompletion(ref this.executionTask, value, _ => OnPropertyChanged(nameof(IsRunning)))) { OnPropertyChanged(nameof(IsRunning)); } diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs index 89e70171139..5de4cb18a6e 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs @@ -221,12 +221,12 @@ static async Task TestAsync(Action> callback) public class SampleModelWithTask : ObservableObject { - private Task data; + private TaskAccessor> data; public Task Data { get => data; - set => SetPropertyAndNotifyOnCompletion(ref data, () => data, value); + set => SetPropertyAndNotifyOnCompletion(ref data, value); } } } From dd4fd4c83f2ec0e88844ec7c4fe460e6feae332b Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 3 Sep 2020 02:03:06 +0200 Subject: [PATCH 15/31] Hidden Task/Task type from TaskNotifier --- .../ComponentModel/ObservableObject.cs | 167 ++++++++++++++---- .../Input/AsyncRelayCommand.cs | 2 +- .../Input/AsyncRelayCommand{T}.cs | 2 +- .../Mvvm/Test_ObservableObject.cs | 2 +- 4 files changed, 138 insertions(+), 35 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index d43e0fd8316..60f5f44d18f 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -305,13 +305,13 @@ parentExpression.Expression is ConstantExpression instanceExpression && /// this method will also monitor the new value of the property (a generic ) and will also /// raise the again for the target property when it completes. /// This can be used to update bindings observing that or any of its properties. - /// This method and its overload specifically rely on the type, which needs + /// This method and its overload specifically rely on the type, which needs /// to be used in the backing field for the target property. The field doesn't need to be - /// initialized, as this method will take care of doing that automatically. The + /// initialized, as this method will take care of doing that automatically. The /// type also includes an implicit operator, so it can be assigned to any instance directly. /// Here is a sample property declaration using this method: /// - /// private TaskAccessor<Task> myTask; + /// private TaskNotifier myTask; /// /// public Task MyTask /// { @@ -320,19 +320,17 @@ parentExpression.Expression is ConstantExpression instanceExpression && /// } /// /// - /// The type of to set and monitor. - /// The field accessor to modify. + /// The field notifier to modify. /// The property's value after the change occurred. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. /// /// The and events are not raised if the current /// and new value for the target property are the same. The return value being only - /// indicates that the new value being assigned to is different than the previous one, - /// and it does not mean the new instance passed as argument is in any particular state. + /// indicates that the new value being assigned to is different than the previous one, + /// and it does not mean the new instance passed as argument is in any particular state. /// - protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? taskAccessor, TTask? newValue, [CallerMemberName] string? propertyName = null) - where TTask : Task + protected bool SetPropertyAndNotifyOnCompletion(ref TaskNotifier? taskNotifier, Task? newValue, [CallerMemberName] string? propertyName = null) { // We invoke the overload with a callback here to avoid code duplication, and simply pass an empty callback. // The lambda expression here is transformed by the C# compiler into an empty closure class with a @@ -340,19 +338,18 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? // instance. This will result in no further allocations after the first time this method is called for a given // generic type. We only pay the cost of the virtual call to the delegate, but this is not performance critical // code and that overhead would still be much lower than the rest of the method anyway, so that's fine. - return SetPropertyAndNotifyOnCompletion(ref taskAccessor, newValue, _ => { }, propertyName); + return SetPropertyAndNotifyOnCompletion(taskNotifier ??= new TaskNotifier(), newValue, _ => { }, propertyName); } /// /// Compares the current and new values for a given field (which should be the backing /// field for a property). If the value has changed, raises the /// event, updates the field and then raises the event. - /// This method is just like , + /// This method is just like , /// with the difference being an extra parameter with a callback being invoked /// either immediately, if the new task has already completed or is , or upon completion. /// - /// The type of to set and monitor. - /// The field accessor to modify. + /// The field notifier to modify. /// The property's value after the change occurred. /// A callback to invoke to update the property value. /// (optional) The name of the property that changed. @@ -361,13 +358,86 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? /// The and events are not raised /// if the current and new value for the target property are the same. /// - protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? taskAccessor, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) - where TTask : Task + protected bool SetPropertyAndNotifyOnCompletion(ref TaskNotifier? taskNotifier, Task? newValue, Action callback, [CallerMemberName] string? propertyName = null) + { + return SetPropertyAndNotifyOnCompletion(taskNotifier ??= new TaskNotifier(), newValue, callback, propertyName); + } + + /// + /// Compares the current and new values for a given field (which should be the backing + /// field for a property). If the value has changed, raises the + /// event, updates the field and then raises the event. + /// The behavior mirrors that of , with the difference being that + /// this method will also monitor the new value of the property (a generic ) and will also + /// raise the again for the target property when it completes. + /// This can be used to update bindings observing that or any of its properties. + /// This method and its overload specifically rely on the type, which needs + /// to be used in the backing field for the target property. The field doesn't need to be + /// initialized, as this method will take care of doing that automatically. The + /// type also includes an implicit operator, so it can be assigned to any instance directly. + /// Here is a sample property declaration using this method: + /// + /// private TaskNotifier<int> myTask; + /// + /// public Task<int> MyTask + /// { + /// get => myTask; + /// private set => SetAndNotifyOnCompletion(ref myTask, value); + /// } + /// + /// + /// The type of result for the to set and monitor. + /// The field notifier to modify. + /// The property's value after the change occurred. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + /// + /// The and events are not raised if the current + /// and new value for the target property are the same. The return value being only + /// indicates that the new value being assigned to is different than the previous one, + /// and it does not mean the new instance passed as argument is in any particular state. + /// + protected bool SetPropertyAndNotifyOnCompletion(ref TaskNotifier? taskNotifier, Task? newValue, [CallerMemberName] string? propertyName = null) { - // Initialize the task accessor, if none is present - var localAccessor = taskAccessor ??= new TaskAccessor(); + return SetPropertyAndNotifyOnCompletion(taskNotifier ??= new TaskNotifier(), newValue, _ => { }, propertyName); + } - if (ReferenceEquals(taskAccessor.Value, newValue)) + /// + /// Compares the current and new values for a given field (which should be the backing + /// field for a property). If the value has changed, raises the + /// event, updates the field and then raises the event. + /// This method is just like , + /// with the difference being an extra parameter with a callback being invoked + /// either immediately, if the new task has already completed or is , or upon completion. + /// + /// The type of result for the to set and monitor. + /// The field notifier to modify. + /// The property's value after the change occurred. + /// A callback to invoke to update the property value. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + /// + /// The and events are not raised + /// if the current and new value for the target property are the same. + /// + protected bool SetPropertyAndNotifyOnCompletion(ref TaskNotifier? taskNotifier, Task? newValue, Action?> callback, [CallerMemberName] string? propertyName = null) + { + return SetPropertyAndNotifyOnCompletion(taskNotifier ??= new TaskNotifier(), newValue, callback, propertyName); + } + + /// + /// Implements the notification logic for the related methods. + /// + /// The type of to set and monitor. + /// The field notifier. + /// The property's value after the change occurred. + /// A callback to invoke to update the property value. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + private bool SetPropertyAndNotifyOnCompletion(ITaskNotifier taskNotifier, TTask? newValue, Action callback, [CallerMemberName] string? propertyName = null) + where TTask : Task + { + if (ReferenceEquals(taskNotifier.Task, newValue)) { return false; } @@ -380,7 +450,7 @@ protected bool SetPropertyAndNotifyOnCompletion(ref TaskAccessor? OnPropertyChanging(propertyName); - localAccessor.Value = newValue; + taskNotifier.Task = newValue; OnPropertyChanged(propertyName); @@ -417,7 +487,7 @@ async void MonitorTask() } // Only notify if the property hasn't changed - if (ReferenceEquals(newValue, localAccessor.Value)) + if (ReferenceEquals(newValue, taskNotifier.Task)) { OnPropertyChanged(propertyName); } @@ -431,33 +501,66 @@ async void MonitorTask() } /// - /// A wrapping class that can hold a value. + /// An interface for task notifiers of a specified type. /// /// The type of value to store. - protected sealed class TaskAccessor + private interface ITaskNotifier where TTask : Task { /// - /// Initializes a new instance of the class. + /// Gets or sets the wrapped value. /// - internal TaskAccessor() + TTask? Task { get; set; } + } + + /// + /// A wrapping class that can hold a value. + /// + protected sealed class TaskNotifier : ITaskNotifier + { + /// + /// Initializes a new instance of the class. + /// + internal TaskNotifier() + { + } + + /// + Task? ITaskNotifier.Task { get; set; } + + /// + /// Unwraps the value stored in the current instance. + /// + /// The input instance. + public static implicit operator Task?(TaskNotifier? notifier) { + return Unsafe.As>(notifier)?.Task; } + } -#pragma warning disable SA1401 // Fields should be private + /// + /// A wrapping class that can hold a value. + /// + /// The type of value for the wrapped instance. + protected sealed class TaskNotifier : ITaskNotifier> + { /// - /// Gets or sets the wrapped value. + /// Initializes a new instance of the class. /// - internal TTask? Value; -#pragma warning restore SA1401 + internal TaskNotifier() + { + } + + /// + Task? ITaskNotifier>.Task { get; set; } /// - /// Unwraps the value stored in the current instance. + /// Unwraps the value stored in the current instance. /// - /// The input instance. - public static implicit operator TTask?(TaskAccessor? accessor) + /// The input instance. + public static implicit operator Task?(TaskNotifier? notifier) { - return accessor?.Value; + return Unsafe.As>>(notifier)?.Task; } } diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs index 2ba5598f5b8..d7edbca8e05 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand.cs @@ -83,7 +83,7 @@ public AsyncRelayCommand(Func cancelableExecute, Func? executionTask; + private TaskNotifier? executionTask; /// public Task? ExecutionTask diff --git a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs index b9e04fb2be4..1acf8e72a23 100644 --- a/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs +++ b/Microsoft.Toolkit.Mvvm/Input/AsyncRelayCommand{T}.cs @@ -83,7 +83,7 @@ public AsyncRelayCommand(Func cancelableExecute, Fun this.canExecute = canExecute; } - private TaskAccessor? executionTask; + private TaskNotifier? executionTask; /// public Task? ExecutionTask diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs index 5de4cb18a6e..a8d44e1179f 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs @@ -221,7 +221,7 @@ static async Task TestAsync(Action> callback) public class SampleModelWithTask : ObservableObject { - private TaskAccessor> data; + private TaskNotifier data; public Task Data { From 51dd094c73bf3be8f666e6674827ea22d2e8d5ca Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 3 Sep 2020 12:35:20 +0200 Subject: [PATCH 16/31] Minor codegen improvements in TaskNotifier --- .../ComponentModel/ObservableObject.cs | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index 60f5f44d18f..52351551dc3 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -487,7 +487,7 @@ async void MonitorTask() } // Only notify if the property hasn't changed - if (ReferenceEquals(newValue, taskNotifier.Task)) + if (ReferenceEquals(taskNotifier.Task, newValue)) { OnPropertyChanged(propertyName); } @@ -525,8 +525,14 @@ internal TaskNotifier() { } + private Task? task; + /// - Task? ITaskNotifier.Task { get; set; } + Task? ITaskNotifier.Task + { + get => this.task; + set => this.task = value; + } /// /// Unwraps the value stored in the current instance. @@ -534,7 +540,7 @@ internal TaskNotifier() /// The input instance. public static implicit operator Task?(TaskNotifier? notifier) { - return Unsafe.As>(notifier)?.Task; + return notifier?.task; } } @@ -551,8 +557,14 @@ internal TaskNotifier() { } + private Task? task; + /// - Task? ITaskNotifier>.Task { get; set; } + Task? ITaskNotifier>.Task + { + get => this.task; + set => this.task = value; + } /// /// Unwraps the value stored in the current instance. @@ -560,7 +572,7 @@ internal TaskNotifier() /// The input instance. public static implicit operator Task?(TaskNotifier? notifier) { - return Unsafe.As>>(notifier)?.Task; + return notifier?.task; } } From 434a128a0974f2f6f915721e010e1a1233e9dd14 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 3 Sep 2020 12:55:44 +0200 Subject: [PATCH 17/31] Minor codegen improvement in SetProperty with Action --- .../ComponentModel/ObservableObject.cs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index a4b786cf668..4c4b020d145 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -136,7 +136,19 @@ protected bool SetProperty(ref T field, T newValue, IEqualityComparer comp /// protected bool SetProperty(T oldValue, T newValue, Action callback, [CallerMemberName] string? propertyName = null) { - return SetProperty(oldValue, newValue, EqualityComparer.Default, callback, propertyName); + // We avoid calling the overload again to ensure the comparison is inlined + if (EqualityComparer.Default.Equals(oldValue, newValue)) + { + return false; + } + + OnPropertyChanging(propertyName); + + callback(newValue); + + OnPropertyChanged(propertyName); + + return true; } /// From be877f2c5b5dc185d1701492453093037abf09ac Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 3 Sep 2020 20:09:34 +0200 Subject: [PATCH 18/31] Speed improvements in wrapping SetProperty --- .../ComponentModel/ObservableObject.cs | 108 ++++++------------ .../ComponentModel/ObservableRecipient.cs | 33 +++--- .../Mvvm/Test_ObservableObject.cs | 4 +- 3 files changed, 58 insertions(+), 87 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index 4c4b020d145..3fbb4bb5aca 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -123,6 +123,11 @@ protected bool SetProperty(ref T field, T newValue, IEqualityComparer comp /// This overload is much less efficient than and it /// should only be used when the former is not viable (eg. when the target property being /// updated does not directly expose a backing field that can be passed by reference). + /// For performance reasons, it is recommended to use a stateful callback if possible through + /// the whenever possible + /// instead of this overload, as that will allow the C# compiler to cache the input callback and + /// reduce the memory allocations. More info on that overload are available in the related XML + /// docs. This overload is here for completeness and in cases where that is not applicable. /// /// The type of the property that changed. /// The current property value. @@ -209,33 +214,43 @@ protected bool SetProperty(T oldValue, T newValue, IEqualityComparer compa /// public string Name /// { /// get => Model.Name; - /// set => Set(() => Model.Name, value); + /// set => Set(Model.Name, value, Model, (model, name) => model.Name = name); /// } /// } /// /// This way we can then use the wrapping object in our application, and all those "proxy" properties will /// also raise notifications when changed. Note that this method is not meant to be a replacement for - /// , which offers better performance and less memory usage. Only use this - /// overload when relaying properties to a model that doesn't support notifications, and only if you can't - /// implement notifications to that model directly (eg. by having it inherit from ). + /// , and it should only be used when relaying properties to a model that + /// doesn't support notifications, and only if you can't implement notifications to that model directly (eg. by having + /// it inherit from ). The syntax relies on passing the target model and a stateless callback + /// to allow the C# compiler to cache the function, which results in much better performance and no memory usage. /// - /// The type of property to set. - /// An returning the property to update. + /// The type of model whose property (or field) to set. + /// The type of property (or field) to set. + /// The current property value. /// The property's value after the change occurred. + /// The model + /// The callback to invoke to set the target property value, if a change has occurred. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. /// - /// The and events are not raised - /// if the current and new value for the target property are the same. Additionally, - /// must return a property from a model that is stored as another property in the current instance. - /// This method only supports one level of indirection: can only - /// be used to access properties of a model that is directly stored as a property of the current instance. - /// Additionally, this method can only be used if the wrapped item is a reference type. + /// The and events are not + /// raised if the current and new value for the target property are the same. /// - /// Thrown when is not valid. - protected bool SetProperty(Expression> propertyExpression, T newValue, [CallerMemberName] string? propertyName = null) + protected bool SetProperty(T oldValue, T newValue, TModel model, Action callback, [CallerMemberName] string? propertyName = null) { - return SetProperty(propertyExpression, newValue, EqualityComparer.Default, out _, propertyName); + if (EqualityComparer.Default.Equals(oldValue, newValue)) + { + return false; + } + + OnPropertyChanging(propertyName); + + callback(model, newValue); + + OnPropertyChanged(propertyName); + + return true; } /// @@ -243,58 +258,19 @@ protected bool SetProperty(Expression> propertyExpression, T newValue /// raises the event, updates the property and then raises the /// event. The behavior mirrors that of , /// with the difference being that this method is used to relay properties from a wrapped model in the - /// current instance. See additional notes about this overload in . - /// - /// The type of property to set. - /// An returning the property to update. - /// The property's value after the change occurred. - /// The instance to use to compare the input values. - /// (optional) The name of the property that changed. - /// if the property was changed, otherwise. - /// Thrown when is not valid. - protected bool SetProperty(Expression> propertyExpression, T newValue, IEqualityComparer comparer, [CallerMemberName] string? propertyName = null) - { - return SetProperty(propertyExpression, newValue, comparer, out _, propertyName); - } - - /// - /// Implements the shared logic for + /// current instance. See additional notes about this overload in . /// - /// The type of property to set. - /// An returning the property to update. + /// The type of model whose property (or field) to set. + /// The type of property (or field) to set. + /// The current property value. /// The property's value after the change occurred. /// The instance to use to compare the input values. - /// The resulting initial value for the target property. + /// The model + /// The callback to invoke to set the target property value, if a change has occurred. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. - /// Thrown when is not valid. - private protected bool SetProperty(Expression> propertyExpression, T newValue, IEqualityComparer comparer, out T oldValue, [CallerMemberName] string? propertyName = null) + protected bool SetProperty(T oldValue, T newValue, IEqualityComparer comparer, TModel model, Action callback, [CallerMemberName] string? propertyName = null) { - PropertyInfo? parentPropertyInfo; - FieldInfo? parentFieldInfo = null; - - // Get the target property info - if (!(propertyExpression.Body is MemberExpression targetExpression && - targetExpression.Member is PropertyInfo targetPropertyInfo && - targetExpression.Expression is MemberExpression parentExpression && - (!((parentPropertyInfo = parentExpression.Member as PropertyInfo) is null) || - !((parentFieldInfo = parentExpression.Member as FieldInfo) is null)) && - parentExpression.Expression is ConstantExpression instanceExpression && - instanceExpression.Value is object instance)) - { - ThrowArgumentExceptionForInvalidPropertyExpression(); - - // This is never executed, as the method above always throws - oldValue = default!; - - return false; - } - - object parent = parentPropertyInfo is null - ? parentFieldInfo!.GetValue(instance) - : parentPropertyInfo.GetValue(instance); - oldValue = (T)targetPropertyInfo.GetValue(parent); - if (comparer.Equals(oldValue, newValue)) { return false; @@ -302,7 +278,7 @@ parentExpression.Expression is ConstantExpression instanceExpression && OnPropertyChanging(propertyName); - targetPropertyInfo.SetValue(parent, newValue); + callback(model, newValue); OnPropertyChanged(propertyName); @@ -453,14 +429,6 @@ async void MonitorTask() return true; } - /// - /// Throws an when a given is invalid for a property. - /// - private static void ThrowArgumentExceptionForInvalidPropertyExpression() - { - throw new ArgumentException("The given expression must be in the form () => MyModel.MyProperty"); - } - /// /// Throws an when a given is invalid for a property field. /// diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs index 5c4ba934e60..a52a1dfbf9c 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs @@ -9,7 +9,6 @@ using System; using System.Collections.Generic; -using System.Linq.Expressions; using System.Runtime.CompilerServices; using Microsoft.Toolkit.Mvvm.Messaging; using Microsoft.Toolkit.Mvvm.Messaging.Messages; @@ -237,42 +236,46 @@ protected bool SetProperty(T oldValue, T newValue, IEqualityComparer compa /// Compares the current and new values for a given nested property. If the value has changed, /// raises the event, updates the property and then raises the /// event. The behavior mirrors that of - /// , with the difference being that this + /// , with the difference being that this /// method is used to relay properties from a wrapped model in the current instance. For more info, see the docs for - /// . + /// . /// - /// The type of property to set. - /// An returning the property to update. + /// The type of model whose property (or field) to set. + /// The type of property (or field) to set. + /// The current property value. /// The property's value after the change occurred. + /// The model + /// The callback to invoke to set the target property value, if a change has occurred. /// If , will also be invoked. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. - /// Thrown when is not valid. - protected bool SetProperty(Expression> propertyExpression, T newValue, bool broadcast, [CallerMemberName] string? propertyName = null) + protected bool SetProperty(T oldValue, T newValue, TModel model, Action callback, bool broadcast, [CallerMemberName] string? propertyName = null) { - return SetProperty(propertyExpression, newValue, EqualityComparer.Default, broadcast, propertyName); + return SetProperty(oldValue, newValue, EqualityComparer.Default, model, callback, broadcast, propertyName); } /// /// Compares the current and new values for a given nested property. If the value has changed, /// raises the event, updates the property and then raises the /// event. The behavior mirrors that of - /// , + /// , /// with the difference being that this method is used to relay properties from a wrapped model in the /// current instance. For more info, see the docs for - /// . + /// . /// - /// The type of property to set. - /// An returning the property to update. + /// The type of model whose property (or field) to set. + /// The type of property (or field) to set. + /// The current property value. /// The property's value after the change occurred. /// The instance to use to compare the input values. + /// The model + /// The callback to invoke to set the target property value, if a change has occurred. /// If , will also be invoked. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. - /// Thrown when is not valid. - protected bool SetProperty(Expression> propertyExpression, T newValue, IEqualityComparer comparer, bool broadcast, [CallerMemberName] string? propertyName = null) + protected bool SetProperty(T oldValue, T newValue, IEqualityComparer comparer, TModel model, Action callback, bool broadcast, [CallerMemberName] string? propertyName = null) { - bool propertyChanged = SetProperty(propertyExpression, newValue, comparer, out T oldValue, propertyName); + bool propertyChanged = SetProperty(oldValue, newValue, comparer, model, callback, propertyName); if (propertyChanged && broadcast) { diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs index 89e70171139..b22997eaa04 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableObject.cs @@ -114,7 +114,7 @@ public WrappingModelWithProperty(Person person) public string Name { get => Person.Name; - set => SetProperty(() => Person.Name, value); + set => SetProperty(Person.Name, value, Person, (person, name) => person.Name = name); } } @@ -164,7 +164,7 @@ public WrappingModelWithField(Person person) public string Name { get => this.person.Name; - set => SetProperty(() => this.person.Name, value); + set => SetProperty(this.person.Name, value, this.person, (person, name) => person.Name = name); } } From a8293f72200221303aa715df102de3efafb53afb Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 3 Sep 2020 21:35:10 +0200 Subject: [PATCH 19/31] Minor codegen improvements for EqualityComparer --- .../ComponentModel/ObservableRecipient.cs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs index a52a1dfbf9c..80249075d38 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs @@ -203,7 +203,14 @@ protected bool SetProperty(ref T field, T newValue, IEqualityComparer comp /// protected bool SetProperty(T oldValue, T newValue, Action callback, bool broadcast, [CallerMemberName] string? propertyName = null) { - return SetProperty(oldValue, newValue, EqualityComparer.Default, callback, broadcast, propertyName); + bool propertyChanged = SetProperty(oldValue, newValue, callback, propertyName); + + if (propertyChanged && broadcast) + { + Broadcast(oldValue, newValue, propertyName); + } + + return propertyChanged; } /// @@ -249,9 +256,16 @@ protected bool SetProperty(T oldValue, T newValue, IEqualityComparer compa /// If , will also be invoked. /// (optional) The name of the property that changed. /// if the property was changed, otherwise. - protected bool SetProperty(T oldValue, T newValue, TModel model, Action callback, bool broadcast, [CallerMemberName] string? propertyName = null) + protected bool SetProperty(T oldValue, T newValue, TModel model, Action callback, bool broadcast, [CallerMemberName] string? propertyName = null) { - return SetProperty(oldValue, newValue, EqualityComparer.Default, model, callback, broadcast, propertyName); + bool propertyChanged = SetProperty(oldValue, newValue, model, callback, propertyName); + + if (propertyChanged && broadcast) + { + Broadcast(oldValue, newValue, propertyName); + } + + return propertyChanged; } /// From 97ab1d8ba90652fa7c338a646fab8e3cda79d22c Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 4 Sep 2020 12:07:04 +0200 Subject: [PATCH 20/31] Removed unnecessary using directives --- Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs index 4b2f37b04d5..e1f2f36df6f 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs @@ -10,8 +10,6 @@ using System; using System.Collections.Generic; using System.ComponentModel; -using System.Linq.Expressions; -using System.Reflection; using System.Runtime.CompilerServices; using System.Threading.Tasks; From dd8c7b4c49e58ffd17e02a0dbf4fed133759aa51 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 4 Sep 2020 18:24:53 +0200 Subject: [PATCH 21/31] Bug fixes and minor optimization to ObservableValidator --- .../ComponentModel/ObservableValidator.cs | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 09fb2728ec9..e7e0da3d1b7 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -22,11 +22,18 @@ public abstract class ObservableValidator : ObservableObject, INotifyDataErrorIn /// private readonly Dictionary> errors = new Dictionary>(); + /// + /// Indicates the total number of properties with errors (not total errors). + /// This is used to allow to operate in O(1) time, as it can just + /// check whether this value is not 0 instead of having to traverse . + /// + private int totalErrors; + /// public event EventHandler? ErrorsChanged; /// - public bool HasErrors => this.errors.Count > 0; + public bool HasErrors => this.totalErrors > 0; /// public IEnumerable GetErrors(string? propertyName) @@ -46,7 +53,7 @@ public IEnumerable GetErrors(string? propertyName) /// The value to test for the specified property. /// The name of the property to validate. /// Thrown when is . - public void ValidateProperty(object value, [CallerMemberName] string? propertyName = null) + protected void ValidateProperty(object value, [CallerMemberName] string? propertyName = null) { if (propertyName is null) { @@ -67,11 +74,23 @@ public void ValidateProperty(object value, [CallerMemberName] string? propertyNa List results = new List(); // Validate the property - if (!Validator.TryValidateProperty( + if (Validator.TryValidateProperty( value, new ValidationContext(this, null, null) { MemberName = propertyName }, results)) { + if (errorsChanged) + { + this.totalErrors--; + } + } + else + { + if (!errorsChanged) + { + this.totalErrors++; + } + // We can avoid the repeated dictionary lookup if we just check the result of the previous // one here. If the retrieved errors collection is null, we can log the ones produced by // the validation we just performed. We also don't need to create a new list and add them From 96828978ddd44ef1e8d295534ca8763aed1ff9e1 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 4 Sep 2020 19:00:02 +0200 Subject: [PATCH 22/31] Improved support for INotifyDataErrorInfo.GetErrors --- .../ComponentModel/ObservableValidator.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index e7e0da3d1b7..f7526d316bb 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -44,7 +44,12 @@ public IEnumerable GetErrors(string? propertyName) return errors; } - return null!; + // The INotifyDataErrorInfo.GetErrors method doesn't specify exactly what to + // return when the input property name is invalid, but given that the return + // type is marked as a non-nullable reference type, here we're returning an + // empty array to respect the contract. We do the same when the input property + // is null as well, as we don't support entity-level errors at the moment. + return Array.Empty(); } /// From 0a7df907f3c63009ee7f36cc5bd45c7e589a5eb4 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 4 Sep 2020 19:00:11 +0200 Subject: [PATCH 23/31] Added tests for ObservableValidator --- .../Mvvm/Test_ObservableValidator.cs | 110 ++++++++++++++++++ .../UnitTests.Shared.projitems | 1 + 2 files changed, 111 insertions(+) create mode 100644 UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs new file mode 100644 index 00000000000..ce1c2501452 --- /dev/null +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs @@ -0,0 +1,110 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Collections.Generic; +using System.ComponentModel; +using System.ComponentModel.DataAnnotations; +using System.Linq; +using Microsoft.Toolkit.Mvvm.ComponentModel; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace UnitTests.Mvvm +{ + [TestClass] + public class Test_ObservableValidator + { + [TestCategory("Mvvm")] + [TestMethod] + public void Test_ObservableValidator_HasErrors() + { + var model = new Person(); + + Assert.IsFalse(model.HasErrors); + + model.Name = "No"; + + Assert.IsTrue(model.HasErrors); + + model.Name = "Valid"; + + Assert.IsFalse(model.HasErrors); + } + + [TestCategory("Mvvm")] + [TestMethod] + public void Test_ObservableValidator_ErrorsChanged() + { + var model = new Person(); + + List<(object Sender, DataErrorsChangedEventArgs Args)> errors = new List<(object, DataErrorsChangedEventArgs)>(); + + model.ErrorsChanged += (s, e) => errors.Add((s, e)); + + model.Name = "Foo"; + + Assert.AreEqual(errors.Count, 1); + Assert.AreSame(errors[0].Sender, model); + Assert.AreEqual(errors[0].Args.PropertyName, nameof(Person.Name)); + + errors.Clear(); + + model.Name = "Bar"; + + Assert.AreEqual(errors.Count, 1); + Assert.AreSame(errors[0].Sender, model); + Assert.AreEqual(errors[0].Args.PropertyName, nameof(Person.Name)); + + errors.Clear(); + + model.Name = "Valid"; + + Assert.AreEqual(errors.Count, 1); + Assert.AreSame(errors[0].Sender, model); + Assert.AreEqual(errors[0].Args.PropertyName, nameof(Person.Name)); + + errors.Clear(); + + model.Name = "This is fine"; + + Assert.AreEqual(errors.Count, 0); + } + + [TestCategory("Mvvm")] + [TestMethod] + public void Test_ObservableValidator_GetErrors() + { + var model = new Person(); + + Assert.AreEqual(model.GetErrors(null).Cast().Count(), 0); + Assert.AreEqual(model.GetErrors(string.Empty).Cast().Count(), 0); + Assert.AreEqual(model.GetErrors("ThereIsntAPropertyWithThisName").Cast().Count(), 0); + Assert.AreEqual(model.GetErrors(nameof(Person.Name)).Cast().Count(), 0); + + model.Name = "Foo"; + + var errors = model.GetErrors(nameof(Person.Name)).Cast().ToArray(); + + Assert.AreEqual(errors.Length, 1); + Assert.AreEqual(errors[0].MemberNames.First(), nameof(Person.Name)); + } + + public class Person : ObservableValidator + { + private string name; + + [MinLength(4)] + [MaxLength(20)] + public string Name + { + get => this.name; + set + { + ValidateProperty(value); + + SetProperty(ref this.name, value); + } + } + } + } +} diff --git a/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems b/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems index 2422170a482..1d958a159c5 100644 --- a/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems +++ b/UnitTests/UnitTests.Shared/UnitTests.Shared.projitems @@ -25,6 +25,7 @@ + From c5cbb8cfd426566a3a71f0be72959b7bd00660ab Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 4 Sep 2020 19:17:49 +0200 Subject: [PATCH 24/31] Added support for entity-level errors --- .../ComponentModel/ObservableValidator.cs | 28 +++++++++++-- .../Mvvm/Test_ObservableValidator.cs | 42 +++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index f7526d316bb..39e96ef6e4d 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -7,6 +7,8 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; +using System.Diagnostics.Contracts; +using System.Linq; using System.Runtime.CompilerServices; namespace Microsoft.Toolkit.Mvvm.ComponentModel @@ -36,10 +38,17 @@ public abstract class ObservableValidator : ObservableObject, INotifyDataErrorIn public bool HasErrors => this.totalErrors > 0; /// + [Pure] public IEnumerable GetErrors(string? propertyName) { - if (!(propertyName is null) && - this.errors.TryGetValue(propertyName, out List errors)) + // Entity-level errors when the target property is null or empty + if (string.IsNullOrEmpty(propertyName)) + { + return this.GetAllErrors(); + } + + // Property-level errors, if any + if (this.errors.TryGetValue(propertyName, out List errors)) { return errors; } @@ -47,11 +56,22 @@ public IEnumerable GetErrors(string? propertyName) // The INotifyDataErrorInfo.GetErrors method doesn't specify exactly what to // return when the input property name is invalid, but given that the return // type is marked as a non-nullable reference type, here we're returning an - // empty array to respect the contract. We do the same when the input property - // is null as well, as we don't support entity-level errors at the moment. + // empty array to respect the contract. This also matches the behavior of + // this method whenever errors for a valid properties are retrieved. return Array.Empty(); } + /// + /// Implements the logic for entity-level errors gathering for . + /// + /// An instance with all the errors in . + [Pure] + [MethodImpl(MethodImplOptions.NoInlining)] + private IEnumerable GetAllErrors() + { + return this.errors.Values.SelectMany(errors => errors); + } + /// /// Validates a property with a specified name and a given input value. /// diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs index ce1c2501452..e7a37486455 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs @@ -87,6 +87,34 @@ public void Test_ObservableValidator_GetErrors() Assert.AreEqual(errors.Length, 1); Assert.AreEqual(errors[0].MemberNames.First(), nameof(Person.Name)); + + Assert.AreEqual(model.GetErrors("ThereIsntAPropertyWithThisName").Cast().Count(), 0); + + errors = model.GetErrors(null).Cast().ToArray(); + + Assert.AreEqual(errors.Length, 1); + Assert.AreEqual(errors[0].MemberNames.First(), nameof(Person.Name)); + + errors = model.GetErrors(string.Empty).Cast().ToArray(); + + Assert.AreEqual(errors.Length, 1); + Assert.AreEqual(errors[0].MemberNames.First(), nameof(Person.Name)); + + model.Age = -1; + + errors = model.GetErrors(null).Cast().ToArray(); + + Assert.AreEqual(errors.Length, 2); + Assert.IsTrue(errors.Any(e => e.MemberNames.First().Equals(nameof(Person.Name)))); + Assert.IsTrue(errors.Any(e => e.MemberNames.First().Equals(nameof(Person.Age)))); + + model.Age = 26; + + errors = model.GetErrors(null).Cast().ToArray(); + + Assert.AreEqual(errors.Length, 1); + Assert.IsTrue(errors.Any(e => e.MemberNames.First().Equals(nameof(Person.Name)))); + Assert.IsFalse(errors.Any(e => e.MemberNames.First().Equals(nameof(Person.Age)))); } public class Person : ObservableValidator @@ -105,6 +133,20 @@ public string Name SetProperty(ref this.name, value); } } + + private int age; + + [Range(0, 100)] + public int Age + { + get => this.age; + set + { + ValidateProperty(value); + + SetProperty(ref this.age, value); + } + } } } } From 4c7b2cf69e82ee76cdf0ca392309be6c25bd1997 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 4 Sep 2020 22:24:03 +0200 Subject: [PATCH 25/31] Suppressed false positive nullability warning string.IsNullOrEmpty lacks annotations on .NET Standard 2.0 --- Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 39e96ef6e4d..2346928feb8 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -48,7 +48,7 @@ public IEnumerable GetErrors(string? propertyName) } // Property-level errors, if any - if (this.errors.TryGetValue(propertyName, out List errors)) + if (this.errors.TryGetValue(propertyName!, out List errors)) { return errors; } From 5aafde6809ae05bd6d0d5de3146a5b24c0188ed6 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Sat, 5 Sep 2020 13:08:04 +0200 Subject: [PATCH 26/31] Added bool return value to ValidateProperty --- .../ComponentModel/ObservableValidator.cs | 12 ++++++++--- .../Mvvm/Test_ObservableValidator.cs | 21 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 2346928feb8..1bfbc4d3549 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -77,8 +77,9 @@ private IEnumerable GetAllErrors() /// /// The value to test for the specified property. /// The name of the property to validate. + /// Whether or not the validation was successful. /// Thrown when is . - protected void ValidateProperty(object value, [CallerMemberName] string? propertyName = null) + protected bool ValidateProperty(object? value, [CallerMemberName] string? propertyName = null) { if (propertyName is null) { @@ -99,10 +100,13 @@ protected void ValidateProperty(object value, [CallerMemberName] string? propert List results = new List(); // Validate the property - if (Validator.TryValidateProperty( + bool isValid = Validator.TryValidateProperty( value, new ValidationContext(this, null, null) { MemberName = propertyName }, - results)) + results); + + // Update the state and/or the errors for the property + if (isValid) { if (errorsChanged) { @@ -140,6 +144,8 @@ protected void ValidateProperty(object value, [CallerMemberName] string? propert { ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } + + return isValid; } /// diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs index e7a37486455..724b7a7d3c7 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs @@ -117,12 +117,28 @@ public void Test_ObservableValidator_GetErrors() Assert.IsFalse(errors.Any(e => e.MemberNames.First().Equals(nameof(Person.Age)))); } + [TestCategory("Mvvm")] + [TestMethod] + public void Test_ObservableValidator_ValidateReturn() + { + var model = new Person(); + + Assert.IsFalse(model.ValidateName(null)); + Assert.IsFalse(model.ValidateName(string.Empty)); + Assert.IsFalse(model.ValidateName("No")); + Assert.IsFalse(model.ValidateName("This text is really, really too long for the target property")); + Assert.IsTrue(model.ValidateName("1234")); + Assert.IsTrue(model.ValidateName("01234567890123456789")); + Assert.IsTrue(model.ValidateName("Hello world")); + } + public class Person : ObservableValidator { private string name; [MinLength(4)] [MaxLength(20)] + [Required] public string Name { get => this.name; @@ -134,6 +150,11 @@ public string Name } } + public bool ValidateName(string value) + { + return ValidateProperty(value, nameof(Name)); + } + private int age; [Range(0, 100)] From 77ed6e8bfdd9e129d2dfb350cbe8e0f673be0c08 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Fri, 11 Sep 2020 17:38:21 +0200 Subject: [PATCH 27/31] Removed unnecessary List allocation --- .../ComponentModel/ObservableValidator.cs | 34 ++++++++----------- 1 file changed, 14 insertions(+), 20 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 1bfbc4d3549..a634d43c280 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -86,24 +86,32 @@ protected bool ValidateProperty(object? value, [CallerMemberName] string? proper ThrowArgumentNullExceptionForNullPropertyName(); } + // Check if the property had already been previously validated, and if so retrieve + // the reusable list of validation errors from the errors dictionary. This list is + // used to add new validation errors below, if any are produced by the validator. + // If the property isn't present in the dictionary, add it now to avoid allocations. + if (!this.errors.TryGetValue(propertyName!, out List? propertyErrors)) + { + propertyErrors = new List(); + + this.errors.Add(propertyName!, propertyErrors); + } + bool errorsChanged = false; // Clear the errors for the specified property, if any - if (this.errors.TryGetValue(propertyName!, out List? propertyErrors) && - propertyErrors.Count > 0) + if (propertyErrors.Count > 0) { propertyErrors.Clear(); errorsChanged = true; } - List results = new List(); - - // Validate the property + // Validate the property, by adding new errors to the existing list bool isValid = Validator.TryValidateProperty( value, new ValidationContext(this, null, null) { MemberName = propertyName }, - results); + propertyErrors); // Update the state and/or the errors for the property if (isValid) @@ -120,20 +128,6 @@ protected bool ValidateProperty(object? value, [CallerMemberName] string? proper this.totalErrors++; } - // We can avoid the repeated dictionary lookup if we just check the result of the previous - // one here. If the retrieved errors collection is null, we can log the ones produced by - // the validation we just performed. We also don't need to create a new list and add them - // to that, as we can directly set the resulting list as value in the mapping. If the - // property already had some other logged errors instead, we can add the new ones as a range. - if (propertyErrors is null) - { - this.errors.Add(propertyName!, results); - } - else - { - propertyErrors.AddRange(results); - } - errorsChanged = true; } From 28b1977bd3f0f88f9e8095adbefe69c9d87b54be Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 24 Sep 2020 11:37:40 +0200 Subject: [PATCH 28/31] Code refactoring in ObservableValidator --- .../ComponentModel/ObservableValidator.cs | 166 +++++++++++++++++- .../Mvvm/Test_ObservableValidator.cs | 47 +++-- 2 files changed, 183 insertions(+), 30 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index a634d43c280..f0580cc39de 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -37,6 +37,167 @@ public abstract class ObservableValidator : ObservableObject, INotifyDataErrorIn /// public bool HasErrors => this.totalErrors > 0; + /// + /// Compares the current and new values for a given property. If the value has changed, + /// raises the event, updates the property with + /// the new value, then raises the event. + /// + /// The type of the property that changed. + /// The field storing the property's value. + /// The property's value after the change occurred. + /// If , will also be validated. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + /// + /// This method is just like , just with the addition + /// of the parameter. If that is set to , the new value will be + /// validated and will be raised if needed. Following the behavior of the base method, + /// the and events + /// are not raised if the current and new value for the target property are the same. + /// + protected bool SetProperty(ref T field, T newValue, bool validate, [CallerMemberName] string? propertyName = null) + { + if (validate) + { + ValidateProperty(newValue, propertyName); + } + + return SetProperty(ref field, newValue, propertyName); + } + + /// + /// Compares the current and new values for a given property. If the value has changed, + /// raises the event, updates the property with + /// the new value, then raises the event. + /// See additional notes about this overload in . + /// + /// The type of the property that changed. + /// The field storing the property's value. + /// The property's value after the change occurred. + /// The instance to use to compare the input values. + /// If , will also be validated. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + protected bool SetProperty(ref T field, T newValue, IEqualityComparer comparer, bool validate, [CallerMemberName] string? propertyName = null) + { + if (validate) + { + ValidateProperty(newValue, propertyName); + } + + return SetProperty(ref field, newValue, comparer, propertyName); + } + + /// + /// Compares the current and new values for a given property. If the value has changed, + /// raises the event, updates the property with + /// the new value, then raises the event. Similarly to + /// the method, this overload should only be + /// used when can't be used directly. + /// + /// The type of the property that changed. + /// The current property value. + /// The property's value after the change occurred. + /// A callback to invoke to update the property value. + /// If , will also be validated. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + /// + /// This method is just like , just with the addition + /// of the parameter. As such, following the behavior of the base method, + /// the and events + /// are not raised if the current and new value for the target property are the same. + /// + protected bool SetProperty(T oldValue, T newValue, Action callback, bool validate, [CallerMemberName] string? propertyName = null) + { + if (validate) + { + ValidateProperty(newValue, propertyName); + } + + return SetProperty(oldValue, newValue, callback, propertyName); + } + + /// + /// Compares the current and new values for a given property. If the value has changed, + /// raises the event, updates the property with + /// the new value, then raises the event. + /// See additional notes about this overload in . + /// + /// The type of the property that changed. + /// The current property value. + /// The property's value after the change occurred. + /// The instance to use to compare the input values. + /// A callback to invoke to update the property value. + /// If , will also be validated. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + protected bool SetProperty(T oldValue, T newValue, IEqualityComparer comparer, Action callback, bool validate, [CallerMemberName] string? propertyName = null) + { + if (validate) + { + ValidateProperty(newValue, propertyName); + } + + return SetProperty(oldValue, newValue, comparer, callback, propertyName); + } + + /// + /// Compares the current and new values for a given nested property. If the value has changed, + /// raises the event, updates the property and then raises the + /// event. The behavior mirrors that of + /// , with the difference being that this + /// method is used to relay properties from a wrapped model in the current instance. For more info, see the docs for + /// . + /// + /// The type of model whose property (or field) to set. + /// The type of property (or field) to set. + /// The current property value. + /// The property's value after the change occurred. + /// The model + /// The callback to invoke to set the target property value, if a change has occurred. + /// If , will also be validated. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + protected bool SetProperty(T oldValue, T newValue, TModel model, Action callback, bool validate, [CallerMemberName] string? propertyName = null) + { + if (validate) + { + ValidateProperty(newValue, propertyName); + } + + return SetProperty(oldValue, newValue, model, callback, propertyName); + } + + /// + /// Compares the current and new values for a given nested property. If the value has changed, + /// raises the event, updates the property and then raises the + /// event. The behavior mirrors that of + /// , + /// with the difference being that this method is used to relay properties from a wrapped model in the + /// current instance. For more info, see the docs for + /// . + /// + /// The type of model whose property (or field) to set. + /// The type of property (or field) to set. + /// The current property value. + /// The property's value after the change occurred. + /// The instance to use to compare the input values. + /// The model + /// The callback to invoke to set the target property value, if a change has occurred. + /// If , will also be validated. + /// (optional) The name of the property that changed. + /// if the property was changed, otherwise. + protected bool SetProperty(T oldValue, T newValue, IEqualityComparer comparer, TModel model, Action callback, bool validate, [CallerMemberName] string? propertyName = null) + { + if (validate) + { + ValidateProperty(newValue, propertyName); + } + + return SetProperty(oldValue, newValue, comparer, model, callback, propertyName); + } + /// [Pure] public IEnumerable GetErrors(string? propertyName) @@ -77,9 +238,8 @@ private IEnumerable GetAllErrors() /// /// The value to test for the specified property. /// The name of the property to validate. - /// Whether or not the validation was successful. /// Thrown when is . - protected bool ValidateProperty(object? value, [CallerMemberName] string? propertyName = null) + private void ValidateProperty(object? value, string? propertyName) { if (propertyName is null) { @@ -138,8 +298,6 @@ protected bool ValidateProperty(object? value, [CallerMemberName] string? proper { ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } - - return isValid; } /// diff --git a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs index 724b7a7d3c7..d5f6e243f40 100644 --- a/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs +++ b/UnitTests/UnitTests.Shared/Mvvm/Test_ObservableValidator.cs @@ -119,17 +119,27 @@ public void Test_ObservableValidator_GetErrors() [TestCategory("Mvvm")] [TestMethod] - public void Test_ObservableValidator_ValidateReturn() + [DataRow(null, false)] + [DataRow("", false)] + [DataRow("No", false)] + [DataRow("This text is really, really too long for the target property", false)] + [DataRow("1234", true)] + [DataRow("01234567890123456789", true)] + [DataRow("Hello world", true)] + public void Test_ObservableValidator_ValidateReturn(string value, bool isValid) { - var model = new Person(); + var model = new Person { Name = value }; + + Assert.AreEqual(model.HasErrors, !isValid); - Assert.IsFalse(model.ValidateName(null)); - Assert.IsFalse(model.ValidateName(string.Empty)); - Assert.IsFalse(model.ValidateName("No")); - Assert.IsFalse(model.ValidateName("This text is really, really too long for the target property")); - Assert.IsTrue(model.ValidateName("1234")); - Assert.IsTrue(model.ValidateName("01234567890123456789")); - Assert.IsTrue(model.ValidateName("Hello world")); + if (isValid) + { + Assert.IsTrue(!model.GetErrors(nameof(Person.Name)).Cast().Any()); + } + else + { + Assert.IsTrue(model.GetErrors(nameof(Person.Name)).Cast().Any()); + } } public class Person : ObservableValidator @@ -142,17 +152,7 @@ public class Person : ObservableValidator public string Name { get => this.name; - set - { - ValidateProperty(value); - - SetProperty(ref this.name, value); - } - } - - public bool ValidateName(string value) - { - return ValidateProperty(value, nameof(Name)); + set => SetProperty(ref this.name, value, true); } private int age; @@ -161,12 +161,7 @@ public bool ValidateName(string value) public int Age { get => this.age; - set - { - ValidateProperty(value); - - SetProperty(ref this.age, value); - } + set => SetProperty(ref this.age, value, true); } } } From e7a482458b9255d594dc4302dd1732c4e22bacc8 Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 24 Sep 2020 11:45:04 +0200 Subject: [PATCH 29/31] Removed tracking field for HasErrors --- .../ComponentModel/ObservableValidator.cs | 46 ++++++++----------- 1 file changed, 19 insertions(+), 27 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index f0580cc39de..fcc3f10b38c 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -24,18 +24,28 @@ public abstract class ObservableValidator : ObservableObject, INotifyDataErrorIn /// private readonly Dictionary> errors = new Dictionary>(); - /// - /// Indicates the total number of properties with errors (not total errors). - /// This is used to allow to operate in O(1) time, as it can just - /// check whether this value is not 0 instead of having to traverse . - /// - private int totalErrors; - /// public event EventHandler? ErrorsChanged; /// - public bool HasErrors => this.totalErrors > 0; + public bool HasErrors + { + get + { + // This uses the value enumerator for Dictionary.ValueCollection, so it doesn't + // allocate. Accessing this property is O(n), but we can stop as soon as we find at least one + // error in the whole entity, and doing this saves 8 bytes in the object size (no fields needed). + foreach (var value in this.errors.Values) + { + if (value.Count > 0) + { + return true; + } + } + + return false; + } + } /// /// Compares the current and new values for a given property. If the value has changed, @@ -273,28 +283,10 @@ private void ValidateProperty(object? value, string? propertyName) new ValidationContext(this, null, null) { MemberName = propertyName }, propertyErrors); - // Update the state and/or the errors for the property - if (isValid) - { - if (errorsChanged) - { - this.totalErrors--; - } - } - else - { - if (!errorsChanged) - { - this.totalErrors++; - } - - errorsChanged = true; - } - // Only raise the event once if needed. This happens either when the target property // had existing errors and is now valid, or if the validation has failed and there are // new errors to broadcast, regardless of the previous validation state for the property. - if (errorsChanged) + if (errorsChanged || !isValid) { ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } From c5a5d5db7635d303c5c267160f045d65c43d334e Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 24 Sep 2020 12:10:49 +0200 Subject: [PATCH 30/31] Fixed build error due to StyleCop --- .../ComponentModel/ObservableValidator.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index fcc3f10b38c..6192af3a51a 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -253,6 +253,11 @@ private void ValidateProperty(object? value, string? propertyName) { if (propertyName is null) { + static void ThrowArgumentNullExceptionForNullPropertyName() + { + throw new ArgumentNullException(nameof(propertyName), "The input property name cannot be null when validating a property"); + } + ThrowArgumentNullExceptionForNullPropertyName(); } @@ -291,13 +296,5 @@ private void ValidateProperty(object? value, string? propertyName) ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } } - - /// - /// Throws an when a property name given as input is . - /// - private static void ThrowArgumentNullExceptionForNullPropertyName() - { - throw new ArgumentNullException("propertyName", "The input property name cannot be null when validating a property"); - } } } \ No newline at end of file From fec4b22d8958b61af6bd33a4cfe9e9663657c36d Mon Sep 17 00:00:00 2001 From: Sergio Pedri Date: Thu, 24 Sep 2020 12:15:37 +0200 Subject: [PATCH 31/31] Code refactoring (not a fan of StyleCop here) --- .../ComponentModel/ObservableValidator.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs index 6192af3a51a..a1c20d82817 100644 --- a/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs +++ b/Microsoft.Toolkit.Mvvm/ComponentModel/ObservableValidator.cs @@ -253,11 +253,6 @@ private void ValidateProperty(object? value, string? propertyName) { if (propertyName is null) { - static void ThrowArgumentNullExceptionForNullPropertyName() - { - throw new ArgumentNullException(nameof(propertyName), "The input property name cannot be null when validating a property"); - } - ThrowArgumentNullExceptionForNullPropertyName(); } @@ -296,5 +291,15 @@ static void ThrowArgumentNullExceptionForNullPropertyName() ErrorsChanged?.Invoke(this, new DataErrorsChangedEventArgs(propertyName)); } } + +#pragma warning disable SA1204 + /// + /// Throws an when a property name given as input is . + /// + private static void ThrowArgumentNullExceptionForNullPropertyName() + { + throw new ArgumentNullException("propertyName", "The input property name cannot be null when validating a property"); + } +#pragma warning restore SA1204 } } \ No newline at end of file