From 18f4e9db31cf051572ac04a7761169aa4cc94f00 Mon Sep 17 00:00:00 2001 From: Alekhya Date: Thu, 2 Jul 2020 13:48:41 -0700 Subject: [PATCH] Fix for Memory issue with context menu items (#4597) * Added the inotifyPropertyChanged to all the properties and that stops the memory for shooting up * some more inotify properties added (cherry picked from commit 26fa05d9b661dadc5ab0257d540ab838a07c43a6) * Revert "some more inotify properties added" This reverts commit 845a94c9b22f8fcaaa73cca6a9e8f511169fdaa0. * Removed unnecessary inotifypropertychanged interfaces and cleaned up the code * removed the ctrl+c from folder plugin * removed unnecessary init * Added unit test to check if PropertyChanged is called * renamed var * refactored the tests * formatting and adding comments * changed access modifier in test * Used observable collection instead of a list * clearing the observable collection instead of setting it to a new one --- src/modules/launcher/Wox.Plugin/BaseModel.cs | 2 +- src/modules/launcher/Wox.Test/Wox.Test.csproj | 1 + src/modules/launcher/Wox.Test/WoxTest.cs | 58 +++++++++++++++++++ .../Wox/ViewModel/ContextMenuItemViewModel.cs | 20 ++++++- .../launcher/Wox/ViewModel/ResultViewModel.cs | 29 ++++------ 5 files changed, 91 insertions(+), 19 deletions(-) create mode 100644 src/modules/launcher/Wox.Test/WoxTest.cs diff --git a/src/modules/launcher/Wox.Plugin/BaseModel.cs b/src/modules/launcher/Wox.Plugin/BaseModel.cs index 54c13be576..11f64b2f2e 100644 --- a/src/modules/launcher/Wox.Plugin/BaseModel.cs +++ b/src/modules/launcher/Wox.Plugin/BaseModel.cs @@ -11,7 +11,7 @@ namespace Wox.Plugin public event PropertyChangedEventHandler PropertyChanged; [NotifyPropertyChangedInvocator] - protected void OnPropertyChanged([CallerMemberName] string propertyName = null) + protected virtual void OnPropertyChanged([CallerMemberName] string propertyName = null) { PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName)); } diff --git a/src/modules/launcher/Wox.Test/Wox.Test.csproj b/src/modules/launcher/Wox.Test/Wox.Test.csproj index ae0a2120fe..779cc5b8d3 100644 --- a/src/modules/launcher/Wox.Test/Wox.Test.csproj +++ b/src/modules/launcher/Wox.Test/Wox.Test.csproj @@ -44,6 +44,7 @@ + diff --git a/src/modules/launcher/Wox.Test/WoxTest.cs b/src/modules/launcher/Wox.Test/WoxTest.cs new file mode 100644 index 0000000000..195c6eefde --- /dev/null +++ b/src/modules/launcher/Wox.Test/WoxTest.cs @@ -0,0 +1,58 @@ +using System.Windows.Input; +using NUnit.Framework; +using Wox.Plugin; +using Wox.ViewModel; +using System.Runtime.CompilerServices; + +namespace Wox.Test +{ + [TestFixture] + public class Wox + { + // A Dummy class to test that OnPropertyChanged() is called while we set the variable + public class DummyTestClass : BaseModel + { + public bool isFunctionCalled = false; + private ICommand _item; + + public ICommand Item + { + get + { + return this._item; + } + + set + { + if (value != this._item) + { + this._item = value; + OnPropertyChanged(); + } + } + } + + // Overriding the OnPropertyChanged() function to test if it is being called + protected override void OnPropertyChanged([CallerMemberName] string propertyName = null) + { + isFunctionCalled = true; + } + } + + + [Test] + public void AnyVariable_MustCallOnPropertyChanged_WhenSet() + { + // Arrange + DummyTestClass testClass = new DummyTestClass(); + + // Act + testClass.Item = new RelayCommand(null); + + // Assert + Assert.IsTrue(testClass.isFunctionCalled); + + } + + } +} diff --git a/src/modules/launcher/Wox/ViewModel/ContextMenuItemViewModel.cs b/src/modules/launcher/Wox/ViewModel/ContextMenuItemViewModel.cs index 5a7f40e727..064949f956 100644 --- a/src/modules/launcher/Wox/ViewModel/ContextMenuItemViewModel.cs +++ b/src/modules/launcher/Wox/ViewModel/ContextMenuItemViewModel.cs @@ -9,11 +9,29 @@ namespace Wox.ViewModel { public class ContextMenuItemViewModel : BaseModel { + private ICommand _command; + public string PluginName { get; set; } public string Title { get; set; } public string Glyph { get; set; } public string FontFamily { get; set; } - public ICommand Command { get; set; } + public ICommand Command { + get + { + return this._command; + } + + set + { + // ICommand does not implement the INotifyPropertyChanged interface and must call OnPropertyChanged() to prevent memory leaks + if (value != this._command) + { + this._command = value; + OnPropertyChanged(); + } + } + } + public Key AcceleratorKey { get; set; } public ModifierKeys AcceleratorModifiers { get; set; } public bool IsAcceleratorKeyEnabled { get; set; } diff --git a/src/modules/launcher/Wox/ViewModel/ResultViewModel.cs b/src/modules/launcher/Wox/ViewModel/ResultViewModel.cs index 7540f39af1..00cde7f9cc 100644 --- a/src/modules/launcher/Wox/ViewModel/ResultViewModel.cs +++ b/src/modules/launcher/Wox/ViewModel/ResultViewModel.cs @@ -22,7 +22,7 @@ namespace Wox.ViewModel Hover }; - public List ContextMenuItems { get; set; } + public ObservableCollection ContextMenuItems { get; set; } = new ObservableCollection(); public ICommand ActivateContextButtonsHoverCommand { get; set; } public ICommand ActivateContextButtonsSelectionCommand { get; set; } @@ -46,7 +46,10 @@ namespace Wox.ViewModel { Result = result; } + ContextMenuSelectedIndex = NoSelectionIndex; + LoadContextMenu(); + ActivateContextButtonsHoverCommand = new RelayCommand(ActivateContextButtonsHoverAction); ActivateContextButtonsSelectionCommand = new RelayCommand(ActivateContextButtonsSelectionAction); DeactivateContextButtonsHoverCommand = new RelayCommand(DeactivateContextButtonsHoverAction); @@ -64,11 +67,6 @@ namespace Wox.ViewModel } public void ActivateContextButtons(ActivationType activationType) { - if (ContextMenuItems == null) - { - LoadContextMenu(); - } - // Result does not contain any context menu items - we don't need to show the context menu ListView at all. if (ContextMenuItems.Count > 0) { @@ -78,14 +76,13 @@ namespace Wox.ViewModel { AreContextButtonsActive = false; } - if (activationType == ActivationType.Selection) { IsSelected = true; EnableContextMenuAcceleratorKeys(); } - else if(activationType == ActivationType.Hover) + else if (activationType == ActivationType.Hover) { IsHovered = true; } @@ -122,17 +119,17 @@ namespace Wox.ViewModel else { AreContextButtonsActive = false; - } + } } public void LoadContextMenu() { var results = PluginManager.GetContextMenusForPlugin(Result); - var newItems = new List(); + ContextMenuItems.Clear(); foreach (var r in results) { - newItems.Add(new ContextMenuItemViewModel + ContextMenuItems.Add(new ContextMenuItemViewModel() { PluginName = r.PluginName, Title = r.Title, @@ -155,13 +152,11 @@ namespace Wox.ViewModel }) }); } - - ContextMenuItems = newItems; } private void EnableContextMenuAcceleratorKeys() { - foreach(var i in ContextMenuItems) + foreach (var i in ContextMenuItems) { i.IsAcceleratorKeyEnabled = true; } @@ -192,7 +187,7 @@ namespace Wox.ViewModel imagePath = ImageLoader.ErrorIconPath; } } - + // will get here either when icoPath has value\icon delegate is null\when had exception in delegate return ImageLoader.Load(imagePath); } @@ -201,10 +196,10 @@ namespace Wox.ViewModel //Returns false if we've already reached the last item. public bool SelectNextContextButton() { - if(ContextMenuSelectedIndex == (ContextMenuItems.Count -1)) + if (ContextMenuSelectedIndex == (ContextMenuItems.Count - 1)) { ContextMenuSelectedIndex = NoSelectionIndex; - return false; + return false; } ContextMenuSelectedIndex++;