From 324731f9df859011ca268c85a7c8c97f7cdc37d3 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Mon, 8 Aug 2022 13:03:44 -0500 Subject: [PATCH] Disconnect old handler after setting new handler (#8781) * Disconnect old handler after setting new handler * - add check for disconnect call * - more disconnect * - fix up order of setting handlers * - fix cast check * Update TestNavigationPage.cs * - change to xunit * - fix namespaces --- .../Core/HandlerImpl/Element/Element.Impl.cs | 6 ++ .../src/Core/HandlerImpl/Shell.Impl.cs | 9 --- .../net-android/PublicAPI.Unshipped.txt | 1 - .../Core.UnitTests/HandlerLifeCycleTests.cs | 55 +++++++++++++++++++ .../TestClasses/TestNavigationPage.cs | 5 +- .../src/Handlers/Element/ElementHandler.cs | 9 ++- 6 files changed, 71 insertions(+), 14 deletions(-) diff --git a/src/Controls/src/Core/HandlerImpl/Element/Element.Impl.cs b/src/Controls/src/Core/HandlerImpl/Element/Element.Impl.cs index 12884a3b3077..3499abecefa7 100644 --- a/src/Controls/src/Core/HandlerImpl/Element/Element.Impl.cs +++ b/src/Controls/src/Core/HandlerImpl/Element/Element.Impl.cs @@ -60,6 +60,12 @@ void SetHandler(IElementHandler newHandler) _handler = newHandler; + // Only call disconnect if the previous handler is still connected to this virtual view. + // If a handler is being reused for a different VirtualView then the virtual + // view would have already rolled + if (_previousHandler?.VirtualView == this) + _previousHandler?.DisconnectHandler(); + if (_handler?.VirtualView != this) _handler?.SetVirtualView(this); diff --git a/src/Controls/src/Core/HandlerImpl/Shell.Impl.cs b/src/Controls/src/Core/HandlerImpl/Shell.Impl.cs index ada29875c1ef..a4f2528c5074 100644 --- a/src/Controls/src/Core/HandlerImpl/Shell.Impl.cs +++ b/src/Controls/src/Core/HandlerImpl/Shell.Impl.cs @@ -28,14 +28,5 @@ protected override void OnPropertyChanged([CallerMemberName] string propertyName if (propertyName == Shell.FlyoutIsPresentedProperty.PropertyName) Handler?.UpdateValue(nameof(IFlyoutView.IsPresented)); } - -#if ANDROID - protected override void OnHandlerChanging(HandlerChangingEventArgs args) - { - base.OnHandlerChanging(args); - args.OldHandler?.DisconnectHandler(); - } - -#endif } } diff --git a/src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt b/src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt index 2ad92603d5ae..a14856db69c3 100644 --- a/src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt +++ b/src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt @@ -17,4 +17,3 @@ override Microsoft.Maui.Controls.TemplatedView.MeasureOverride(double widthConst *REMOVED*override Microsoft.Maui.Controls.FlexLayout.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size static readonly Microsoft.Maui.Controls.VisualElement.ZIndexProperty -> Microsoft.Maui.Controls.BindableProperty! *REMOVED*override Microsoft.Maui.Controls.Platform.ControlsAccessibilityDelegate.OnInitializeAccessibilityNodeInfo(Android.Views.View? host, AndroidX.Core.View.Accessibility.AccessibilityNodeInfoCompat? info) -> void -~override Microsoft.Maui.Controls.Shell.OnHandlerChanging(Microsoft.Maui.Controls.HandlerChangingEventArgs args) -> void diff --git a/src/Controls/tests/Core.UnitTests/HandlerLifeCycleTests.cs b/src/Controls/tests/Core.UnitTests/HandlerLifeCycleTests.cs index de6514f724f6..e447f9a56f2d 100644 --- a/src/Controls/tests/Core.UnitTests/HandlerLifeCycleTests.cs +++ b/src/Controls/tests/Core.UnitTests/HandlerLifeCycleTests.cs @@ -2,8 +2,11 @@ using System.Collections.Generic; using System.Linq; using System.Reflection; +using Microsoft.Maui.Controls.Hosting; using Microsoft.Maui.Controls.Internals; using Microsoft.Maui.Graphics; +using Microsoft.Maui.Hosting; +using Microsoft.Maui.Platform; using Xunit; using Xunit.Sdk; @@ -12,6 +15,58 @@ namespace Microsoft.Maui.Controls.Core.UnitTests public class HandlerLifeCycleTests : BaseTestFixture { + [Fact] + public void SettingHandlerToNullDisconnectsHandlerFromVirtualView() + { + var mauiApp1 = MauiApp.CreateBuilder() + .UseMauiApp() + .ConfigureMauiHandlers(handlers => handlers.AddHandler()) + .Build(); + + LifeCycleButton button = new LifeCycleButton(); + + MauiContext mauiContext1 = new MauiContext(mauiApp1.Services); + var handler1 = button.ToHandler(mauiContext1); + button.Handler = null; + Assert.NotEqual(button, handler1.VirtualView); + } + + [Fact] + public void SettingNewHandlerDisconnectsOldHandler() + { + var mauiApp1 = MauiApp.CreateBuilder() + .UseMauiApp() + .ConfigureMauiHandlers(handlers => handlers.AddHandler()) + .Build(); + + LifeCycleButton button = new LifeCycleButton(); + + MauiContext mauiContext1 = new MauiContext(mauiApp1.Services); + var handler1 = button.ToHandler(mauiContext1); + + var mauiApp2 = MauiApp.CreateBuilder() + .UseMauiApp() + .ConfigureMauiHandlers(handlers => handlers.AddHandler()) + .Build(); + + MauiContext mauiContext2 = new MauiContext(mauiApp2.Services); + + List changingArgs = new List(); + button.HandlerChanging += (s, a) => + { + Assert.Equal(handler1, a.OldHandler); + Assert.NotNull(a.NewHandler); + + changingArgs.Add(a); + }; + + var handler2 = button.ToHandler(mauiContext2); + + Assert.NotEqual(button, handler1.VirtualView); + Assert.Equal(button, handler2.VirtualView); + Assert.Single(changingArgs); + } + [Fact] public void ChangingAndChangedBothFireInitially() { diff --git a/src/Controls/tests/Core.UnitTests/TestClasses/TestNavigationPage.cs b/src/Controls/tests/Core.UnitTests/TestClasses/TestNavigationPage.cs index db34182996be..e3816bbc9f15 100644 --- a/src/Controls/tests/Core.UnitTests/TestClasses/TestNavigationPage.cs +++ b/src/Controls/tests/Core.UnitTests/TestClasses/TestNavigationPage.cs @@ -62,8 +62,9 @@ public void CompleteCurrentNavigation() var newStack = CurrentNavigationRequest.NavigationStack.ToList(); CurrentNavigationRequest = null; - (VirtualView as IStackNavigation) - .NavigationFinished(newStack); + + if ((this as IElementHandler).VirtualView is IStackNavigation sn) + sn.NavigationFinished(newStack); } async void RequestNavigation(NavigationRequest navigationRequest) diff --git a/src/Core/src/Handlers/Element/ElementHandler.cs b/src/Core/src/Handlers/Element/ElementHandler.cs index edffdf7d717a..47ef3fcd97a1 100644 --- a/src/Core/src/Handlers/Element/ElementHandler.cs +++ b/src/Core/src/Handlers/Element/ElementHandler.cs @@ -43,8 +43,6 @@ public virtual void SetVirtualView(IElement view) return; var oldVirtualView = VirtualView; - if (oldVirtualView?.Handler != null) - oldVirtualView.Handler = null; bool setupPlatformView = oldVirtualView == null; @@ -54,6 +52,13 @@ public virtual void SetVirtualView(IElement view) if (VirtualView.Handler != this) VirtualView.Handler = this; + // We set the previous virtual view to null after setting it on the incoming virtual view. + // This makes it easier for the incoming virtual view to have influence + // on how the exchange of handlers happens. + // We will just set the handler to null ourselves as a last resort cleanup + if (oldVirtualView?.Handler != null) + oldVirtualView.Handler = null; + if (setupPlatformView) { ConnectHandler(PlatformView);