Skip to content

Commit

Permalink
Microsoft.Toolkit.Mvvm package (part 2) (#3413)
Browse files Browse the repository at this point in the history
## Follow up to #3229 
<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

### NOTE
Marking this not as a draft so that the CI will run, but added the "DO NOT MERGE ⚠" tag as we might come up with further improvements to add to this PR before actually deciding to merge this and finalize the package.

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

 - Bugfix 
 - Feature
 - Refactoring (no functional changes, no api changes) 
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->

## Notable changes
<!-- Describe how was this issue resolved or changed? -->
This PR includes some refinements and tweaks to the `Microsoft.Toolkit.Mvvm` package, like:

- Added new `ObservableRecipient.SetProperty` overloads with `Expression<Func<T>>` and `bool broadcast` params
- Fixed a possible bug in `Messenger.Type2.Equals` (just to be extra sure)
- Improved codegen for `ObservableRecipient.Set<T>(ref T, T, string)` (missing `EqualityComparer<T>.Default.Equals` inlining)

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~
- [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~
    - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [X] Contains **NO** breaking changes
  • Loading branch information
msftbot[bot] authored Aug 11, 2020
2 parents f8b78a3 + 3579e24 commit ff7f24d
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 45 deletions.
42 changes: 39 additions & 3 deletions Microsoft.Toolkit.Mvvm/ComponentModel/ObservableObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,26 @@ protected virtual void OnPropertyChanging([CallerMemberName] string? propertyNam
/// </remarks>
protected bool SetProperty<T>(ref T field, T newValue, [CallerMemberName] string? propertyName = null)
{
return SetProperty(ref field, newValue, EqualityComparer<T>.Default, propertyName);
// We duplicate the code here instead of calling the overload because we can't
// guarantee that the invoked SetProperty<T> will be inlined, and we need the JIT
// to be able to see the full EqualityComparer<T>.Default.Equals call, so that
// it'll use the intrinsics version of it and just replace the whole invocation
// with a direct comparison when possible (eg. for primitive numeric types).
// This is the fastest SetProperty<T> overload so we particularly care about
// the codegen quality here, and the code is small and simple enough so that
// duplicating it still doesn't make the whole class harder to maintain.
if (EqualityComparer<T>.Default.Equals(field, newValue))
{
return false;
}

OnPropertyChanging(propertyName);

field = newValue;

OnPropertyChanged(propertyName);

return true;
}

/// <summary>
Expand Down Expand Up @@ -203,7 +222,7 @@ protected bool SetProperty<T>(T oldValue, T newValue, IEqualityComparer<T> compa
/// </remarks>
protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, [CallerMemberName] string? propertyName = null)
{
return SetProperty(propertyExpression, newValue, EqualityComparer<T>.Default, propertyName);
return SetProperty(propertyExpression, newValue, EqualityComparer<T>.Default, out _, propertyName);
}

/// <summary>
Expand All @@ -220,6 +239,21 @@ protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue
/// <param name="propertyName">(optional) The name of the property that changed.</param>
/// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, IEqualityComparer<T> comparer, [CallerMemberName] string? propertyName = null)
{
return SetProperty(propertyExpression, newValue, comparer, out _, propertyName);
}

/// <summary>
/// Implements the shared logic for <see cref="SetProperty{T}(Expression{Func{T}},T,IEqualityComparer{T},string)"/>
/// </summary>
/// <typeparam name="T">The type of property to set.</typeparam>
/// <param name="propertyExpression">An <see cref="Expression{TDelegate}"/> returning the property to update.</param>
/// <param name="newValue">The property's value after the change occurred.</param>
/// <param name="comparer">The <see cref="IEqualityComparer{T}"/> instance to use to compare the input values.</param>
/// <param name="oldValue">The resulting initial value for the target property.</param>
/// <param name="propertyName">(optional) The name of the property that changed.</param>
/// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
private protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, IEqualityComparer<T> comparer, out T oldValue, [CallerMemberName] string? propertyName = null)
{
PropertyInfo? parentPropertyInfo;
FieldInfo? parentFieldInfo = null;
Expand All @@ -236,13 +270,15 @@ parentExpression.Expression is ConstantExpression instanceExpression &&
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);
T oldValue = (T)targetPropertyInfo.GetValue(parent);
oldValue = (T)targetPropertyInfo.GetValue(parent);

if (comparer.Equals(oldValue, newValue))
{
Expand Down
81 changes: 65 additions & 16 deletions Microsoft.Toolkit.Mvvm/ComponentModel/ObservableRecipient.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

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;
Expand Down Expand Up @@ -140,7 +141,18 @@ protected virtual void Broadcast<T>(T oldValue, T newValue, string? propertyName
/// </remarks>
protected bool SetProperty<T>(ref T field, T newValue, bool broadcast, [CallerMemberName] string? propertyName = null)
{
return SetProperty(ref field, newValue, EqualityComparer<T>.Default, broadcast, propertyName);
T oldValue = field;

// We duplicate the code as in the base class here to leverage
// the intrinsics support for EqualityComparer<T>.Default.Equals.
bool propertyChanged = SetProperty(ref field, newValue, propertyName);

if (propertyChanged && broadcast)
{
Broadcast(oldValue, newValue, propertyName);
}

return propertyChanged;
}

/// <summary>
Expand All @@ -158,21 +170,16 @@ protected bool SetProperty<T>(ref T field, T newValue, bool broadcast, [CallerMe
/// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
protected bool SetProperty<T>(ref T field, T newValue, IEqualityComparer<T> comparer, bool broadcast, [CallerMemberName] string? propertyName = null)
{
if (!broadcast)
{
return SetProperty(ref field, newValue, comparer, propertyName);
}

T oldValue = field;

if (SetProperty(ref field, newValue, comparer, propertyName))
bool propertyChanged = SetProperty(ref field, newValue, comparer, propertyName);

if (propertyChanged && broadcast)
{
Broadcast(oldValue, newValue, propertyName);

return true;
}

return false;
return propertyChanged;
}

/// <summary>
Expand Down Expand Up @@ -216,19 +223,61 @@ protected bool SetProperty<T>(T oldValue, T newValue, Action<T> callback, bool b
/// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
protected bool SetProperty<T>(T oldValue, T newValue, IEqualityComparer<T> comparer, Action<T> callback, bool broadcast, [CallerMemberName] string? propertyName = null)
{
if (!broadcast)
bool propertyChanged = SetProperty(oldValue, newValue, comparer, callback, propertyName);

if (propertyChanged && broadcast)
{
return SetProperty(oldValue, newValue, comparer, callback, propertyName);
Broadcast(oldValue, newValue, propertyName);
}

if (SetProperty(oldValue, newValue, comparer, callback, propertyName))
return propertyChanged;
}

/// <summary>
/// Compares the current and new values for a given nested property. If the value has changed,
/// raises the <see cref="ObservableObject.PropertyChanging"/> event, updates the property and then raises the
/// <see cref="ObservableObject.PropertyChanged"/> event. The behavior mirrors that of
/// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,string)"/>, 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
/// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,string)"/>.
/// </summary>
/// <typeparam name="T">The type of property to set.</typeparam>
/// <param name="propertyExpression">An <see cref="Expression{TDelegate}"/> returning the property to update.</param>
/// <param name="newValue">The property's value after the change occurred.</param>
/// <param name="broadcast">If <see langword="true"/>, <see cref="Broadcast{T}"/> will also be invoked.</param>
/// <param name="propertyName">(optional) The name of the property that changed.</param>
/// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, bool broadcast, [CallerMemberName] string? propertyName = null)
{
return SetProperty(propertyExpression, newValue, EqualityComparer<T>.Default, broadcast, propertyName);
}

/// <summary>
/// Compares the current and new values for a given nested property. If the value has changed,
/// raises the <see cref="ObservableObject.PropertyChanging"/> event, updates the property and then raises the
/// <see cref="ObservableObject.PropertyChanged"/> event. The behavior mirrors that of
/// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,IEqualityComparer{T},string)"/>,
/// 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
/// <see cref="ObservableObject.SetProperty{T}(Expression{Func{T}},T,IEqualityComparer{T},string)"/>.
/// </summary>
/// <typeparam name="T">The type of property to set.</typeparam>
/// <param name="propertyExpression">An <see cref="Expression{TDelegate}"/> returning the property to update.</param>
/// <param name="newValue">The property's value after the change occurred.</param>
/// <param name="comparer">The <see cref="IEqualityComparer{T}"/> instance to use to compare the input values.</param>
/// <param name="broadcast">If <see langword="true"/>, <see cref="Broadcast{T}"/> will also be invoked.</param>
/// <param name="propertyName">(optional) The name of the property that changed.</param>
/// <returns><see langword="true"/> if the property was changed, <see langword="false"/> otherwise.</returns>
protected bool SetProperty<T>(Expression<Func<T>> propertyExpression, T newValue, IEqualityComparer<T> comparer, bool broadcast, [CallerMemberName] string? propertyName = null)
{
bool propertyChanged = SetProperty(propertyExpression, newValue, comparer, out T oldValue, propertyName);

if (propertyChanged && broadcast)
{
Broadcast(oldValue, newValue, propertyName);

return true;
}

return false;
return propertyChanged;
}
}
}
2 changes: 1 addition & 1 deletion Microsoft.Toolkit.Mvvm/DependencyInjection/Ioc.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public sealed class Ioc : IServiceProvider
/// <summary>
/// The <see cref="ServiceProvider"/> instance to use, if initialized.
/// </summary>
private ServiceProvider? serviceProvider;
private volatile ServiceProvider? serviceProvider;

/// <inheritdoc/>
object? IServiceProvider.GetService(Type serviceType)
Expand Down
9 changes: 7 additions & 2 deletions Microsoft.Toolkit.Mvvm/Messaging/Messenger.cs
Original file line number Diff line number Diff line change
Expand Up @@ -638,9 +638,14 @@ public Type2(Type tMessage, Type tToken)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool Equals(Type2 other)
{
// We can't just use reference equality, as that's technically not guaranteed
// to work and might fail in very rare cases (eg. with type forwarding between
// different assemblies). Instead, we can use the == operator to compare for
// equality, which still avoids the callvirt overhead of calling Type.Equals,
// and is also implemented as a JIT intrinsic on runtimes such as .NET Core.
return
ReferenceEquals(this.tMessage, other.tMessage) &&
ReferenceEquals(this.tToken, other.tToken);
this.tMessage == other.tMessage &&
this.tToken == other.tToken;
}

/// <inheritdoc/>
Expand Down
51 changes: 31 additions & 20 deletions Microsoft.Toolkit.Mvvm/Messaging/MessengerExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,37 @@ namespace Microsoft.Toolkit.Mvvm.Messaging
public static partial class MessengerExtensions
{
/// <summary>
/// The <see cref="MethodInfo"/> instance associated with <see cref="Register{TMessage,TToken}(IMessenger,IRecipient{TMessage},TToken)"/>.
/// A class that acts as a container to load the <see cref="MethodInfo"/> instance linked to
/// the <see cref="Register{TMessage,TToken}(IMessenger,IRecipient{TMessage},TToken)"/> method.
/// This class is needed to avoid forcing the initialization code in the static constructor to run as soon as
/// the <see cref="MessengerExtensions"/> type is referenced, even if that is done just to use methods
/// that do not actually require this <see cref="MethodInfo"/> instance to be available.
/// We're effectively using this type to leverage the lazy loading of static constructors done by the runtime.
/// </summary>
private static readonly MethodInfo RegisterIRecipientMethodInfo;
private static class MethodInfos
{
/// <summary>
/// Initializes static members of the <see cref="MethodInfos"/> class.
/// </summary>
static MethodInfos()
{
RegisterIRecipient = (
from methodInfo in typeof(MessengerExtensions).GetMethods()
where methodInfo.Name == nameof(Register) &&
methodInfo.IsGenericMethod &&
methodInfo.GetGenericArguments().Length == 2
let parameters = methodInfo.GetParameters()
where parameters.Length == 3 &&
parameters[1].ParameterType.IsGenericType &&
parameters[1].ParameterType.GetGenericTypeDefinition() == typeof(IRecipient<>)
select methodInfo).First();
}

/// <summary>
/// The <see cref="MethodInfo"/> instance associated with <see cref="Register{TMessage,TToken}(IMessenger,IRecipient{TMessage},TToken)"/>.
/// </summary>
public static readonly MethodInfo RegisterIRecipient;
}

/// <summary>
/// A class that acts as a static container to associate a <see cref="ConditionalWeakTable{TKey,TValue}"/> instance to each
Expand All @@ -39,23 +67,6 @@ public static readonly ConditionalWeakTable<Type, Action<IMessenger, object, TTo
= new ConditionalWeakTable<Type, Action<IMessenger, object, TToken>[]>();
}

/// <summary>
/// Initializes static members of the <see cref="MessengerExtensions"/> class.
/// </summary>
static MessengerExtensions()
{
RegisterIRecipientMethodInfo = (
from methodInfo in typeof(MessengerExtensions).GetMethods()
where methodInfo.Name == nameof(Register) &&
methodInfo.IsGenericMethod &&
methodInfo.GetGenericArguments().Length == 2
let parameters = methodInfo.GetParameters()
where parameters.Length == 3 &&
parameters[1].ParameterType.IsGenericType &&
parameters[1].ParameterType.GetGenericTypeDefinition() == typeof(IRecipient<>)
select methodInfo).First();
}

/// <summary>
/// Checks whether or not a given recipient has already been registered for a message.
/// </summary>
Expand Down Expand Up @@ -110,7 +121,7 @@ from interfaceType in type.GetInterfaces()
where interfaceType.IsGenericType &&
interfaceType.GetGenericTypeDefinition() == typeof(IRecipient<>)
let messageType = interfaceType.GenericTypeArguments[0]
let registrationMethod = RegisterIRecipientMethodInfo.MakeGenericMethod(messageType, typeof(TToken))
let registrationMethod = MethodInfos.RegisterIRecipient.MakeGenericMethod(messageType, typeof(TToken))
let registrationAction = GetRegistrationAction(type, registrationMethod)
select registrationAction).ToArray();
}
Expand Down
6 changes: 3 additions & 3 deletions Microsoft.Toolkit.Mvvm/Microsoft.Toolkit.Mvvm.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,16 @@
<TargetFrameworks>netstandard2.0;netstandard2.1</TargetFrameworks>
<LangVersion>8.0</LangVersion>
<Nullable>enable</Nullable>
<Title>Windows Community Toolkit Mvvm .NET Standard</Title>
<Title>Windows Community Toolkit MVVM Toolkit</Title>
<Description>
This package includes Mvvm .NET Standard code only helpers such as:
This package includes a .NET Standard MVVM library with helpers such as:
- ObservableObject: a base class for objects implementing the INotifyPropertyChanged interface.
- ObservableRecipient: a base class for observable objects with support for the IMessenger service.
- RelayCommand: a simple delegate command implementing the ICommand interface.
- Messenger: a messaging system to exchange messages through different loosely-coupled objects.
- Ioc: a helper class to configure dependency injection service containers.
</Description>
<PackageTags>UWP Toolkit Windows Mvvm observable Ioc dependency injection services extensions helpers</PackageTags>
<PackageTags>UWP Toolkit Windows MVVM MVVMToolkit observable Ioc dependency injection services extensions helpers</PackageTags>
</PropertyGroup>

<!-- .NET Standard 2.0 doesn't have the Span<T> type -->
Expand Down

0 comments on commit ff7f24d

Please sign in to comment.