From 01edb8c8b128318a3c12d85be433af429dd1639f Mon Sep 17 00:00:00 2001 From: Adir Date: Wed, 7 Oct 2020 19:25:18 +0300 Subject: [PATCH 1/9] Added potential fix for hotkey manager instances not clearing up after detaching from root --- src/Avalonia.Controls/HotkeyManager.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Avalonia.Controls/HotkeyManager.cs b/src/Avalonia.Controls/HotkeyManager.cs index 95752e7875..9a2b4f2d0a 100644 --- a/src/Avalonia.Controls/HotkeyManager.cs +++ b/src/Avalonia.Controls/HotkeyManager.cs @@ -55,6 +55,11 @@ namespace Avalonia.Controls private void OnParentChanged(TopLevel control) { + if (control == null) + { + Stop(); + return; + } Unregister(); _root = control; Register(); From 188192c1259c34abcbcaaca1f5c4ede0d7259850 Mon Sep 17 00:00:00 2001 From: Adir Date: Wed, 7 Oct 2020 20:57:41 +0300 Subject: [PATCH 2/9] Changed HotkeyManager.cs to stop/init based on detaching/attaching to visual tree --- src/Avalonia.Controls/HotkeyManager.cs | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/src/Avalonia.Controls/HotkeyManager.cs b/src/Avalonia.Controls/HotkeyManager.cs index 9a2b4f2d0a..bff25d6dad 100644 --- a/src/Avalonia.Controls/HotkeyManager.cs +++ b/src/Avalonia.Controls/HotkeyManager.cs @@ -2,6 +2,7 @@ using System; using System.Windows.Input; using Avalonia.Controls.Utils; using Avalonia.Input; +using Avalonia.LogicalTree; namespace Avalonia.Controls { @@ -45,6 +46,8 @@ namespace Avalonia.Controls { _control = control; _wrapper = new HotkeyCommandWrapper(_control); + _control.DetachedFromVisualTree += ControlOnDetachedFromVisualTree; + _control.AttachedToLogicalTree += ControlOnAttachedToLogicalTree; } public void Init() @@ -53,13 +56,19 @@ namespace Avalonia.Controls _parentSub = AncestorFinder.Create(_control).Subscribe(OnParentChanged); } + private void ControlOnAttachedToLogicalTree(object sender, LogicalTreeAttachmentEventArgs e) + { + Stop(); + Init(); + } + + private void ControlOnDetachedFromVisualTree(object sender, VisualTreeAttachmentEventArgs e) + { + Stop(); + } + private void OnParentChanged(TopLevel control) { - if (control == null) - { - Stop(); - return; - } Unregister(); _root = control; Register(); @@ -89,7 +98,7 @@ namespace Avalonia.Controls { if (_root != null && _hotkey != null) { - _binding = new KeyBinding() {Gesture = _hotkey, Command = _wrapper}; + _binding = new KeyBinding() { Gesture = _hotkey, Command = _wrapper }; _root.KeyBindings.Add(_binding); } } @@ -107,11 +116,12 @@ namespace Avalonia.Controls HotKeyProperty.Changed.Subscribe(args => { var control = args.Sender as IControl; - if (args.OldValue != null|| control == null) + if (args.OldValue != null || control == null) return; new Manager(control).Init(); }); } + public static void SetHotKey(AvaloniaObject target, KeyGesture value) => target.SetValue(HotKeyProperty, value); public static KeyGesture GetHotKey(AvaloniaObject target) => target.GetValue(HotKeyProperty); } From ecba5f65f05da4917b6a454e81abf3f99b76ceb4 Mon Sep 17 00:00:00 2001 From: Adir Date: Wed, 7 Oct 2020 23:11:50 +0300 Subject: [PATCH 3/9] Reverted HotkeyManager.cs Changed Button.cs to remove it's HotKey property when detaching from visual tree --- src/Avalonia.Controls/Button.cs | 12 ++++++++++-- src/Avalonia.Controls/HotkeyManager.cs | 14 -------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/Avalonia.Controls/Button.cs b/src/Avalonia.Controls/Button.cs index e94d00b2ff..c541a384b0 100644 --- a/src/Avalonia.Controls/Button.cs +++ b/src/Avalonia.Controls/Button.cs @@ -80,6 +80,7 @@ namespace Avalonia.Controls private ICommand _command; private bool _commandCanExecute = true; + private KeyGesture _hotkey; /// /// Initializes static members of the class. @@ -168,11 +169,13 @@ namespace Avalonia.Controls private set { SetValue(IsPressedProperty, value); } } - protected override bool IsEnabledCore => base.IsEnabledCore && _commandCanExecute; + protected override bool IsEnabledCore => base.IsEnabledCore && _commandCanExecute; /// protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) { + if (_hotkey != null) + HotKey = _hotkey; base.OnAttachedToVisualTree(e); if (IsDefault) @@ -182,6 +185,7 @@ namespace Avalonia.Controls ListenForDefault(inputElement); } } + if (IsCancel) { if (e.Root is IInputElement inputElement) @@ -194,6 +198,8 @@ namespace Avalonia.Controls /// protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e) { + _hotkey = HotKey; + HotKey = null; base.OnDetachedFromVisualTree(e); if (IsDefault) @@ -239,6 +245,7 @@ namespace Avalonia.Controls { OnClick(); } + IsPressed = true; e.Handled = true; } @@ -255,6 +262,7 @@ namespace Avalonia.Controls { OnClick(); } + IsPressed = false; e.Handled = true; } @@ -309,7 +317,7 @@ namespace Avalonia.Controls } } } - + protected override void OnPointerCaptureLost(PointerCaptureLostEventArgs e) { IsPressed = false; diff --git a/src/Avalonia.Controls/HotkeyManager.cs b/src/Avalonia.Controls/HotkeyManager.cs index bff25d6dad..5972a0fdd9 100644 --- a/src/Avalonia.Controls/HotkeyManager.cs +++ b/src/Avalonia.Controls/HotkeyManager.cs @@ -2,7 +2,6 @@ using System; using System.Windows.Input; using Avalonia.Controls.Utils; using Avalonia.Input; -using Avalonia.LogicalTree; namespace Avalonia.Controls { @@ -46,8 +45,6 @@ namespace Avalonia.Controls { _control = control; _wrapper = new HotkeyCommandWrapper(_control); - _control.DetachedFromVisualTree += ControlOnDetachedFromVisualTree; - _control.AttachedToLogicalTree += ControlOnAttachedToLogicalTree; } public void Init() @@ -56,17 +53,6 @@ namespace Avalonia.Controls _parentSub = AncestorFinder.Create(_control).Subscribe(OnParentChanged); } - private void ControlOnAttachedToLogicalTree(object sender, LogicalTreeAttachmentEventArgs e) - { - Stop(); - Init(); - } - - private void ControlOnDetachedFromVisualTree(object sender, VisualTreeAttachmentEventArgs e) - { - Stop(); - } - private void OnParentChanged(TopLevel control) { Unregister(); From 7c952cfc5fb3063b03b3860d7faf1b69e6db1b46 Mon Sep 17 00:00:00 2001 From: Adir Date: Thu, 8 Oct 2020 16:33:32 +0300 Subject: [PATCH 4/9] Changed Hotkey reset to detatch from logical tree (was visual tree) --- src/Avalonia.Controls/Button.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/Avalonia.Controls/Button.cs b/src/Avalonia.Controls/Button.cs index c541a384b0..046253b3cd 100644 --- a/src/Avalonia.Controls/Button.cs +++ b/src/Avalonia.Controls/Button.cs @@ -174,8 +174,6 @@ namespace Avalonia.Controls /// protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) { - if (_hotkey != null) - HotKey = _hotkey; base.OnAttachedToVisualTree(e); if (IsDefault) @@ -198,8 +196,6 @@ namespace Avalonia.Controls /// protected override void OnDetachedFromVisualTree(VisualTreeAttachmentEventArgs e) { - _hotkey = HotKey; - HotKey = null; base.OnDetachedFromVisualTree(e); if (IsDefault) @@ -213,6 +209,8 @@ namespace Avalonia.Controls protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e) { + if (_hotkey != null) + HotKey = _hotkey; base.OnAttachedToLogicalTree(e); if (Command != null) @@ -223,6 +221,12 @@ namespace Avalonia.Controls protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e) { + if (HotKey != null) + { + _hotkey = HotKey; + HotKey = null; + } + base.OnDetachedFromLogicalTree(e); if (Command != null) From 3c302f72f236a94d5b1d3f31fcab52d351d31625 Mon Sep 17 00:00:00 2001 From: Adir Date: Thu, 8 Oct 2020 18:03:52 +0300 Subject: [PATCH 5/9] Added docs explaining why need to change Hotkey on attach/detach from logical tree Added the same logic to MenuItem.cs Reverted auto indentation in Button.cs --- src/Avalonia.Controls/Button.cs | 9 ++++----- src/Avalonia.Controls/MenuItem.cs | 11 +++++++++++ 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/Avalonia.Controls/Button.cs b/src/Avalonia.Controls/Button.cs index 046253b3cd..87fabf5ed7 100644 --- a/src/Avalonia.Controls/Button.cs +++ b/src/Avalonia.Controls/Button.cs @@ -169,7 +169,7 @@ namespace Avalonia.Controls private set { SetValue(IsPressedProperty, value); } } - protected override bool IsEnabledCore => base.IsEnabledCore && _commandCanExecute; + protected override bool IsEnabledCore => base.IsEnabledCore && _commandCanExecute; /// protected override void OnAttachedToVisualTree(VisualTreeAttachmentEventArgs e) @@ -183,7 +183,6 @@ namespace Avalonia.Controls ListenForDefault(inputElement); } } - if (IsCancel) { if (e.Root is IInputElement inputElement) @@ -210,6 +209,7 @@ namespace Avalonia.Controls protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e) { if (_hotkey != null) + // Control attached again, set Hotkey to create a hotkey manager for this control HotKey = _hotkey; base.OnAttachedToLogicalTree(e); @@ -223,6 +223,7 @@ namespace Avalonia.Controls { if (HotKey != null) { + // This will cause the hotkey manager to dispose the observer and the reference to this control _hotkey = HotKey; HotKey = null; } @@ -249,7 +250,6 @@ namespace Avalonia.Controls { OnClick(); } - IsPressed = true; e.Handled = true; } @@ -266,7 +266,6 @@ namespace Avalonia.Controls { OnClick(); } - IsPressed = false; e.Handled = true; } @@ -321,7 +320,7 @@ namespace Avalonia.Controls } } } - + protected override void OnPointerCaptureLost(PointerCaptureLostEventArgs e) { IsPressed = false; diff --git a/src/Avalonia.Controls/MenuItem.cs b/src/Avalonia.Controls/MenuItem.cs index 7d4fef009d..faf92c97fe 100644 --- a/src/Avalonia.Controls/MenuItem.cs +++ b/src/Avalonia.Controls/MenuItem.cs @@ -102,6 +102,7 @@ namespace Avalonia.Controls private ICommand? _command; private bool _commandCanExecute = true; private Popup? _popup; + private KeyGesture _hotkey; /// /// Initializes static members of the class. @@ -337,6 +338,9 @@ namespace Avalonia.Controls protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e) { + if (_hotkey != null) + // Control attached again, set Hotkey to create a hotkey manager for this control + HotKey = _hotkey; base.OnAttachedToLogicalTree(e); if (Command != null) @@ -347,6 +351,13 @@ namespace Avalonia.Controls protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e) { + if (HotKey != null) + { + // This will cause the hotkey manager to dispose the observer and the reference to this control + _hotkey = HotKey; + HotKey = null; + } + base.OnDetachedFromLogicalTree(e); if (Command != null) From 68f67f703401c50105ed63250266f44ec7f190a7 Mon Sep 17 00:00:00 2001 From: Adir Date: Thu, 8 Oct 2020 18:05:46 +0300 Subject: [PATCH 6/9] Reverted auto indentation in HotkeyManager.cs --- src/Avalonia.Controls/HotkeyManager.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Avalonia.Controls/HotkeyManager.cs b/src/Avalonia.Controls/HotkeyManager.cs index 5972a0fdd9..95752e7875 100644 --- a/src/Avalonia.Controls/HotkeyManager.cs +++ b/src/Avalonia.Controls/HotkeyManager.cs @@ -84,7 +84,7 @@ namespace Avalonia.Controls { if (_root != null && _hotkey != null) { - _binding = new KeyBinding() { Gesture = _hotkey, Command = _wrapper }; + _binding = new KeyBinding() {Gesture = _hotkey, Command = _wrapper}; _root.KeyBindings.Add(_binding); } } @@ -102,12 +102,11 @@ namespace Avalonia.Controls HotKeyProperty.Changed.Subscribe(args => { var control = args.Sender as IControl; - if (args.OldValue != null || control == null) + if (args.OldValue != null|| control == null) return; new Manager(control).Init(); }); } - public static void SetHotKey(AvaloniaObject target, KeyGesture value) => target.SetValue(HotKeyProperty, value); public static KeyGesture GetHotKey(AvaloniaObject target) => target.GetValue(HotKeyProperty); } From fd2da7aa49b2ba59289d205e35b39cff177b43db Mon Sep 17 00:00:00 2001 From: Adir Date: Thu, 8 Oct 2020 21:26:21 +0300 Subject: [PATCH 7/9] Added unit tests for enforcing manager releasing reference to the controls Changed AncestorFinder.cs to dispose child nodes and subject (need review on this) --- src/Avalonia.Controls/Utils/AncestorFinder.cs | 2 + .../Utils/HotKeyManagerTests.cs | 113 +++++++++++++++++- 2 files changed, 114 insertions(+), 1 deletion(-) diff --git a/src/Avalonia.Controls/Utils/AncestorFinder.cs b/src/Avalonia.Controls/Utils/AncestorFinder.cs index 529aec1393..42e0d85dcd 100644 --- a/src/Avalonia.Controls/Utils/AncestorFinder.cs +++ b/src/Avalonia.Controls/Utils/AncestorFinder.cs @@ -47,6 +47,8 @@ namespace Avalonia.Controls.Utils public void Dispose() { + _child?.Dispose(); + _subject.Dispose(); _disposable.Dispose(); } } diff --git a/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs b/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs index 9f2712c93c..6425c5c8f4 100644 --- a/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs @@ -1,9 +1,14 @@ -using Moq; +using System; +using System.Collections.Generic; +using Avalonia.Collections; using Avalonia.Controls.Presenters; using Avalonia.Controls.Templates; +using Avalonia.Data; using Avalonia.Input; using Avalonia.Platform; using Avalonia.Styling; +using Avalonia.UnitTests; +using Moq; using Xunit; namespace Avalonia.Controls.UnitTests.Utils @@ -50,10 +55,116 @@ namespace Avalonia.Controls.UnitTests.Utils HotKeyManager.SetHotKey(button, null); Assert.Empty(tl.KeyBindings); + } + } + + [Fact] + public void HotKeyManager_Release_Reference_When_Control_Detached() + { + using (AvaloniaLocator.EnterScope()) + { + var styler = new Mock(); + + AvaloniaLocator.CurrentMutable + .Bind().ToConstant(new WindowingPlatformMock()) + .Bind().ToConstant(styler.Object); + + var gesture1 = new KeyGesture(Key.A, KeyModifiers.Control); + + WeakReference reference = null; + + var tl = new Window(); + + new Action(() => + { + var button = new Button(); + reference = new WeakReference(button, true); + tl.Content = button; + tl.Template = CreateWindowTemplate(); + tl.ApplyTemplate(); + tl.Presenter.ApplyTemplate(); + HotKeyManager.SetHotKey(button, gesture1); + + // Detach the button from the logical tree, so there is no reference to it + tl.Content = null; + tl.ApplyTemplate(); + })(); + + // The button should be collected since it's detached from the listbox + GC.Collect(); + GC.WaitForPendingFinalizers(); + + Assert.Null(reference?.Target); + } + } + + [Fact] + public void HotKeyManager_Release_Reference_When_Control_In_Item_Template_Detached() + { + using (UnitTestApplication.Start(TestServices.StyledWindow)) + { + var styler = new Mock(); + + AvaloniaLocator.CurrentMutable + .Bind().ToConstant(new WindowingPlatformMock()) + .Bind().ToConstant(styler.Object); + + var gesture1 = new KeyGesture(Key.A, KeyModifiers.Control); + + var weakReferences = new List(); + var tl = new Window { SizeToContent = SizeToContent.WidthAndHeight, IsVisible = true }; + var lm = tl.LayoutManager; + + var keyGestures = new AvaloniaList { gesture1 }; + var listBox = new ListBox + { + Width = 100, + Height = 100, + VirtualizationMode = ItemVirtualizationMode.None, + // Create a button with binding to the KeyGesture in the template and add it to references list + ItemTemplate = new FuncDataTemplate(typeof(KeyGesture), (o, scope) => + { + var keyGesture = o as KeyGesture; + var button = new Button + { + DataContext = keyGesture, [!Button.HotKeyProperty] = new Binding("") + }; + weakReferences.Add(new WeakReference(button, true)); + return button; + }) + }; + // Add the listbox and render it + tl.Content = listBox; + lm.ExecuteInitialLayoutPass(); + listBox.Items = keyGestures; + lm.ExecuteLayoutPass(); + + // Let the button detach when clearing the source items + keyGestures.Clear(); + lm.ExecuteLayoutPass(); + + // Add it again to double check,and render + keyGestures.Add(gesture1); + lm.ExecuteLayoutPass(); + + keyGestures.Clear(); + lm.ExecuteLayoutPass(); + + // The button should be collected since it's detached from the listbox + GC.Collect(); + GC.WaitForPendingFinalizers(); + + + Assert.True(weakReferences.Count > 0); + foreach (var weakReference in weakReferences) + { + Assert.Null(weakReference.Target); + } } } + private FuncControlTemplate CreateWindowTemplate() { return new FuncControlTemplate((parent, scope) => From d58d80d11b8c8e7d038899e99c62e75da59fbc2c Mon Sep 17 00:00:00 2001 From: Adir Date: Fri, 9 Oct 2020 01:34:14 +0300 Subject: [PATCH 8/9] Added another GC collect to the HotKeyManagerTests.cs memory leak tests Renamed tests --- .../Utils/HotKeyManagerTests.cs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs b/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs index 6425c5c8f4..2e23de366e 100644 --- a/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Utils/HotKeyManagerTests.cs @@ -59,7 +59,7 @@ namespace Avalonia.Controls.UnitTests.Utils } [Fact] - public void HotKeyManager_Release_Reference_When_Control_Detached() + public void HotKeyManager_Should_Release_Reference_When_Control_Detached() { using (AvaloniaLocator.EnterScope()) { @@ -94,13 +94,15 @@ namespace Avalonia.Controls.UnitTests.Utils // The button should be collected since it's detached from the listbox GC.Collect(); GC.WaitForPendingFinalizers(); + GC.Collect(); + GC.WaitForPendingFinalizers(); Assert.Null(reference?.Target); } } [Fact] - public void HotKeyManager_Release_Reference_When_Control_In_Item_Template_Detached() + public void HotKeyManager_Should_Release_Reference_When_Control_In_Item_Template_Detached() { using (UnitTestApplication.Start(TestServices.StyledWindow)) { @@ -154,7 +156,8 @@ namespace Avalonia.Controls.UnitTests.Utils // The button should be collected since it's detached from the listbox GC.Collect(); GC.WaitForPendingFinalizers(); - + GC.Collect(); + GC.WaitForPendingFinalizers(); Assert.True(weakReferences.Count > 0); foreach (var weakReference in weakReferences) From 41c4630bbb2fac9e9247f864d510079a9e60327b Mon Sep 17 00:00:00 2001 From: Adir Date: Fri, 23 Oct 2020 14:08:05 +0300 Subject: [PATCH 9/9] Changed if statements to have braces Moved comments above if --- src/Avalonia.Controls/Button.cs | 8 +++++--- src/Avalonia.Controls/MenuItem.cs | 8 +++++--- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/Avalonia.Controls/Button.cs b/src/Avalonia.Controls/Button.cs index 87fabf5ed7..9cd7a19e7e 100644 --- a/src/Avalonia.Controls/Button.cs +++ b/src/Avalonia.Controls/Button.cs @@ -208,9 +208,11 @@ namespace Avalonia.Controls protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e) { - if (_hotkey != null) - // Control attached again, set Hotkey to create a hotkey manager for this control + if (_hotkey != null) // Control attached again, set Hotkey to create a hotkey manager for this control + { HotKey = _hotkey; + } + base.OnAttachedToLogicalTree(e); if (Command != null) @@ -221,9 +223,9 @@ namespace Avalonia.Controls protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e) { + // This will cause the hotkey manager to dispose the observer and the reference to this control if (HotKey != null) { - // This will cause the hotkey manager to dispose the observer and the reference to this control _hotkey = HotKey; HotKey = null; } diff --git a/src/Avalonia.Controls/MenuItem.cs b/src/Avalonia.Controls/MenuItem.cs index faf92c97fe..8612f696bc 100644 --- a/src/Avalonia.Controls/MenuItem.cs +++ b/src/Avalonia.Controls/MenuItem.cs @@ -338,9 +338,11 @@ namespace Avalonia.Controls protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e) { - if (_hotkey != null) - // Control attached again, set Hotkey to create a hotkey manager for this control + if (_hotkey != null) // Control attached again, set Hotkey to create a hotkey manager for this control + { HotKey = _hotkey; + } + base.OnAttachedToLogicalTree(e); if (Command != null) @@ -351,9 +353,9 @@ namespace Avalonia.Controls protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e) { + // This will cause the hotkey manager to dispose the observer and the reference to this control if (HotKey != null) { - // This will cause the hotkey manager to dispose the observer and the reference to this control _hotkey = HotKey; HotKey = null; }