Cleanup Popup control.

src/Avalonia.Controls/Primitives/Popup.cs

@@ -2,12 +2,10 @@
 // Licensed under the MIT license. See licence.md file in the project root for full license information.
 
 using System;
-using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using System.Reactive.Disposables;
 using Avalonia.Controls.Presenters;
-using Avalonia.Data;
 using Avalonia.Input;
 using Avalonia.Input.Raw;
 using Avalonia.Interactivity;
@@ -15,6 +13,8 @@ using Avalonia.LogicalTree;
 using Avalonia.Metadata;
 using Avalonia.VisualTree;
 
+#nullable enable
+
 namespace Avalonia.Controls.Primitives
 {
     /// <summary>
@@ -43,11 +43,13 @@ namespace Avalonia.Controls.Primitives
         public static readonly StyledProperty<PlacementMode> PlacementModeProperty =
             AvaloniaProperty.Register<Popup, PlacementMode>(nameof(PlacementMode), defaultValue: PlacementMode.Bottom);
 
+#pragma warning disable 618
         /// <summary>
         /// Defines the <see cref="ObeyScreenEdges"/> property.
         /// </summary>
         public static readonly StyledProperty<bool> ObeyScreenEdgesProperty =
             AvaloniaProperty.Register<Popup, bool>(nameof(ObeyScreenEdges), true);
+#pragma warning restore 618
 
         /// <summary>
         /// Defines the <see cref="HorizontalOffset"/> property.
@@ -80,12 +82,8 @@ namespace Avalonia.Controls.Primitives
             AvaloniaProperty.Register<Popup, bool>(nameof(Topmost));
 
         private bool _isOpen;
-        private IPopupHost _popupHost;
+        private bool _ignoreIsOpenChanged;
-        private TopLevel _topLevel;
+        private PopupOpenState? _openState;
-        private IDisposable _nonClientListener;
-        private IDisposable _presenterSubscription;
-        bool _ignoreIsOpenChanged = false;
-        private List<IDisposable> _bindings = new List<IDisposable>();
 
         /// <summary>
         /// Initializes static members of the <see cref="Popup"/> class.
@@ -97,22 +95,17 @@ namespace Avalonia.Controls.Primitives
             IsOpenProperty.Changed.AddClassHandler<Popup>((x, e) => x.IsOpenChanged(e));
         }
 
-        public Popup()
-        {
-            
-        }
-
         /// <summary>
         /// Raised when the popup closes.
         /// </summary>
-        public event EventHandler Closed;
+        public event EventHandler? Closed;
 
         /// <summary>
         /// Raised when the popup opens.
         /// </summary>
-        public event EventHandler Opened;
+        public event EventHandler? Opened;
 
-        public IPopupHost Host => _popupHost;
+        public IPopupHost? Host => _openState?.PopupHost;
 
         /// <summary>
         /// Gets or sets the control to display in the popup.
@@ -131,7 +124,7 @@ namespace Avalonia.Controls.Primitives
         /// This property allows a client to customize the behaviour of the popup by injecting
         /// a specialized dependency resolver into the <see cref="PopupRoot"/>'s constructor.
         /// </remarks>
