From b27ed6474e57c52b1d97d28c3b624406db66c4db Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 11 Nov 2024 20:09:30 +0900 Subject: [PATCH] Allow CachedModelDependencyContainer to cache models with non-bindable fields --- .../Reflection/CachedModelDependenciesTest.cs | 60 ++-------------- .../CachedModelDependenciesTest.cs | 60 ++-------------- .../CachedModelDependencyContainer.cs | 69 ++++++------------- 3 files changed, 34 insertions(+), 155 deletions(-) diff --git a/osu.Framework.Tests/Dependencies/Reflection/CachedModelDependenciesTest.cs b/osu.Framework.Tests/Dependencies/Reflection/CachedModelDependenciesTest.cs index 8c27434c91..02297e5bde 100644 --- a/osu.Framework.Tests/Dependencies/Reflection/CachedModelDependenciesTest.cs +++ b/osu.Framework.Tests/Dependencies/Reflection/CachedModelDependenciesTest.cs @@ -1,9 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - -using System; using System.Diagnostics.CodeAnalysis; using NUnit.Framework; using osu.Framework.Allocation; @@ -15,24 +12,6 @@ namespace osu.Framework.Tests.Dependencies.Reflection [SuppressMessage("Performance", "OFSG001:Class contributes to dependency injection and should be partial")] public class CachedModelDependenciesTest { - [Test] - public void TestModelWithNonBindableFieldsFails() - { - IReadOnlyDependencyContainer unused; - - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - } - - [Test] - public void TestModelWithNonReadOnlyFieldsFails() - { - IReadOnlyDependencyContainer unused; - - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - } - [Test] public void TestSettingNoModelResolvesDefault() { @@ -195,7 +174,7 @@ public void TestSetModelToNullAfterResolved() var model = new FieldModel { Bindable = { Value = 2 } }; - var dependencies = new CachedModelDependencyContainer(null) + var dependencies = new CachedModelDependencyContainer(null) { Model = { Value = model } }; @@ -248,7 +227,7 @@ public void TestResolveIndividualProperties() BindableString = { Value = "3" } }; - var dependencies = new CachedModelDependencyContainer(null) + var dependencies = new CachedModelDependencyContainer(null) { Model = { Value = model1 } }; @@ -269,33 +248,6 @@ public void TestResolveIndividualProperties() Assert.AreEqual(null, resolver.BindableString.Value); } - private class NonBindablePublicFieldModel : IDependencyInjectionCandidate - { -#pragma warning disable 649 - public readonly int FailingField; -#pragma warning restore 649 - } - - private class NonBindablePrivateFieldModel : IDependencyInjectionCandidate - { -#pragma warning disable 169 - private readonly int failingField; -#pragma warning restore 169 - } - - private class NonReadOnlyFieldModel : IDependencyInjectionCandidate - { -#pragma warning disable 649 - public Bindable Bindable; -#pragma warning restore 649 - } - - private class PropertyModel : IDependencyInjectionCandidate - { - // ReSharper disable once UnusedMember.Local - public Bindable Bindable { get; private set; } - } - private class FieldModel : IDependencyInjectionCandidate { [Cached] @@ -311,22 +263,22 @@ private class DerivedFieldModel : FieldModel private class FieldModelResolver : IDependencyInjectionCandidate { [Resolved] - public FieldModel Model { get; private set; } + public FieldModel Model { get; private set; } = null!; } private class DerivedFieldModelResolver : IDependencyInjectionCandidate { [Resolved] - public DerivedFieldModel Model { get; private set; } + public DerivedFieldModel Model { get; private set; } = null!; } private class DerivedFieldModelPropertyResolver : IDependencyInjectionCandidate { [Resolved(typeof(DerivedFieldModel))] - public Bindable Bindable { get; private set; } + public Bindable Bindable { get; private set; } = null!; [Resolved(typeof(DerivedFieldModel))] - public Bindable BindableString { get; private set; } + public Bindable BindableString { get; private set; } = null!; } } } diff --git a/osu.Framework.Tests/Dependencies/SourceGeneration/CachedModelDependenciesTest.cs b/osu.Framework.Tests/Dependencies/SourceGeneration/CachedModelDependenciesTest.cs index 8555380a72..84529a4ae5 100644 --- a/osu.Framework.Tests/Dependencies/SourceGeneration/CachedModelDependenciesTest.cs +++ b/osu.Framework.Tests/Dependencies/SourceGeneration/CachedModelDependenciesTest.cs @@ -1,9 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - -using System; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Bindables; @@ -13,24 +10,6 @@ namespace osu.Framework.Tests.Dependencies.SourceGeneration [TestFixture] public partial class CachedModelDependenciesTest { - [Test] - public void TestModelWithNonBindableFieldsFails() - { - IReadOnlyDependencyContainer unused; - - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - } - - [Test] - public void TestModelWithNonReadOnlyFieldsFails() - { - IReadOnlyDependencyContainer unused; - - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - Assert.Throws(() => unused = new CachedModelDependencyContainer(null)); - } - [Test] public void TestSettingNoModelResolvesDefault() { @@ -193,7 +172,7 @@ public void TestSetModelToNullAfterResolved() var model = new FieldModel { Bindable = { Value = 2 } }; - var dependencies = new CachedModelDependencyContainer(null) + var dependencies = new CachedModelDependencyContainer(null) { Model = { Value = model } }; @@ -246,7 +225,7 @@ public void TestResolveIndividualProperties() BindableString = { Value = "3" } }; - var dependencies = new CachedModelDependencyContainer(null) + var dependencies = new CachedModelDependencyContainer(null) { Model = { Value = model1 } }; @@ -267,33 +246,6 @@ public void TestResolveIndividualProperties() Assert.AreEqual(null, resolver.BindableString.Value); } - private partial class NonBindablePublicFieldModel : IDependencyInjectionCandidate - { -#pragma warning disable 649 - public readonly int FailingField; -#pragma warning restore 649 - } - - private partial class NonBindablePrivateFieldModel : IDependencyInjectionCandidate - { -#pragma warning disable 169 - private readonly int failingField; -#pragma warning restore 169 - } - - private partial class NonReadOnlyFieldModel : IDependencyInjectionCandidate - { -#pragma warning disable 649 - public Bindable Bindable; -#pragma warning restore 649 - } - - private partial class PropertyModel : IDependencyInjectionCandidate - { - // ReSharper disable once UnusedMember.Local - public Bindable Bindable { get; private set; } - } - private partial class FieldModel : IDependencyInjectionCandidate { [Cached] @@ -309,22 +261,22 @@ private partial class DerivedFieldModel : FieldModel private partial class FieldModelResolver : IDependencyInjectionCandidate { [Resolved] - public FieldModel Model { get; private set; } + public FieldModel Model { get; private set; } = null!; } private partial class DerivedFieldModelResolver : IDependencyInjectionCandidate { [Resolved] - public DerivedFieldModel Model { get; private set; } + public DerivedFieldModel Model { get; private set; } = null!; } private partial class DerivedFieldModelPropertyResolver : IDependencyInjectionCandidate { [Resolved(typeof(DerivedFieldModel))] - public Bindable Bindable { get; private set; } + public Bindable Bindable { get; private set; } = null!; [Resolved(typeof(DerivedFieldModel))] - public Bindable BindableString { get; private set; } + public Bindable BindableString { get; private set; } = null!; } } } diff --git a/osu.Framework/Allocation/CachedModelDependencyContainer.cs b/osu.Framework/Allocation/CachedModelDependencyContainer.cs index ec3f0768f1..12f37dbe87 100644 --- a/osu.Framework/Allocation/CachedModelDependencyContainer.cs +++ b/osu.Framework/Allocation/CachedModelDependencyContainer.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Reflection; using osu.Framework.Bindables; @@ -19,7 +17,7 @@ namespace osu.Framework.Allocation /// /// The type of the model to cache. Must contain only fields or auto-properties. public class CachedModelDependencyContainer : IReadOnlyDependencyContainer - where TModel : class, IDependencyInjectionCandidate, new() + where TModel : class?, IDependencyInjectionCandidate?, new() { private const BindingFlags activator_flags = BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly; @@ -32,17 +30,16 @@ public class CachedModelDependencyContainer : IReadOnlyDependencyContain public readonly Bindable Model = new Bindable(); private readonly TModel shadowModel = new TModel(); - - private readonly IReadOnlyDependencyContainer parent; + private readonly IReadOnlyDependencyContainer? parent; private readonly IReadOnlyDependencyContainer shadowDependencies; - public CachedModelDependencyContainer(IReadOnlyDependencyContainer parent) + public CachedModelDependencyContainer(IReadOnlyDependencyContainer? parent) { this.parent = parent; shadowDependencies = DependencyActivator.MergeDependencies(shadowModel, null, new CacheInfo(parent: typeof(TModel))); - TModel currentModel = null; + TModel? currentModel = null; Model.BindValueChanged(e => { // When setting a null model, we actually want to reset the shadow model to a default state @@ -55,9 +52,9 @@ public CachedModelDependencyContainer(IReadOnlyDependencyContainer parent) }); } - public object Get(Type type) => Get(type, default); + public object? Get(Type type) => Get(type, default); - public object Get(Type type, CacheInfo info) + public object? Get(Type type, CacheInfo info) { if (info.Parent == null) return type == typeof(TModel) ? createChildShadowModel() : parent?.Get(type, info); @@ -87,65 +84,43 @@ private TModel createChildShadowModel() /// The shadow model to update. /// The model to unbind from. /// The model to bind to. - private void updateShadowModel(TModel targetShadowModel, TModel lastModel, TModel newModel) + private void updateShadowModel(TModel targetShadowModel, TModel? lastModel, TModel newModel) { - // Due to static-constructor checks, we are guaranteed that all fields will be IBindable - - foreach (var type in typeof(TModel).EnumerateBaseTypes()) + if (lastModel != null) { - foreach (var field in type.GetFields(activator_flags)) + foreach (var type in typeof(TModel).EnumerateBaseTypes()) { - perform(targetShadowModel, field, lastModel, (shadowProp, modelProp) => shadowProp.UnbindFrom(modelProp)); + foreach (var field in type.GetFields(activator_flags)) + perform(field, targetShadowModel, lastModel, (shadowProp, modelProp) => shadowProp.UnbindFrom(modelProp)); } } foreach (var type in typeof(TModel).EnumerateBaseTypes()) { foreach (var field in type.GetFields(activator_flags)) - { - perform(targetShadowModel, field, newModel, (shadowProp, modelProp) => shadowProp.BindTo(modelProp)); - } + perform(field, targetShadowModel, newModel, (shadowProp, modelProp) => shadowProp.BindTo(modelProp)); } } /// /// Perform an arbitrary action across a shadow model and model. /// - private void perform(TModel targetShadowModel, MemberInfo member, TModel target, Action action) + private static void perform(FieldInfo field, TModel shadowModel, TModel targetModel, Action action) { - if (target == null) return; + IBindable? shadowBindable = null; + IBindable? targetBindable = null; - switch (member) + try { - case PropertyInfo pi: - action((IBindable)pi.GetValue(targetShadowModel), (IBindable)pi.GetValue(target)); - break; - - case FieldInfo fi: - action((IBindable)fi.GetValue(targetShadowModel), (IBindable)fi.GetValue(target)); - break; + shadowBindable = field.GetValue(shadowModel) as IBindable; + targetBindable = field.GetValue(targetModel) as IBindable; } - } - - static CachedModelDependencyContainer() - { - foreach (var type in typeof(TModel).EnumerateBaseTypes()) + catch { - foreach (var field in type.GetFields(activator_flags)) - { - if (!typeof(IBindable).IsAssignableFrom(field.FieldType)) - { - throw new InvalidOperationException($"\"{field.DeclaringType}.{field.Name}\" does not subclass {nameof(IBindable)}. " - + $"All fields of {typeof(TModel)} must subclass {nameof(IBindable)} to be used in a {nameof(CachedModelDependencyContainer)}."); - } - - if (!field.IsInitOnly) - { - throw new InvalidOperationException($"\"{field.DeclaringType}.{field.Name}\" is not readonly. " - + $"All fields of {typeof(TModel)} must be readonly to be used in a {nameof(CachedModelDependencyContainer)}."); - } - } } + + if (shadowBindable != null && targetBindable != null) + action(shadowBindable, targetBindable); } } }