From 7334501bf4ccbcea53180fedc0091cb4bd0066ce Mon Sep 17 00:00:00 2001 From: bendikberg Date: Tue, 21 Jan 2025 11:59:32 +0100 Subject: [PATCH] Search bar stuttering improvements (#15726) Co-authored-by: pinzart90 --- src/DynamoCoreWpf/DynamoCoreWpf.csproj | 1 + .../Utilities/ActionDebouncer.cs | 61 ++++++ .../ViewModels/Search/SearchViewModel.cs | 44 +++- .../DynamoFeatureFlags/FeatureFlagsClient.cs | 4 +- test/DynamoCoreWpfTests/CoreUITests.cs | 194 +++++++++++++++++- 5 files changed, 300 insertions(+), 4 deletions(-) create mode 100644 src/DynamoCoreWpf/Utilities/ActionDebouncer.cs diff --git a/src/DynamoCoreWpf/DynamoCoreWpf.csproj b/src/DynamoCoreWpf/DynamoCoreWpf.csproj index 5836dc0cac1..483b81af869 100644 --- a/src/DynamoCoreWpf/DynamoCoreWpf.csproj +++ b/src/DynamoCoreWpf/DynamoCoreWpf.csproj @@ -432,6 +432,7 @@ + diff --git a/src/DynamoCoreWpf/Utilities/ActionDebouncer.cs b/src/DynamoCoreWpf/Utilities/ActionDebouncer.cs new file mode 100644 index 00000000000..c1c89a3e2f6 --- /dev/null +++ b/src/DynamoCoreWpf/Utilities/ActionDebouncer.cs @@ -0,0 +1,61 @@ +using Dynamo.Logging; +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Dynamo.Wpf.Utilities +{ + /// + /// The ActionDebouncer class offers a means to reduce the number of UI notifications for a specified time. + /// It is meant to be used in UI elements where too many UI updates can cause perfomance issues. + /// + internal class ActionDebouncer(ILogger logger) : IDisposable + { + private readonly ILogger logger = logger; + private CancellationTokenSource cts; + + public void Cancel() + { + if (cts != null) + { + cts.Cancel(); + cts.Dispose(); + cts = null; + } + } + + /// + /// Delays the "action" for a "timeout" number of milliseconds + /// The input Action will run on same syncronization context as the Debounce method call. + /// + /// Number of milliseconds to wait + /// The action to execute after the timeout runs out. + /// A task that finishes when the deboucing is cancelled or the input action has completed (successfully or not). Should be discarded in most scenarios. + public void Debounce(int timeout, Action action) + { + Cancel(); + cts = new CancellationTokenSource(); + + Task.Delay(timeout, cts.Token).ContinueWith((t) => + { + try + { + if (t.Status == TaskStatus.RanToCompletion) + { + action(); + } + } + catch (Exception ex) + { + logger?.Log("Failed to run debounce action with the following error:"); + logger?.Log(ex.ToString()); + } + }, TaskScheduler.FromCurrentSynchronizationContext()); + } + + public void Dispose() + { + Cancel(); + } + } +} diff --git a/src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs b/src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs index 4526b1f3afc..ffe005440ec 100644 --- a/src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs +++ b/src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs @@ -18,7 +18,9 @@ using Dynamo.UI; using Dynamo.Utilities; using Dynamo.Wpf.Services; +using Dynamo.Wpf.Utilities; using Dynamo.Wpf.ViewModels; +using DynamoUtilities; namespace Dynamo.ViewModels { @@ -73,7 +75,11 @@ public bool BrowserVisibility set { browserVisibility = value; RaisePropertyChanged("BrowserVisibility"); } } - private string searchText; + internal int searchDelayTimeout = 150; + // Feature flags activated debouncer for the search UI. + internal ActionDebouncer searchDebouncer = null; + + private string searchText = string.Empty; /// /// SearchText property /// @@ -86,10 +92,28 @@ public string SearchText set { searchText = value; - OnSearchTextChanged(this, EventArgs.Empty); + RaisePropertyChanged("SearchText"); RaisePropertyChanged("BrowserRootCategories"); RaisePropertyChanged("CurrentMode"); + + // The searchText is set multiple times before the control becomes visible and interactable. + // To prevent any debounces from triggering at some unexpected point before or after the control + // becomes visible, this flag is only set once the searchText value is set by the user + // (unless it somehow gets set somewhere else) + // + // pinzart: The search text is set multiple times with an empty value. Seems sufficient to only use the debouncer + // if we get a non-empty value. + if (!string.IsNullOrEmpty(searchText) && searchDebouncer != null) + { + searchDebouncer.Debounce(searchDelayTimeout, () => OnSearchTextChanged(this, EventArgs.Empty)); + } + else + { + // Make sure any previously scheduled debounces are cancelled + searchDebouncer?.Cancel(); + OnSearchTextChanged(this, EventArgs.Empty); + } } } @@ -374,9 +398,19 @@ internal SearchViewModel(DynamoViewModel dynamoViewModel) iconServices = new IconServices(pathManager); + DynamoFeatureFlagsManager.FlagsRetrieved += TryInitializeDebouncer; + InitializeCore(); } + private void TryInitializeDebouncer() + { + if (DynamoModel.FeatureFlags?.CheckFeatureFlag("searchbar_debounce", false) ?? false) + { + searchDebouncer ??= new ActionDebouncer(dynamoViewModel?.Model?.Logger); + } + } + // Just for tests. Please refer to LibraryTests.cs internal SearchViewModel(NodeSearchModel model) { @@ -407,6 +441,9 @@ public override void Dispose() Model.EntryUpdated -= UpdateEntry; Model.EntryRemoved -= RemoveEntry; + searchDebouncer?.Dispose(); + DynamoFeatureFlagsManager.FlagsRetrieved -= TryInitializeDebouncer; + base.Dispose(); } @@ -434,6 +471,9 @@ private void InitializeCore() DefineFullCategoryNames(LibraryRootCategories, ""); InsertClassesIntoTree(LibraryRootCategories); + + // If feature flags are already cached, try to initialize the debouncer + TryInitializeDebouncer(); } private void AddEntry(NodeSearchElement entry) diff --git a/src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs b/src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs index 1d618f5fcfb..9966455d13c 100644 --- a/src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs +++ b/src/Tools/DynamoFeatureFlags/FeatureFlagsClient.cs @@ -85,7 +85,9 @@ internal FeatureFlagsClient(string userkey, string mobileKey = null, bool testMo AllFlags = LdValue.ObjectFrom(new Dictionary { { "TestFlag1",LdValue.Of(true) }, { "TestFlag2", LdValue.Of("I am a string") }, //in tests we want instancing on so we can test it. - { "graphics-primitive-instancing", LdValue.Of(true) } }); + { "graphics-primitive-instancing", LdValue.Of(true) }, + //in tests we want search debouncing on so we can test it. + { "searchbar_debounce", LdValue.Of(true) } }); return; } diff --git a/test/DynamoCoreWpfTests/CoreUITests.cs b/test/DynamoCoreWpfTests/CoreUITests.cs index 7bdfeb586a4..7ccd638edc6 100644 --- a/test/DynamoCoreWpfTests/CoreUITests.cs +++ b/test/DynamoCoreWpfTests/CoreUITests.cs @@ -26,6 +26,7 @@ using Dynamo.Utilities; using Dynamo.ViewModels; using Dynamo.Views; +using Dynamo.Wpf.Utilities; using DynamoCoreWpfTests.Utility; using Moq; using Moq.Protected; @@ -968,12 +969,203 @@ public void InCanvasSearchTextChangeTriggersOneSearchCommand() int count = 0; (searchControl.DataContext as SearchViewModel).SearchCommand = new Dynamo.UI.Commands.DelegateCommand((object _) => { count++; }); searchControl.SearchTextBox.Text = "dsfdf"; - DispatcherUtil.DoEvents(); + DispatcherUtil.DoEventsLoop(() => count == 1); Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); Assert.AreEqual(count, 1); } + [Test] + [Category("UnitTests")] + public void InCanvasSearchTextChangeTriggersOneSearchCommandWithDebounce() + { + var currentWs = View.ChildOfType(); + + // open context menu + RightClick(currentWs.zoomBorder); + + // show in-canvas search + ViewModel.CurrentSpaceViewModel.ShowInCanvasSearchCommand.Execute(ShowHideFlags.Show); + + var searchControl = currentWs.ChildrenOfType().Select(x => (x as Popup)?.Child as InCanvasSearchControl).Where(c => c != null).FirstOrDefault(); + Assert.IsNotNull(searchControl); + + DispatcherUtil.DoEvents(); + + int count = 0; + var vm = searchControl.DataContext as SearchViewModel; + Assert.IsNotNull(vm); + vm.SearchCommand = new Dynamo.UI.Commands.DelegateCommand((object _) => { count++; }); + + // run without debouncer + vm.searchDebouncer.Dispose(); + vm.searchDebouncer = null; // disable the debouncer + searchControl.SearchTextBox.Text = "dsfdf"; + DispatcherUtil.DoEventsLoop(() => count == 1); + + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(1, count, "changing the text once should cause a single update"); + + int currThreadId = Environment.CurrentManagedThreadId; + int debouncerThreadId = -1; + var debouncer = new ActionDebouncer(null); + + int dbCount = 0; + debouncer.Debounce(100, () => + { + dbCount++; + debouncerThreadId = Environment.CurrentManagedThreadId; + }); + + DispatcherUtil.DoEventsLoop(() => debouncerThreadId != -1); + Assert.AreEqual(currThreadId, debouncerThreadId); + + vm.searchDebouncer = debouncer; + searchControl.SearchTextBox.Text = "dsfdf"; + DispatcherUtil.DoEventsLoop(() => dbCount == 1); + Assert.AreEqual(1, dbCount); + + // Empty strings should not hit the debounce action + searchControl.SearchTextBox.Text = ""; + Assert.AreEqual(1, dbCount); + DispatcherUtil.DoEventsLoop(() => count == 2); + } + + [Test] + [Category("UnitTests")] + public void InCanvasSearchTextWithDebouncer() + { + var currentWs = View.ChildOfType(); + + // open context menu + RightClick(currentWs.zoomBorder); + + // show in-canvas search + ViewModel.CurrentSpaceViewModel.ShowInCanvasSearchCommand.Execute(ShowHideFlags.Show); + + var searchControl = currentWs.ChildrenOfType().Select(x => (x as Popup)?.Child as InCanvasSearchControl).Where(c => c != null).FirstOrDefault(); + Assert.IsNotNull(searchControl); + + DispatcherUtil.DoEvents(); + + int count = 0; + var vm = searchControl.DataContext as SearchViewModel; + Assert.IsNotNull(vm); + + // Check that the default debouncer is setup. + Assert.IsNotNull(vm.searchDebouncer); + + void Vm_SearchTextChanged(object sender, EventArgs e) + { + count++; + throw new Exception("Failure that should be logged"); + } + + vm.searchDelayTimeout = 50; + vm.SearchTextChanged += Vm_SearchTextChanged; + + vm.SearchText = "point"; + DispatcherUtil.DoEventsLoop(() => count == 1); + Assert.AreEqual(1, count, "Search updates were sent out"); + + + vm.SearchText = "point.by"; + DispatcherUtil.DoEventsLoop(() => count == 2); + Assert.AreEqual(2, count, "thread sees updated count"); + + vm.SearchText = "abcde"; + DispatcherUtil.DoEventsLoop(() => count == 3); + Assert.AreEqual(3, count, "thread sees updated count"); + + searchControl.SearchTextBox.Text = ""; + DispatcherUtil.DoEventsLoop(() => currentWs.InCanvasSearchBar.IsOpen); + + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(4, count, "main sees updated count"); + } + + [Test] + [Category("UnitTests")] + public void InCanvasSearchTextChangeTriggersDebouncer() + { + var currentWs = View.ChildOfType(); + + // open context menu + RightClick(currentWs.zoomBorder); + + // show in-canvas search + ViewModel.CurrentSpaceViewModel.ShowInCanvasSearchCommand.Execute(ShowHideFlags.Show); + + var searchControl = currentWs.ChildrenOfType().Select(x => (x as Popup)?.Child as InCanvasSearchControl).Where(c => c != null).FirstOrDefault(); + Assert.IsNotNull(searchControl); + + DispatcherUtil.DoEvents(); + + int count = 0; + var vm = searchControl.DataContext as SearchViewModel; + Assert.IsNotNull(vm); + vm.SearchCommand = new Dynamo.UI.Commands.DelegateCommand((object _) => { count++; }); + + // prepare debounce tests + vm.searchDelayTimeout = 50; + var sleepTime = vm.searchDelayTimeout * 2; + Assert.NotNull(vm.searchDebouncer); + + // run with debouncer + count = 0; + searchControl.SearchTextBox.Text = "dsfdfdsfdf"; + Thread.Sleep(sleepTime); + DispatcherUtil.DoEventsLoop(() => count == 1); + + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(1, count, "changing the text once should cause a single update after timeout expires"); + + // multiple updates with debouncer + count = 0; + searchControl.SearchTextBox.Text = "dsfdf"; + searchControl.SearchTextBox.Text = "dsfdfdsfdf"; + searchControl.SearchTextBox.Text = "wer"; + searchControl.SearchTextBox.Text = "dsfdf"; + DispatcherUtil.DoEventsLoop(() => count == 1); + // Do another events loop to make sure no other debouncer actions are triggered + DispatcherUtil.DoEventsLoop(null, 10); + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(1, count, "changing the text multiple times in quick succession should cause a single update once timeout expires"); + + // multiple updates with empty string debouncer + count = 0; + searchControl.SearchTextBox.Text = "dsfdf"; + searchControl.SearchTextBox.Text = ""; + searchControl.SearchTextBox.Text = "dsfdf"; + DispatcherUtil.DoEventsLoop(() => count == 2); + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(2, count, "changing the text to empty should not trigger the debouncer"); + + // test timeout expiration + count = 0; + searchControl.SearchTextBox.Text = "dsfdfdsfdf"; + Thread.Sleep(sleepTime); + searchControl.SearchTextBox.Text = "dsfdf"; + Thread.Sleep(sleepTime); + DispatcherUtil.DoEventsLoop(() => count == 2); + + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(2, count, "2 timeout expirations should cause 2 updates"); + + // run with debouncer, then without + count = 0; + searchControl.SearchTextBox.Text = "dsfdfdsfdf"; + vm.searchDebouncer.Dispose(); + vm.searchDebouncer = null; // disable debounce + searchControl.SearchTextBox.Text = "dsfdf"; + + DispatcherUtil.DoEventsLoop(() => count == 1); + DispatcherUtil.DoEventsLoop(null, 10); + + Assert.IsTrue(currentWs.InCanvasSearchBar.IsOpen); + Assert.AreEqual(1, count, "the debounced update should have been cancelled by the immediate set"); + } + [Test] public void WarningShowsWhenSavingWithLinterWarningsOrErrors() {