-        public IAvaloniaDependencyResolver DependencyResolver
+        public IAvaloniaDependencyResolver? DependencyResolver
         {
             get;
             set;
@@ -211,7 +204,7 @@ namespace Avalonia.Controls.Primitives
         /// <summary>
         /// Gets the root of the popup window.
         /// </summary>
-        IVisual IVisualTreeHost.Root => _popupHost?.HostedVisualTreeRoot;
+        IVisual? IVisualTreeHost.Root => _openState?.PopupHost.HostedVisualTreeRoot;
 
         /// <summary>
         /// Opens the popup.
@@ -219,50 +212,91 @@ namespace Avalonia.Controls.Primitives
         public void Open()
         {
             // Popup is currently open
-            if (_topLevel != null)
+            if (_openState != null)
+            {
                 return;
-            CloseCurrent();
+            }
+
             var placementTarget = PlacementTarget ?? this.GetLogicalAncestors().OfType<IVisual>().FirstOrDefault();
+
             if (placementTarget == null)
+            {
                 throw new InvalidOperationException("Popup has no logical parent and PlacementTarget is null");
+            }
             
-            _topLevel = placementTarget.GetVisualRoot() as TopLevel;
+            var topLevel = placementTarget.VisualRoot as TopLevel;
 
-            if (_topLevel == null)
+            if (topLevel == null)
             {
                 throw new InvalidOperationException(
                     "Attempted to open a popup not attached to a TopLevel");
             }
 
-            _popupHost = OverlayPopupHost.CreatePopupHost(placementTarget, DependencyResolver);
+            var popupHost = OverlayPopupHost.CreatePopupHost(placementTarget, DependencyResolver);
+
+            var handlerCleanup = new CompositeDisposable(5);
 
-            _bindings.Add(_popupHost.BindConstraints(this, WidthProperty, MinWidthProperty, MaxWidthProperty,
+            void DeferCleanup(IDisposable? disposable)
+            {
+                if (disposable is null)
+                {
+                    return;
+                }
+
+                handlerCleanup.Add(disposable);
+            }
+
+            DeferCleanup(popupHost.BindConstraints(this, WidthProperty, MinWidthProperty, MaxWidthProperty,
                 HeightProperty, MinHeightProperty, MaxHeightProperty, TopmostProperty));
 
-            _popupHost.SetChild(Child);
+            popupHost.SetChild(Child);
-            ((ISetLogicalParent)_popupHost).SetParent(this);
+            ((ISetLogicalParent)popupHost).SetParent(this);
-            _popupHost.ConfigurePosition(placementTarget,
+
-                PlacementMode, new Point(HorizontalOffset, VerticalOffset));
+            popupHost.ConfigurePosition(
-            _popupHost.TemplateApplied += RootTemplateApplied;
+                placementTarget,
-            
+                PlacementMode, 
-            var window = _topLevel as Window;
+                new Point(HorizontalOffset, VerticalOffset));
-            if (window != null)
+
+            DeferCleanup(SubscribeToEventHandler<IPopupHost, EventHandler<TemplateAppliedEventArgs>>(popupHost, RootTemplateApplied,
+                (x, handler) => x.TemplateApplied += handler,
+                (x, handler) => x.TemplateApplied -= handler));
+
+            if (topLevel is Window window)
             {
-                window.Deactivated += WindowDeactivated;
+                DeferCleanup(SubscribeToEventHandler<Window, EventHandler>(window, WindowDeactivated,
+                    (x, handler) => x.Deactivated += handler,
+                    (x, handler) => x.Deactivated -= handler));
             }
             else
             {
-                var parentPopuproot = _topLevel as PopupRoot;
+                var parentPopupRoot = topLevel as PopupRoot;
-                if (parentPopuproot?.Parent is Popup popup)
+
+                if (parentPopupRoot?.Parent is Popup popup)
                 {
-                    popup.Closed += ParentClosed;
+                    DeferCleanup(SubscribeToEventHandler<Popup, EventHandler>(popup, ParentClosed,
+                        (x, handler) => x.Closed += handler,
+                        (x, handler) => x.Closed -= handler));
                 }
             }
-            _topLevel.AddHandler(PointerPressedEvent, PointerPressedOutside, RoutingStrategies.Tunnel);
-            _nonClientListener = InputManager.Instance?.Process.Subscribe(ListenForNonClientClick);
-        
 
-            _popupHost.Show();
+            DeferCleanup(topLevel.AddHandler(PointerPressedEvent, PointerPressedOutside, RoutingStrategies.Tunnel));
+
+            DeferCleanup(InputManager.Instance?.Process.Subscribe(ListenForNonClientClick));
+
+            var cleanupPopup = Disposable.Create((popupHost, handlerCleanup), state =>
+            {
+                state.handlerCleanup.Dispose();
+
+                state.popupHost.SetChild(null);
+                state.popupHost.Hide();
+
+                ((ISetLogicalParent)state.popupHost).SetParent(null);
+                state.popupHost.Dispose();
+            });
+
+            _openState = new PopupOpenState(topLevel, popupHost, cleanupPopup);
+
+            popupHost.Show();
 
             using (BeginIgnoringIsOpen())
             {
@@ -277,19 +311,19 @@ namespace Avalonia.Controls.Primitives
         /// </summary>
         public void Close()
         {
-            if (_topLevel is null)
+            if (_openState is null)
             {
-                return;
+                using (BeginIgnoringIsOpen())
-            }
+                {
+                    IsOpen = false;
+                }
 
-            if (_popupHost != null)
+                return;
-            {
-                _popupHost.TemplateApplied -= RootTemplateApplied;
             }
 
-            _presenterSubscription?.Dispose();
+            _openState.Dispose();
+            _openState = null;
 
-            CloseCurrent();
             using (BeginIgnoringIsOpen())
             {
                 IsOpen = false;
@@ -298,41 +332,6 @@ namespace Avalonia.Controls.Primitives
             Closed?.Invoke(this, EventArgs.Empty);
         }
 
-        void CloseCurrent()
-        {
-            if (_topLevel != null)
-            {
-                _topLevel.RemoveHandler(PointerPressedEvent, PointerPressedOutside);
-                var window = _topLevel as Window;
-                if (window != null)
-                    window.Deactivated -= WindowDeactivated;
-                else
-                {
-                    var parentPopuproot = _topLevel as PopupRoot;
-                    if (parentPopuproot?.Parent is Popup popup)
-                    {
-                        popup.Closed -= ParentClosed;
-                    }
-                }
-                _nonClientListener?.Dispose();
-                _nonClientListener = null;
-                
-                _topLevel = null;
-            }
-            if (_popupHost != null)
-            {
-                foreach(var b in _bindings)
-                    b.Dispose();
-                _bindings.Clear();
-                _popupHost.SetChild(null);
-                _popupHost.Hide();
-                ((ISetLogicalParent)_popupHost).SetParent(null);
-                _popupHost.Dispose();
-                _popupHost = null;
-            }
-
-        }
-
         /// <summary>
         /// Measures the control.
         /// </summary>
@@ -350,6 +349,12 @@ namespace Avalonia.Controls.Primitives
             Close();
         }
 
+        private static IDisposable SubscribeToEventHandler<T, TEventHandler>(T target, TEventHandler handler, Action<T, TEventHandler> subscribe, Action<T, TEventHandler> unsubscribe)
+        {
+            subscribe(target, handler);
+
+            return Disposable.Create((unsubscribe, target, handler), state => state.unsubscribe(state.target, state.handler));
+        }
 
         /// <summary>
         /// Called when the <see cref="IsOpen"/> property changes.
@@ -378,7 +383,7 @@ namespace Avalonia.Controls.Primitives
         {
             LogicalChildren.Clear();
 
-            ((ISetLogicalParent)e.OldValue)?.SetParent(null);
+            ((ISetLogicalParent?)e.OldValue)?.SetParent(null);
 
             if (e.NewValue != null)
             {
@@ -411,22 +416,28 @@ namespace Avalonia.Controls.Primitives
 
         private void RootTemplateApplied(object sender, TemplateAppliedEventArgs e)
         {
-            _popupHost.TemplateApplied -= RootTemplateApplied;
+            if (_openState is null)
-
-            if (_presenterSubscription != null)
             {
-                _presenterSubscription.Dispose();
+                return;
-                _presenterSubscription = null;
             }
 
+            var popupHost = _openState.PopupHost;
+
+            popupHost.TemplateApplied -= RootTemplateApplied;
+
+            _openState.CleanupPresenterSubscription();
+
             // If the Popup appears in a control template, then the child controls
             // that appear in the popup host need to have their TemplatedParent
             // properties set.
-            if (TemplatedParent != null)
+            if (TemplatedParent != null && popupHost.Presenter != null)
             {
-                _popupHost.Presenter?.ApplyTemplate();
+                popupHost.Presenter.ApplyTemplate();
-                _popupHost.Presenter?.GetObservable(ContentPresenter.ChildProperty)
+
+                var presenterSubscription = popupHost.Presenter.GetObservable(ContentPresenter.ChildProperty)
                     .Subscribe(SetTemplatedParentAndApplyChildTemplates);
+
+                _openState.DeferCleanupPresenterSubscription(presenterSubscription);
             }
         }
 
@@ -445,7 +456,7 @@ namespace Avalonia.Controls.Primitives
 
                 if (!(control is IPresenter) && control.TemplatedParent == templatedParent)
                 {
-                    foreach (IControl child in control.GetVisualChildren())
+                    foreach (IControl child in control.VisualChildren)
                     {
                         SetTemplatedParentAndApplyChildTemplates(child);
                     }
@@ -455,22 +466,41 @@ namespace Avalonia.Controls.Primitives
 
         private bool IsChildOrThis(IVisual child)
         {
-            IVisual root = child.GetVisualRoot();
+            if (_openState is null)
-            while (root is IHostedVisualTreeRoot hostedRoot )
             {
-                if (root == this._popupHost)
+                return false;
+            }
+
+            var popupHost = _openState.PopupHost;
+
+            IVisual? root = child.VisualRoot;
+            
+            while (root is IHostedVisualTreeRoot hostedRoot)
+            {
+                if (root == popupHost)
+                {
                     return true;
-                root = hostedRoot.Host?.GetVisualRoot();
+                }
+
+                root = hostedRoot.Host?.VisualRoot;
             }
+
             return false;
         }
         
         public bool IsInsidePopup(IVisual visual)
         {
-            return _popupHost != null && ((IVisual)_popupHost)?.IsVisualAncestorOf(visual) == true;
+            if (_openState is null)
+            {
+                return false;
+            }
+
+            var popupHost = _openState.PopupHost;
+
+            return popupHost != null && ((IVisual)popupHost).IsVisualAncestorOf(visual);
         }
 
-        public bool IsPointerOverPopup => ((IInputElement)_popupHost).IsPointerOver;
+        public bool IsPointerOverPopup => ((IInputElement?)_openState?.PopupHost)?.IsPointerOver ?? false;
 
         private void WindowDeactivated(object sender, EventArgs e)
         {
@@ -508,5 +538,48 @@ namespace Avalonia.Controls.Primitives
                 _owner._ignoreIsOpenChanged = false;
             }
         }
+
+        private class PopupOpenState : IDisposable
+        {
+            private readonly IDisposable _cleanup;
+            private IDisposable? _presenterCleanup;
+
+            public PopupOpenState(TopLevel topLevel, IPopupHost popupHost, IDisposable cleanup)
+            {
+                TopLevel = topLevel;
+                PopupHost = popupHost;
+                _cleanup = cleanup;
+            }
+
+            public TopLevel TopLevel { get; }
+
+            public IPopupHost PopupHost { get; }
+
+            public void CleanupPresenterSubscription()
+            {
+                _presenterCleanup?.Dispose();
+                _presenterCleanup = null;
+            }
+
+            public void DeferCleanupPresenterSubscription(IDisposable presenterCleanup)
+            {
+                if (_presenterCleanup != null)
+                {
+                    // Shouldn't really happen since we always cleanup before calling this.
+                    Debug.Assert(false);
+
+                    _presenterCleanup.Dispose();
+                }
+
+                _presenterCleanup = presenterCleanup;
+            }
+
+            public void Dispose()
+            {
+                _presenterCleanup?.Dispose();
+
+                _cleanup.Dispose();
+            }
+        }
     }
 }