From 45d726e0c9ddac4e4a7c821e7589d51f6947ce7f Mon Sep 17 00:00:00 2001 From: Mario Uhlmann Date: Fri, 10 Jun 2022 19:22:50 +0200 Subject: [PATCH 1/4] Style improvements - primary OnCollectionChanged refactored (removed unnecessary cast and null checks in loops) --- src/Avalonia.Base/Styling/Styles.cs | 119 +++++++++++++++------------- 1 file changed, 62 insertions(+), 57 deletions(-) diff --git a/src/Avalonia.Base/Styling/Styles.cs b/src/Avalonia.Base/Styling/Styles.cs index 7c0bc4ad7f..903db5ffc7 100644 --- a/src/Avalonia.Base/Styling/Styles.cs +++ b/src/Avalonia.Base/Styling/Styles.cs @@ -2,6 +2,7 @@ using System; using System.Collections; using System.Collections.Generic; using System.Collections.Specialized; +using System.Linq; using Avalonia.Collections; using Avalonia.Controls; @@ -17,7 +18,7 @@ namespace Avalonia.Styling IStyle, IResourceProvider { - private readonly AvaloniaList _styles = new AvaloniaList(); + private readonly AvaloniaList _styles = new(); private IResourceHost? _owner; private IResourceDictionary? _resources; private StyleCache? _cache; @@ -62,16 +63,18 @@ namespace Avalonia.Styling { value = value ?? throw new ArgumentNullException(nameof(Resources)); - if (Owner is object) + var currentOwner = Owner; + + if (currentOwner is not null) { - _resources?.RemoveOwner(Owner); + _resources?.RemoveOwner(currentOwner); } _resources = value; - if (Owner is object) + if (currentOwner is not null) { - _resources.AddOwner(Owner); + _resources.AddOwner(currentOwner); } } } @@ -89,7 +92,7 @@ namespace Avalonia.Styling foreach (var i in this) { - if (i is IResourceProvider p && p.HasResources) + if (i is IResourceProvider { HasResources: true }) { return true; } @@ -188,9 +191,9 @@ namespace Avalonia.Styling /// void IResourceProvider.AddOwner(IResourceHost owner) { - owner = owner ?? throw new ArgumentNullException(nameof(owner)); + ArgumentNullException.ThrowIfNull(owner); - if (Owner != null) + if (Owner is not null) { throw new InvalidOperationException("The Styles already has a owner."); } @@ -210,7 +213,7 @@ namespace Avalonia.Styling /// void IResourceProvider.RemoveOwner(IResourceHost owner) { - owner = owner ?? throw new ArgumentNullException(nameof(owner)); + ArgumentNullException.ThrowIfNull(owner); if (Owner == owner) { @@ -227,70 +230,72 @@ namespace Avalonia.Styling } } - private void OnCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) + private static IReadOnlyList ToReadOnlyList(ICollection list) { - static IReadOnlyList ToReadOnlyList(IList list) + if (list is IReadOnlyList readOnlyList) { - if (list is IReadOnlyList) - { - return (IReadOnlyList)list; - } - else - { - var result = new T[list.Count]; - list.CopyTo(result, 0); - return result; - } + return readOnlyList; } - void Add(IList items) + var result = new T[list.Count]; + list.CopyTo(result, 0); + return result; + } + + private static void InternalAdd(IList items, IResourceHost owner, ref StyleCache? cache) + { + foreach (var resourceProvider in items.OfType()) { - for (var i = 0; i < items.Count; ++i) - { - var style = (IStyle)items[i]!; + resourceProvider.AddOwner(owner); + } - if (Owner is object && style is IResourceProvider resourceProvider) - { - resourceProvider.AddOwner(Owner); - } + if (items.Count > 0) + { + cache = null; + } - _cache = null; - } + (owner as IStyleHost)?.StylesAdded(ToReadOnlyList(items)); + } - (Owner as IStyleHost)?.StylesAdded(ToReadOnlyList(items)); + private static void InternalRemove(IList items, IResourceHost owner, ref StyleCache? cache) + { + foreach (var resourceProvider in items.OfType()) + { + resourceProvider.RemoveOwner(owner); } - void Remove(IList items) + if (items.Count > 0) { - for (var i = 0; i < items.Count; ++i) - { - var style = (IStyle)items[i]!; - - if (Owner is object && style is IResourceProvider resourceProvider) - { - resourceProvider.RemoveOwner(Owner); - } + cache = null; + } - _cache = null; - } + (owner as IStyleHost)?.StylesRemoved(ToReadOnlyList(items)); + } - (Owner as IStyleHost)?.StylesRemoved(ToReadOnlyList(items)); + private void OnCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) + { + if (e.Action == NotifyCollectionChangedAction.Reset) + { + throw new InvalidOperationException("Reset should not be called on Styles."); } - switch (e.Action) + var currentOwner = Owner; + + if (currentOwner is not null) { - case NotifyCollectionChangedAction.Add: - Add(e.NewItems!); - break; - case NotifyCollectionChangedAction.Remove: - Remove(e.OldItems!); - break; - case NotifyCollectionChangedAction.Replace: - Remove(e.OldItems!); - Add(e.NewItems!); - break; - case NotifyCollectionChangedAction.Reset: - throw new InvalidOperationException("Reset should not be called on Styles."); + switch (e.Action) + { + case NotifyCollectionChangedAction.Add: + InternalAdd(e.NewItems!, currentOwner, ref _cache); + break; + case NotifyCollectionChangedAction.Remove: + InternalRemove(e.OldItems!, currentOwner, ref _cache); + break; + case NotifyCollectionChangedAction.Replace: + InternalRemove(e.OldItems!, currentOwner, ref _cache); + InternalAdd(e.NewItems!, currentOwner, ref _cache); + break; + } } CollectionChanged?.Invoke(this, e); From 1d0d1f20841440fd2b3385638aa0f24e8d9825d3 Mon Sep 17 00:00:00 2001 From: Mario Uhlmann Date: Fri, 10 Jun 2022 19:35:14 +0200 Subject: [PATCH 2/4] compile fix - ArgumentNullException.ThrowIfNull (net standard 2.0 fail) --- src/Avalonia.Base/Styling/Styles.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Base/Styling/Styles.cs b/src/Avalonia.Base/Styling/Styles.cs index 903db5ffc7..175068541b 100644 --- a/src/Avalonia.Base/Styling/Styles.cs +++ b/src/Avalonia.Base/Styling/Styles.cs @@ -191,7 +191,7 @@ namespace Avalonia.Styling /// void IResourceProvider.AddOwner(IResourceHost owner) { - ArgumentNullException.ThrowIfNull(owner); + owner = owner ?? throw new ArgumentNullException(nameof(owner)); if (Owner is not null) { @@ -213,7 +213,7 @@ namespace Avalonia.Styling /// void IResourceProvider.RemoveOwner(IResourceHost owner) { - ArgumentNullException.ThrowIfNull(owner); + owner = owner ?? throw new ArgumentNullException(nameof(owner)); if (Owner == owner) { From 76765f75856f32eae7a24c5ad3224b8d66725bf9 Mon Sep 17 00:00:00 2001 From: Mario Uhlmann Date: Sat, 11 Jun 2022 06:51:27 +0200 Subject: [PATCH 3/4] Old cache reset logic --- src/Avalonia.Base/Styling/Styles.cs | 51 +++++++++++++++-------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/Avalonia.Base/Styling/Styles.cs b/src/Avalonia.Base/Styling/Styles.cs index 175068541b..1d0be96ac9 100644 --- a/src/Avalonia.Base/Styling/Styles.cs +++ b/src/Avalonia.Base/Styling/Styles.cs @@ -242,34 +242,40 @@ namespace Avalonia.Styling return result; } - private static void InternalAdd(IList items, IResourceHost owner, ref StyleCache? cache) + private static void InternalAdd(IList items, IResourceHost? owner, ref StyleCache? cache) { - foreach (var resourceProvider in items.OfType()) + if (owner is not null) { - resourceProvider.AddOwner(owner); + foreach (var resourceProvider in items.OfType()) + { + resourceProvider.AddOwner(owner); + } + + (owner as IStyleHost)?.StylesAdded(ToReadOnlyList(items)); } if (items.Count > 0) { cache = null; } - - (owner as IStyleHost)?.StylesAdded(ToReadOnlyList(items)); } - private static void InternalRemove(IList items, IResourceHost owner, ref StyleCache? cache) + private static void InternalRemove(IList items, IResourceHost? owner, ref StyleCache? cache) { - foreach (var resourceProvider in items.OfType()) + if (owner is not null) { - resourceProvider.RemoveOwner(owner); + foreach (var resourceProvider in items.OfType()) + { + resourceProvider.RemoveOwner(owner); + } + + (owner as IStyleHost)?.StylesRemoved(ToReadOnlyList(items)); } if (items.Count > 0) { cache = null; } - - (owner as IStyleHost)?.StylesRemoved(ToReadOnlyList(items)); } private void OnCollectionChanged(object? sender, NotifyCollectionChangedEventArgs e) @@ -281,21 +287,18 @@ namespace Avalonia.Styling var currentOwner = Owner; - if (currentOwner is not null) + switch (e.Action) { - switch (e.Action) - { - case NotifyCollectionChangedAction.Add: - InternalAdd(e.NewItems!, currentOwner, ref _cache); - break; - case NotifyCollectionChangedAction.Remove: - InternalRemove(e.OldItems!, currentOwner, ref _cache); - break; - case NotifyCollectionChangedAction.Replace: - InternalRemove(e.OldItems!, currentOwner, ref _cache); - InternalAdd(e.NewItems!, currentOwner, ref _cache); - break; - } + case NotifyCollectionChangedAction.Add: + InternalAdd(e.NewItems!, currentOwner, ref _cache); + break; + case NotifyCollectionChangedAction.Remove: + InternalRemove(e.OldItems!, currentOwner, ref _cache); + break; + case NotifyCollectionChangedAction.Replace: + InternalRemove(e.OldItems!, currentOwner, ref _cache); + InternalAdd(e.NewItems!, currentOwner, ref _cache); + break; } CollectionChanged?.Invoke(this, e); From bbe7d0abb255863ed05b7ea1eee32be47e09df37 Mon Sep 17 00:00:00 2001 From: Mario Uhlmann Date: Sat, 11 Jun 2022 13:23:44 +0200 Subject: [PATCH 4/4] foreach .. "items.OfType<" replaced with traditional for-loop --- src/Avalonia.Base/Styling/Styles.cs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Avalonia.Base/Styling/Styles.cs b/src/Avalonia.Base/Styling/Styles.cs index 1d0be96ac9..e4c3371007 100644 --- a/src/Avalonia.Base/Styling/Styles.cs +++ b/src/Avalonia.Base/Styling/Styles.cs @@ -2,7 +2,6 @@ using System; using System.Collections; using System.Collections.Generic; using System.Collections.Specialized; -using System.Linq; using Avalonia.Collections; using Avalonia.Controls; @@ -246,9 +245,12 @@ namespace Avalonia.Styling { if (owner is not null) { - foreach (var resourceProvider in items.OfType()) + for (var i = 0; i < items.Count; ++i) { - resourceProvider.AddOwner(owner); + if (items[i] is IResourceProvider provider) + { + provider.AddOwner(owner); + } } (owner as IStyleHost)?.StylesAdded(ToReadOnlyList(items)); @@ -264,9 +266,12 @@ namespace Avalonia.Styling { if (owner is not null) { - foreach (var resourceProvider in items.OfType()) + for (var i = 0; i < items.Count; ++i) { - resourceProvider.RemoveOwner(owner); + if (items[i] is IResourceProvider provider) + { + provider.RemoveOwner(owner); + } } (owner as IStyleHost)?.StylesRemoved(ToReadOnlyList(items));