diff --git a/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs b/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs index 2f12a9f710e9..7a485f7b49ab 100644 --- a/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs +++ b/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs @@ -118,16 +118,6 @@ void UpdateDetail() void UpdateFlyout() { _ = MauiContext ?? throw new InvalidOperationException($"{nameof(MauiContext)} should have been set by base class."); - - // Once this issue has been taken care of - // https://github.com/dotnet/maui/issues/8456 - // we can remove this code - if (VirtualView.Flyout.Handler?.MauiContext != null && - VirtualView.Flyout.Handler.MauiContext != MauiContext) - { - VirtualView.Flyout.Handler.DisconnectHandler(); - } - _ = VirtualView.Flyout.ToPlatform(MauiContext); var newFlyoutView = VirtualView.Flyout.ToPlatform(); diff --git a/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.cs b/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.cs index 8cce554da35c..ec3b42944bc4 100644 --- a/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.cs +++ b/src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.cs @@ -16,11 +16,19 @@ namespace Microsoft.Maui.Handlers { public partial class FlyoutViewHandler : IFlyoutViewHandler { - public static IPropertyMapper Mapper = new PropertyMapper(ViewHandler.ViewMapper) + // Like IViewHandler.ContainerView, those properties should be set with priority because other mappers depend on them (like IToolbarElement.Toolbar). + // So we have a separate mapper for them. + private static readonly IPropertyMapper FlyoutLayoutMapper = new PropertyMapper() { #if ANDROID || WINDOWS || TIZEN [nameof(IFlyoutView.Flyout)] = MapFlyout, [nameof(IFlyoutView.Detail)] = MapDetail, +#endif + }; + + public static IPropertyMapper Mapper = new PropertyMapper(ViewHandler.ViewMapper, FlyoutLayoutMapper) + { +#if ANDROID || WINDOWS || TIZEN [nameof(IFlyoutView.IsPresented)] = MapIsPresented, [nameof(IFlyoutView.FlyoutBehavior)] = MapFlyoutBehavior, [nameof(IFlyoutView.FlyoutWidth)] = MapFlyoutWidth, diff --git a/src/Core/src/Handlers/View/AndroidBatchPropertyMapper.cs b/src/Core/src/Handlers/View/AndroidBatchPropertyMapper.cs index 60d71571646f..fe20df17c5fa 100644 --- a/src/Core/src/Handlers/View/AndroidBatchPropertyMapper.cs +++ b/src/Core/src/Handlers/View/AndroidBatchPropertyMapper.cs @@ -1,13 +1,13 @@ -using System; +using System; using System.Collections.Generic; namespace Microsoft.Maui.Handlers { #if ANDROID - class AndroidBatchPropertyMapper : PropertyMapper - where TVirtualView : IElement - where TViewHandler : IElementHandler + class AndroidBatchPropertyMapper { + public const string InitializeBatchedPropertiesKey = "_InitializeBatchedProperties"; + // During mass property updates, this list of properties will be skipped public static HashSet SkipList = new(StringComparer.Ordinal) { @@ -27,30 +27,48 @@ class AndroidBatchPropertyMapper : PropertyMapper : PropertyMapper + where TVirtualView : IElement + where TViewHandler : IElementHandler + { public AndroidBatchPropertyMapper(params IPropertyMapper[] chained) : base(chained) { } public override IEnumerable GetKeys() { - foreach (var key in _mapper.Keys) + var skipList = AndroidBatchPropertyMapper.SkipList; + + // We want to retain the initial order of the keys to avoid race conditions + // when a property mapping is overridden by a new instance of property mapper. + // As an example, the container view mapper should always run first. + // Siblings mapper should not have keys intersection. + var chainedPropertyMappers = Chained; + if (chainedPropertyMappers is not null) { - // When reporting the key list for mass updates up the chain, ignore properties in SkipList. - // These will be handled by ViewHandler.SetVirtualView() instead. - if (SkipList.Contains(key)) + for (int i = chainedPropertyMappers.Length - 1; i >= 0; i--) { - continue; + foreach (var key in chainedPropertyMappers[i].GetKeys()) + { + yield return key; + } } - - yield return key; } - if (Chained is not null) + // Enqueue keys from this mapper. + foreach (var mapper in _mapper) { - foreach (var chain in Chained) - foreach (var key in chain.GetKeys()) - yield return key; + var key = mapper.Key; + + // When reporting the key list for mass updates up the chain, ignore properties in SkipList. + // These will be handled by ViewHandler.SetVirtualView() instead. + if (!skipList.Contains(key)) + { + yield return key; + } } } } #endif -} +} \ No newline at end of file diff --git a/src/Core/src/Handlers/View/ViewHandler.cs b/src/Core/src/Handlers/View/ViewHandler.cs index c2dbc56e153c..43b76c1f695c 100644 --- a/src/Core/src/Handlers/View/ViewHandler.cs +++ b/src/Core/src/Handlers/View/ViewHandler.cs @@ -26,11 +26,17 @@ public abstract partial class ViewHandler : ElementHandler, IViewHandler public static IPropertyMapper ViewMapper = #if ANDROID // Use a custom mapper for Android which knows how to batch the initial property sets - new AndroidBatchPropertyMapper(ElementMapper) + new AndroidBatchPropertyMapper(ElementHandler.ElementMapper) #else new PropertyMapper(ElementHandler.ElementMapper) #endif { + // This property is a special one and needs to be set before other properties. + [nameof(IViewHandler.ContainerView)] = MapContainerView, +#if ANDROID + [AndroidBatchPropertyMapper.InitializeBatchedPropertiesKey] = MapInitializeBatchedProperties, +#endif + [nameof(IView.AutomationId)] = MapAutomationId, [nameof(IView.Clip)] = MapClip, [nameof(IView.Shadow)] = MapShadow, @@ -56,7 +62,6 @@ public abstract partial class ViewHandler : ElementHandler, IViewHandler [nameof(IView.RotationY)] = MapRotationY, [nameof(IView.AnchorX)] = MapAnchorX, [nameof(IView.AnchorY)] = MapAnchorY, - [nameof(IViewHandler.ContainerView)] = MapContainerView, #pragma warning disable CS0618 // Type or member is obsolete [nameof(IBorder.Border)] = MapBorderView, #pragma warning restore CS0618 // Type or member is obsolete @@ -68,10 +73,6 @@ public abstract partial class ViewHandler : ElementHandler, IViewHandler #if WINDOWS || MACCATALYST [nameof(IContextFlyoutElement.ContextFlyout)] = MapContextFlyout, #endif - -#if ANDROID - ["_InitializeBatchedProperties"] = MapInitializeBatchedProperties -#endif }; /// diff --git a/src/Core/src/Platform/iOS/ContainerViewController.cs b/src/Core/src/Platform/iOS/ContainerViewController.cs index 3b85e030c268..1ff5a1fd2504 100644 --- a/src/Core/src/Platform/iOS/ContainerViewController.cs +++ b/src/Core/src/Platform/iOS/ContainerViewController.cs @@ -68,7 +68,10 @@ public override void LoadView() void LoadPlatformView(IElement view) { - currentPlatformView = _pendingLoadedView ?? CreatePlatformView(view); + var platformView = _pendingLoadedView ?? CreatePlatformView(view); + platformView = platformView.Superview as WrapperView ?? platformView; + + currentPlatformView = platformView; _pendingLoadedView = null; View!.AddSubview(currentPlatformView); diff --git a/src/Core/src/PropertyMapper.cs b/src/Core/src/PropertyMapper.cs index 61bf26b5c92b..f084199719ea 100644 --- a/src/Core/src/PropertyMapper.cs +++ b/src/Core/src/PropertyMapper.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; #if IOS || MACCATALYST using PlatformView = UIKit.UIView; @@ -15,13 +16,17 @@ namespace Microsoft.Maui { public abstract class PropertyMapper : IPropertyMapper { + // TODO: Make this private in .NET10 protected readonly Dictionary> _mapper = new(StringComparer.Ordinal); - IPropertyMapper[]? _chained; - // Keep a distinct list of the keys so we don't run any duplicate (overridden) updates more than once - // when we call UpdateProperties - HashSet? _updateKeys; + List? _updatePropertiesKeys; + List>? _updatePropertiesMappers; + Dictionary?>? _cachedMappers; + + List UpdatePropertiesKeys => _updatePropertiesKeys ?? SnapshotMappers().UpdatePropertiesKeys; + List> UpdatePropertiesMappers => _updatePropertiesMappers ?? SnapshotMappers().UpdatePropertiesMappers; + Dictionary?> CachedMappers => _cachedMappers ?? SnapshotMappers().CachedMappers; public PropertyMapper() { @@ -35,29 +40,65 @@ public PropertyMapper(params IPropertyMapper[]? chained) protected virtual void SetPropertyCore(string key, Action action) { _mapper[key] = action; + ClearKeyCache(); } protected virtual void UpdatePropertyCore(string key, IElementHandler viewHandler, IElement virtualView) { if (!viewHandler.CanInvokeMappers()) + { return; + } - var action = GetProperty(key); - action?.Invoke(viewHandler, virtualView); + TryUpdatePropertyCore(key, viewHandler, virtualView); + } + + internal bool TryUpdatePropertyCore(string key, IElementHandler viewHandler, IElement virtualView) + { + var cachedMappers = CachedMappers; + if (cachedMappers.TryGetValue(key, out var action)) + { + if (action is not null) + { + action(viewHandler, virtualView); + return true; + } + + return false; + } + + // CachedMappers initially contains only the UpdateProperties keys which may not contain the key we are looking for. + // See AndroidBatchPropertyMapper for an example. + var mapper = GetProperty(key); + cachedMappers[key] = mapper; + + if (mapper is not null) + { + mapper(viewHandler, virtualView); + return true; + } + + return false; } public virtual Action? GetProperty(string key) { if (_mapper.TryGetValue(key, out var action)) + { return action; - else if (Chained is not null) + } + + var chainedPropertyMappers = Chained; + if (chainedPropertyMappers is not null) { - foreach (var ch in Chained) + foreach (var ch in chainedPropertyMappers) { var returnValue = ch.GetProperty(key); if (returnValue != null) + { return returnValue; + } } } @@ -66,20 +107,24 @@ protected virtual void UpdatePropertyCore(string key, IElementHandler viewHandle public void UpdateProperty(IElementHandler viewHandler, IElement? virtualView, string property) { - if (virtualView == null) + if (virtualView == null || !viewHandler.CanInvokeMappers()) + { return; + } - UpdatePropertyCore(property, viewHandler, virtualView); + TryUpdatePropertyCore(property, viewHandler, virtualView); } public void UpdateProperties(IElementHandler viewHandler, IElement? virtualView) { - if (virtualView == null) + if (virtualView == null || !viewHandler.CanInvokeMappers()) + { return; + } - foreach (var key in UpdateKeys) + foreach (var mapper in UpdatePropertiesMappers) { - UpdatePropertyCore(key, viewHandler, virtualView); + mapper(viewHandler, virtualView); } } @@ -93,35 +138,66 @@ public IPropertyMapper[]? Chained } } - private HashSet PopulateKeys() - { - var keys = new HashSet(StringComparer.Ordinal); - foreach (var key in GetKeys()) - { - keys.Add(key); - } - return keys; - } - + // TODO: Make private in .NET10 with a new name: ClearMergedMappers protected virtual void ClearKeyCache() { - _updateKeys = null; + _updatePropertiesMappers = null; + _updatePropertiesKeys = null; + _cachedMappers = null; } - public virtual IReadOnlyCollection UpdateKeys => _updateKeys ??= PopulateKeys(); + // TODO: Remove in .NET10 + public virtual IReadOnlyCollection UpdateKeys => UpdatePropertiesKeys; public virtual IEnumerable GetKeys() { - foreach (var key in _mapper.Keys) - yield return key; - - if (Chained is not null) + // We want to retain the initial order of the keys to avoid race conditions + // when a property mapping is overridden by a new instance of property mapper. + // As an example, the container view mapper should always run first. + // Siblings mapper should not have keys intersection. + var chainedPropertyMappers = Chained; + if (chainedPropertyMappers is not null) { - foreach (var chain in Chained) - foreach (var key in chain.GetKeys()) + for (int i = chainedPropertyMappers.Length - 1; i >= 0; i--) + { + foreach (var key in chainedPropertyMappers[i].GetKeys()) + { yield return key; + } + } + } + + // Enqueue keys from this mapper. + foreach (var mapper in _mapper) + { + yield return mapper.Key; } } + + private (List UpdatePropertiesKeys, List> UpdatePropertiesMappers, Dictionary?> CachedMappers) SnapshotMappers() + { + var updatePropertiesKeys = GetKeys().Distinct().ToList(); + var updatePropertiesMappers = new List>(updatePropertiesKeys.Count); +#if ANDROID + var cacheSize = updatePropertiesKeys.Count + AndroidBatchPropertyMapper.SkipList.Count; +#else + var cacheSize = updatePropertiesKeys.Count; +#endif + var cachedMappers = new Dictionary?>(cacheSize); + + foreach (var key in updatePropertiesKeys) + { + var mapper = GetProperty(key)!; + updatePropertiesMappers.Add(mapper); + cachedMappers[key] = mapper; + } + + _updatePropertiesKeys = updatePropertiesKeys; + _updatePropertiesMappers = updatePropertiesMappers; + _cachedMappers = cachedMappers; + + return (updatePropertiesKeys, updatePropertiesMappers, cachedMappers); + } } public interface IPropertyMapper @@ -169,12 +245,22 @@ public void Add(string key, Action action) => SetPropertyCore(key, (h, v) => { if (v is TVirtualView vv) + { action?.Invoke((TViewHandler)h, vv); + } else if (Chained != null) { foreach (var chain in Chained) { - if (chain.GetProperty(key) != null) + // Try to leverage our internal method which uses merged mappers + if (chain is PropertyMapper propertyMapper) + { + if (propertyMapper.TryUpdatePropertyCore(key, h, v)) + { + break; + } + } + else if (chain.GetProperty(key) != null) { chain.UpdateProperty(h, v, key); break; diff --git a/src/Core/tests/Benchmarks/Benchmarks/PropertyMapperBenchmarker.cs b/src/Core/tests/Benchmarks/Benchmarks/PropertyMapperBenchmarker.cs new file mode 100644 index 000000000000..ac6387b871f7 --- /dev/null +++ b/src/Core/tests/Benchmarks/Benchmarks/PropertyMapperBenchmarker.cs @@ -0,0 +1,52 @@ +using BenchmarkDotNet.Attributes; +using Microsoft.Maui.Controls; + +namespace Microsoft.Maui.Handlers.Benchmarks; + +[MemoryDiagnoser] +public class PropertyMapperBenchmarker +{ + class TestButtonHandler : ButtonHandler + { + protected override object CreatePlatformView() + { + return new object(); + } + } + + readonly Registrar _registrar; + readonly IViewHandler _handler; + readonly Button _button; + + public PropertyMapperBenchmarker() + { + _button = new Button(); + + _registrar = new Registrar(); + _registrar.Register(); + + _handler = _registrar.GetHandler(); + _handler.SetVirtualView(new Button()); + } + + [Benchmark] + public void BenchmarkUpdateProperties() + { + var button = new Button(); + + for (int i = 0; i < 100_000; i++) + { + var handler = _registrar.GetHandler(); + handler.SetVirtualView(button); + } + } + + [Benchmark] + public void BenchmarkUpdateProperty() + { + for (int i = 0; i < 1_000_000; i++) + { + _handler.UpdateValue(nameof(IView.Opacity)); + } + } +} \ No newline at end of file diff --git a/src/Core/tests/UnitTests/PropertyMapperTests.cs b/src/Core/tests/UnitTests/PropertyMapperTests.cs index 0218c689bb37..d2edcf23417a 100644 --- a/src/Core/tests/UnitTests/PropertyMapperTests.cs +++ b/src/Core/tests/UnitTests/PropertyMapperTests.cs @@ -1,3 +1,6 @@ +using System; +using System.Collections.Generic; +using System.Linq; using Microsoft.Maui; using Microsoft.Maui.Controls; using Microsoft.Maui.Handlers; @@ -9,6 +12,106 @@ namespace Microsoft.Maui.UnitTests [Category(TestCategory.Core, TestCategory.PropertyMapping)] public class PropertyMapperTests { + [Fact] + public void MapperRetainsInsertionOrder() + { + // This test ensures the internal Dictionary implementation is not changed. + + // Thanks to the way Dictionary is internally implemented, + // when looping through the dictionary entries, and considering _mapper is an append-only dictionary, + // the order is guaranteed to be the same as the order mappers were added. + + var mapper = new PropertyMapper(); + var letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"; + var insertedProperties = new List(); + var updatedProperties = new List(); + for (int i = 0; i < 1000; i++) + { + var randomPropertyName = string.Join(string.Empty, Enumerable.Range(0, Random.Shared.Next(8, 60)) + .Select(chars => string.Join(string.Empty, + Enumerable.Range(0, chars).Select(_ => letters[Random.Shared.Next(0, letters.Length)])))); + + insertedProperties.Add(randomPropertyName); + mapper.Add(randomPropertyName, (h, v) => updatedProperties.Add(randomPropertyName)); + } + + var keys = mapper.GetKeys().ToList(); + mapper.UpdateProperties(null!, new Button()); + Assert.Equal(insertedProperties, keys); + Assert.Equal(insertedProperties, updatedProperties); + } + + [Fact] + public void MapperExecutesChainedKeysFirst() + { + int counter = 0; + + int background3 = 0; + int scale1 = 0; + int scale3 = 0; + int zindex2 = 0; + int zindex3 = 0; + + var mapper1 = new PropertyMapper + { + [nameof(IView.Scale)] = (r, v) => scale1 = ++counter + }; + + var mapper2 = new PropertyMapper + { + [nameof(IView.ZIndex)] = (r, v) => zindex2 = ++counter + }; + + var mapper3 = new PropertyMapper(mapper2, mapper1) + { + [nameof(IView.Background)] = (r, v) => background3 = ++counter, + [nameof(IView.Scale)] = (r, v) => scale3 = ++counter, + [nameof(IView.ZIndex)] = (r, v) => zindex3 = ++counter, + }; + + mapper3.UpdateProperties(null!, new Button()); + + Assert.Equal(0, scale1); + Assert.Equal(0, zindex2); + Assert.Equal(1, scale3); + Assert.Equal(2, zindex3); + Assert.Equal(3, background3); + } + + [Fact] + public void MapperCanExecuteSkippedMappers() + { + int counter = 0; + + int scale1 = 0; + int scale2 = 0; + int zindex1 = 0; + + var mapper1 = new SkippingPropertyMapper() + { + [nameof(IView.Scale)] = (r, v) => scale1 = ++counter, + [nameof(IView.ZIndex)] = (r, v) => zindex1 = ++counter + }; + + var mapper2 = new PropertyMapper(mapper1) + { + [nameof(IView.Scale)] = (r, v) => scale2 = ++counter, + }; + + mapper2.UpdateProperties(null!, new Button()); + + // ZIndex is skipped, so it should not be updated + Assert.Equal(0, zindex1); + // Scale is skipped in the first mapper, but not in the second + Assert.Equal(0, scale1); + Assert.Equal(1, scale2); + + mapper2.UpdateProperty(null!, new Button(), nameof(IView.ZIndex)); + + // When updating a single property, the skipped mapper should be executed + Assert.Equal(2, zindex1); + } + [Fact] public void ChainingMappersOverrideBase() { @@ -143,5 +246,14 @@ public void GenericMappersWorks() Assert.True(wasMapper1Called); Assert.True(wasMapper2Called); } + + class SkippingPropertyMapper : PropertyMapper + where T : IElement + { + public override IEnumerable GetKeys() + { + return Enumerable.Empty(); + } + } } }