From 78116eb30ddf67cabaebdf9ec4a0e1b3c6e931e3 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 15 May 2024 10:15:25 -0500 Subject: [PATCH 1/3] Don't propagate VE.Frame changes to Handler --- src/Controls/src/Core/Element/Element.cs | 5 +- .../src/Core/VisualElement/VisualElement.cs | 12 ++++ .../TestClasses/BasicVisualElement.cs | 23 ++++++++ .../Core.UnitTests/VisualElementTests.cs | 55 +++++++++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) create mode 100644 src/Controls/tests/Core.UnitTests/TestClasses/BasicVisualElement.cs diff --git a/src/Controls/src/Core/Element/Element.cs b/src/Controls/src/Core/Element/Element.cs index 8d18b3e2458c..bf3dada20406 100644 --- a/src/Controls/src/Core/Element/Element.cs +++ b/src/Controls/src/Core/Element/Element.cs @@ -571,7 +571,7 @@ protected override void OnPropertyChanged([CallerMemberName] string propertyName { base.OnPropertyChanged(propertyName); - Handler?.UpdateValue(propertyName); + UpdateHandlerValue(propertyName); if (_effects?.Count > 0) { @@ -583,6 +583,9 @@ protected override void OnPropertyChanged([CallerMemberName] string propertyName } } + private protected virtual void UpdateHandlerValue(string property) => + Handler?.UpdateValue(property); + internal IEnumerable Descendants() => Descendants(); diff --git a/src/Controls/src/Core/VisualElement/VisualElement.cs b/src/Controls/src/Core/VisualElement/VisualElement.cs index 243e4a49eda8..bab6ad48f890 100644 --- a/src/Controls/src/Core/VisualElement/VisualElement.cs +++ b/src/Controls/src/Core/VisualElement/VisualElement.cs @@ -1942,6 +1942,18 @@ static double EnsurePositive(double value) return value; } + private protected override void UpdateHandlerValue(string property) + { + // The Height and Width Property aren't really meant to propagate back down to the handler + // These properties are only set with the platform code updates the VisualElement.Frame property + // indicating what the actual width and height are. + // WidthRequest and HeightRequest are the properties that we use to propagate changes down to the Handler. + if (this.Batched && (property == HeightProperty.PropertyName || property == WidthProperty.PropertyName)) + return; + + base.UpdateHandlerValue(property); + } + /// double IView.Width { diff --git a/src/Controls/tests/Core.UnitTests/TestClasses/BasicVisualElement.cs b/src/Controls/tests/Core.UnitTests/TestClasses/BasicVisualElement.cs new file mode 100644 index 000000000000..bfffbd55f44a --- /dev/null +++ b/src/Controls/tests/Core.UnitTests/TestClasses/BasicVisualElement.cs @@ -0,0 +1,23 @@ +using System.Threading.Tasks; +using Microsoft.Maui.Animations; + +using Microsoft.Maui.Handlers; + +namespace Microsoft.Maui.Controls.Core.UnitTests +{ + public class BasicVisualElement : VisualElement + { + } + + public class BasicVisualElementHandler : ViewHandler + { + public BasicVisualElementHandler(IPropertyMapper mapper, CommandMapper commandMapper = null) : base(mapper, commandMapper) + { + } + + protected override object CreatePlatformView() + { + return new object(); + } + } +} \ No newline at end of file diff --git a/src/Controls/tests/Core.UnitTests/VisualElementTests.cs b/src/Controls/tests/Core.UnitTests/VisualElementTests.cs index ba86e9bd57fc..531688f267b9 100644 --- a/src/Controls/tests/Core.UnitTests/VisualElementTests.cs +++ b/src/Controls/tests/Core.UnitTests/VisualElementTests.cs @@ -2,7 +2,12 @@ using System.Data.Common; using System.Threading.Tasks; using Microsoft.Maui.Controls.Shapes; + +using Microsoft.Maui.Controls.Hosting; using Microsoft.Maui.Graphics; +using Microsoft.Maui.Hosting; +using Microsoft.Maui.Platform; + using Microsoft.Maui.Handlers; using Microsoft.Maui.Primitives; using Xunit; @@ -237,5 +242,55 @@ public async Task ShadowDoesNotLeak() Assert.False(reference.IsAlive, "VisualElement should not be alive!"); } + + [Fact] + public void HandlerDoesntPropagateWidthChangesDuringBatchUpdates() + { + bool mapperCalled = false; + + var mapper = new PropertyMapper(ViewHandler.ViewMapper) + { + [nameof(IView.Height)] = (_,_) => mapperCalled = true, + [nameof(IView.Width)] = (_,_) => mapperCalled = true, + }; + + var mauiApp1 = MauiApp.CreateBuilder() + .UseMauiApp() + .ConfigureMauiHandlers(handlers => handlers.AddHandler((services) => new BasicVisualElementHandler(mapper))) + .Build(); + + var element = new BasicVisualElement(); + var platformView = element.ToPlatform(new MauiContext(mauiApp1.Services)); + + mapperCalled = false; + element.Frame = new Rect(0,0,100,100); + Assert.False(mapperCalled); + } + + [Fact] + public void HandlerDoesPropagateWidthChangesWhenUpdatedDuringSizedChanged() + { + bool mapperCalled = false; + + var mapper = new PropertyMapper(ViewHandler.ViewMapper) + { + [nameof(IView.Height)] = (_,_) => mapperCalled = true, + [nameof(IView.Width)] = (_,_) => mapperCalled = true, + }; + + var mauiApp1 = MauiApp.CreateBuilder() + .UseMauiApp() + .ConfigureMauiHandlers(handlers => handlers.AddHandler((services) => new BasicVisualElementHandler(mapper))) + .Build(); + + var element = new BasicVisualElement(); + var platformView = element.ToPlatform(new MauiContext(mauiApp1.Services)); + + element.SizeChanged += (_,_) => element.HeightRequest = 100; + mapperCalled = false; + element.Frame = new Rect(0,0,100,100); + + Assert.True(mapperCalled); + } } } From f3bb604f5f8d9cffad88a160cf93035abe5b1f57 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 15 May 2024 10:22:37 -0500 Subject: [PATCH 2/3] - fix comments and add more tests --- .../src/Core/VisualElement/VisualElement.cs | 9 +++--- .../Core.UnitTests/VisualElementTests.cs | 28 +++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/src/Controls/src/Core/VisualElement/VisualElement.cs b/src/Controls/src/Core/VisualElement/VisualElement.cs index bab6ad48f890..d2a9abe2f7da 100644 --- a/src/Controls/src/Core/VisualElement/VisualElement.cs +++ b/src/Controls/src/Core/VisualElement/VisualElement.cs @@ -1944,10 +1944,11 @@ static double EnsurePositive(double value) private protected override void UpdateHandlerValue(string property) { - // The Height and Width Property aren't really meant to propagate back down to the handler - // These properties are only set with the platform code updates the VisualElement.Frame property - // indicating what the actual width and height are. - // WidthRequest and HeightRequest are the properties that we use to propagate changes down to the Handler. + // The Height and Width properties aren't really meant to propagate back down to the handler. + // These properties are only set when the platform code updates the VisualElement.Frame property + // during an arrange pass, indicating what the actual width and height of the platform element are. + // WidthRequest and HeightRequest are the properties that we use to propagate changes down to the handler. + // These will still propagate down to the handler via the `OnRequestChanged` method. if (this.Batched && (property == HeightProperty.PropertyName || property == WidthProperty.PropertyName)) return; diff --git a/src/Controls/tests/Core.UnitTests/VisualElementTests.cs b/src/Controls/tests/Core.UnitTests/VisualElementTests.cs index 531688f267b9..ada2d595e1e0 100644 --- a/src/Controls/tests/Core.UnitTests/VisualElementTests.cs +++ b/src/Controls/tests/Core.UnitTests/VisualElementTests.cs @@ -292,5 +292,33 @@ public void HandlerDoesPropagateWidthChangesWhenUpdatedDuringSizedChanged() Assert.True(mapperCalled); } + + [Fact] + public void WidthAndHeightRequestPropagateToHandler() + { + bool mapperCalled = false; + + var mapper = new PropertyMapper(ViewHandler.ViewMapper) + { + [nameof(IView.Height)] = (_,_) => mapperCalled = true, + [nameof(IView.Width)] = (_,_) => mapperCalled = true, + }; + + var mauiApp1 = MauiApp.CreateBuilder() + .UseMauiApp() + .ConfigureMauiHandlers(handlers => handlers.AddHandler((services) => new BasicVisualElementHandler(mapper))) + .Build(); + + var element = new BasicVisualElement(); + var platformView = element.ToPlatform(new MauiContext(mauiApp1.Services)); + + mapperCalled = false; + element.WidthRequest = 99; + Assert.True(mapperCalled); + + mapperCalled = false; + element.HeightRequest = 99; + Assert.True(mapperCalled); + } } } From 923fcbebb04466557220d10c2540f922d2c66c97 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 15 May 2024 10:56:04 -0500 Subject: [PATCH 3/3] - update comment and test --- .../src/Core/VisualElement/VisualElement.cs | 10 +++++----- .../tests/Core.UnitTests/VisualElementTests.cs | 17 ++++++++++------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/Controls/src/Core/VisualElement/VisualElement.cs b/src/Controls/src/Core/VisualElement/VisualElement.cs index d2a9abe2f7da..fd9d905470fe 100644 --- a/src/Controls/src/Core/VisualElement/VisualElement.cs +++ b/src/Controls/src/Core/VisualElement/VisualElement.cs @@ -1944,11 +1944,11 @@ static double EnsurePositive(double value) private protected override void UpdateHandlerValue(string property) { - // The Height and Width properties aren't really meant to propagate back down to the handler. - // These properties are only set when the platform code updates the VisualElement.Frame property - // during an arrange pass, indicating what the actual width and height of the platform element are. - // WidthRequest and HeightRequest are the properties that we use to propagate changes down to the handler. - // These will still propagate down to the handler via the `OnRequestChanged` method. + // The HeightProperty and WidthProperty are not designed to propagate back to the handler. + // Instead, we use WidthRequestProperty and HeightRequestProperty to propagate changes to the handler. + // HeightProperty and WidthProperty are readonly and only update when the VisualElement.Frame property is set + // during an arrange pass, which indicates the actual width and height of the platform element. + // Changes to WidthRequestProperty and HeightRequestProperty will propagate to the handler via the `OnRequestChanged` method. if (this.Batched && (property == HeightProperty.PropertyName || property == WidthProperty.PropertyName)) return; diff --git a/src/Controls/tests/Core.UnitTests/VisualElementTests.cs b/src/Controls/tests/Core.UnitTests/VisualElementTests.cs index ada2d595e1e0..76cd9e8fc7c6 100644 --- a/src/Controls/tests/Core.UnitTests/VisualElementTests.cs +++ b/src/Controls/tests/Core.UnitTests/VisualElementTests.cs @@ -296,12 +296,13 @@ public void HandlerDoesPropagateWidthChangesWhenUpdatedDuringSizedChanged() [Fact] public void WidthAndHeightRequestPropagateToHandler() { - bool mapperCalled = false; + int heightMapperCalled = 0; + int widthMapperCalled = 0; var mapper = new PropertyMapper(ViewHandler.ViewMapper) { - [nameof(IView.Height)] = (_,_) => mapperCalled = true, - [nameof(IView.Width)] = (_,_) => mapperCalled = true, + [nameof(IView.Height)] = (_,_) => heightMapperCalled++, + [nameof(IView.Width)] = (_,_) => widthMapperCalled++, }; var mauiApp1 = MauiApp.CreateBuilder() @@ -312,13 +313,15 @@ public void WidthAndHeightRequestPropagateToHandler() var element = new BasicVisualElement(); var platformView = element.ToPlatform(new MauiContext(mauiApp1.Services)); - mapperCalled = false; + heightMapperCalled = 0; + widthMapperCalled = 0; element.WidthRequest = 99; - Assert.True(mapperCalled); + Assert.Equal(1, heightMapperCalled); + Assert.Equal(1, widthMapperCalled); - mapperCalled = false; element.HeightRequest = 99; - Assert.True(mapperCalled); + Assert.Equal(2, heightMapperCalled); + Assert.Equal(2, widthMapperCalled); } } }