Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-8045: Fix hanged node search when the user types faster than search #15733

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/DynamoCore/Search/NodeSearchModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,8 @@ internal string ProcessNodeCategory(string category, ref SearchElementGroup grou

internal IEnumerable<NodeSearchElement> Search(string search, LuceneSearchUtility luceneSearchUtility)
{

if (luceneSearchUtility != null)
if (luceneSearchUtility == null) return null;
lock (luceneSearchUtility)
Copy link
Member

@mjkkirschner mjkkirschner Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still required in addition to the search operations taking a lock out on the queue token?

{
//The DirectoryReader and IndexSearcher have to be assigned after commiting indexing changes and before executing the Searcher.Search() method, otherwise new indexed info won't be reflected
luceneSearchUtility.dirReader = luceneSearchUtility.writer != null ? luceneSearchUtility.writer.GetReader(applyAllDeletes: true) : DirectoryReader.Open(luceneSearchUtility.indexDir);
Expand Down Expand Up @@ -277,7 +277,6 @@ internal IEnumerable<NodeSearchElement> Search(string search, LuceneSearchUtilit
}
return candidates;
}
return null;
}

internal NodeSearchElement FindModelForNodeNameAndCategory(string nodeName, string nodeCategory, string parameters)
Expand Down
40 changes: 27 additions & 13 deletions src/DynamoCoreWpf/Controls/IncanvasSearchControl.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,18 @@ private void OnMouseLeftButtonUp(object sender, MouseButtonEventArgs e)
private void ExecuteSearchElement(ListBoxItem listBoxItem)
{
var searchElement = listBoxItem.DataContext as NodeSearchElementViewModel;
if (searchElement != null)
{
searchElement.Position = ViewModel.InCanvasSearchPosition;
searchElement.ClickedCommand?.Execute(null);
Analytics.TrackEvent(
Dynamo.Logging.Actions.Select,
Dynamo.Logging.Categories.InCanvasSearchOperations,
searchElement.FullName);
}
ExecuteSearchElement(searchElement);
}

private void ExecuteSearchElement(NodeSearchElementViewModel searchElement)
{
if (searchElement == null) return;
searchElement.Position = ViewModel.InCanvasSearchPosition;
searchElement.ClickedCommand?.Execute(null);
Analytics.TrackEvent(
Dynamo.Logging.Actions.Select,
Dynamo.Logging.Categories.InCanvasSearchOperations,
searchElement.FullName);
}

private void OnMouseEnter(object sender, MouseEventArgs e)
Expand Down Expand Up @@ -187,11 +190,22 @@ private void OnInCanvasSearchKeyDown(object sender, KeyEventArgs e)
OnRequestShowInCanvasSearch(ShowHideFlags.Hide);
break;
case Key.Enter:
if (HighlightedItem != null && ViewModel.CurrentMode != SearchViewModel.ViewMode.LibraryView)
ViewModel.AfterLastPendingSearch(() =>
{
ExecuteSearchElement(HighlightedItem);
OnRequestShowInCanvasSearch(ShowHideFlags.Hide);
}
Dispatcher.BeginInvoke(() =>
{
var searchElement = HighlightedItem?.DataContext as NodeSearchElementViewModel;

//if dropdown hasn't yet fully loaded lets assume the user wants the first element
searchElement ??= ViewModel.FilteredResults.FirstOrDefault();

if (searchElement != null && ViewModel.CurrentMode != SearchViewModel.ViewMode.LibraryView)
{
ExecuteSearchElement(searchElement);
OnRequestShowInCanvasSearch(ShowHideFlags.Hide);
}
});
});
break;
case Key.Up:
index = MoveToNextMember(false, members, highlightedMember);
Expand Down
4 changes: 3 additions & 1 deletion src/DynamoCoreWpf/DynamoCoreWpf.csproj
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<UILib>true</UILib>
</PropertyGroup>
Expand Down Expand Up @@ -331,6 +331,7 @@
<Compile Include="Utilities\CrashReportTool.cs" />
<Compile Include="Utilities\CrashUtilities.cs" />
<Compile Include="Utilities\GroupStyleItemSelector.cs" />
<Compile Include="Utilities\JobDebouncer.cs" />
<Compile Include="Utilities\LibraryDragAndDrop.cs" />
<Compile Include="Utilities\MessageBoxUtilities.cs" />
<Compile Include="Utilities\NodeContextMenuBuilder.cs" />
Expand Down Expand Up @@ -405,6 +406,7 @@
</Compile>
<Compile Include="UI\SharedResourceDictionary.cs" />
<Compile Include="Utilities\PreferencesPanelUtilities.cs" />
<Compile Include="Utilities\SerialQueue.cs" />
<Compile Include="Utilities\WebView2Utilities.cs" />
<Compile Include="ViewModels\Core\AnnotationExtension.cs" />
<Compile Include="ViewModels\Core\AnnotationViewModel.cs" />
Expand Down
12 changes: 12 additions & 0 deletions src/DynamoCoreWpf/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2867,6 +2867,7 @@ Dynamo.ViewModels.RequestBitmapSourceHandler
Dynamo.ViewModels.RequestOpenDocumentationLinkHandler
Dynamo.ViewModels.RequestPackagePublishDialogHandler
Dynamo.ViewModels.SearchViewModel
Dynamo.ViewModels.SearchViewModel.AfterLastPendingSearch(System.Action action) -> void
Dynamo.ViewModels.SearchViewModel.BrowserRootCategories.get -> System.Collections.ObjectModel.ObservableCollection<Dynamo.Wpf.ViewModels.NodeCategoryViewModel>
Dynamo.ViewModels.SearchViewModel.BrowserVisibility.get -> bool
Dynamo.ViewModels.SearchViewModel.BrowserVisibility.set -> void
Expand Down Expand Up @@ -3671,10 +3672,19 @@ Dynamo.Wpf.Utilities.DynamoWebView2
Dynamo.Wpf.Utilities.DynamoWebView2.DynamoWebView2() -> void
Dynamo.Wpf.Utilities.GroupStyleItemSelector
Dynamo.Wpf.Utilities.GroupStyleItemSelector.GroupStyleItemSelector() -> void
Dynamo.Wpf.Utilities.JobDebouncer
Dynamo.Wpf.Utilities.JobDebouncer.DebounceQueueToken
Dynamo.Wpf.Utilities.JobDebouncer.DebounceQueueToken.DebounceQueueToken() -> void
Dynamo.Wpf.Utilities.JobDebouncer.DebounceQueueToken.LastExecutionId -> long
Dynamo.Wpf.Utilities.JobDebouncer.DebounceQueueToken.SerialQueue -> Dynamo.Wpf.Utilities.SerialQueue
Dynamo.Wpf.Utilities.LibraryDragAndDrop
Dynamo.Wpf.Utilities.LibraryDragAndDrop.LibraryDragAndDrop() -> void
Dynamo.Wpf.Utilities.MessageBoxService
Dynamo.Wpf.Utilities.MessageBoxService.MessageBoxService() -> void
Dynamo.Wpf.Utilities.SerialQueue
Dynamo.Wpf.Utilities.SerialQueue.DispatchAsync(System.Action action) -> void
Dynamo.Wpf.Utilities.SerialQueue.SerialQueue() -> void
Dynamo.Wpf.Utilities.SerialQueue.UnhandledException -> System.Action<System.Action, System.Exception>
Dynamo.Wpf.Utilities.WebView2Utilities
Dynamo.Wpf.VariableInputNodeViewCustomization
Dynamo.Wpf.VariableInputNodeViewCustomization.Dispose() -> void
Expand Down Expand Up @@ -5614,6 +5624,8 @@ static Dynamo.Wpf.UI.VisualConfigurations.ErrorTextFontWeight -> System.Windows.
static Dynamo.Wpf.UI.VisualConfigurations.LibraryTooltipTextFontWeight -> System.Windows.FontWeight
static Dynamo.Wpf.UI.VisualConfigurations.NodeTooltipTextFontWeight -> System.Windows.FontWeight
static Dynamo.Wpf.Utilities.CompactBubbleHandler.Process(ProtoCore.Mirror.MirrorData value) -> Dynamo.ViewModels.CompactBubbleViewModel
static Dynamo.Wpf.Utilities.JobDebouncer.EnqueueMandatoryJobAsync(System.Action job, Dynamo.Wpf.Utilities.JobDebouncer.DebounceQueueToken token) -> void
static Dynamo.Wpf.Utilities.JobDebouncer.EnqueueOptionalJobAsync(System.Action job, Dynamo.Wpf.Utilities.JobDebouncer.DebounceQueueToken token) -> void
static Dynamo.Wpf.Utilities.MessageBoxService.Show(string msg, string title, bool showRichTextBox, System.Windows.MessageBoxButton button, System.Windows.MessageBoxImage img) -> System.Windows.MessageBoxResult
static Dynamo.Wpf.Utilities.MessageBoxService.Show(string msg, string title, System.Windows.MessageBoxButton button, System.Collections.Generic.IEnumerable<string> buttonNames, System.Windows.MessageBoxImage img) -> System.Windows.MessageBoxResult
static Dynamo.Wpf.Utilities.MessageBoxService.Show(string msg, string title, System.Windows.MessageBoxButton button, System.Windows.MessageBoxImage img) -> System.Windows.MessageBoxResult
Expand Down
43 changes: 43 additions & 0 deletions src/DynamoCoreWpf/Utilities/JobDebouncer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
using System;

namespace Dynamo.Wpf.Utilities
{
public static class JobDebouncer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • can you add some more description to this utility, as to when this should be used? I understand we all know that answer already, but would like to document so everyone and new developers on team also learn to use it.

  • There are also a few other instances using deboucer in our code, can you unify the usage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not make this public.

{
public class DebounceQueueToken
{
public long LastExecutionId = 0;
public SerialQueue SerialQueue = new();
};
/// <summary>
/// Action <paramref name="job"/> is guaranteed to run at most once for every call, and exactly once after the last call.
/// Execution is sequential, and optional jobs that share a <see cref="DebounceQueueToken"/> with a newer optional job will be ignored.
/// </summary>
/// <param name="job"></param>
/// <param name="token"></param>
/// <returns></returns>
public static void EnqueueOptionalJobAsync(Action job, DebounceQueueToken token)
{
lock (token)
{
token.LastExecutionId++;
var myExecutionId = token.LastExecutionId;
token.SerialQueue.DispatchAsync(() =>
{
if (myExecutionId < token.LastExecutionId) return;
job();
});
}
}
public static void EnqueueMandatoryJobAsync(Action job, DebounceQueueToken token)
{
lock (token)
{
token.SerialQueue.DispatchAsync(() =>
{
job();
});
}
}
}
}
80 changes: 80 additions & 0 deletions src/DynamoCoreWpf/Utilities/SerialQueue.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
//https://github.com/gentlee/SerialQueue/blob/master/SerialQueue/SerialQueue.cs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@QilongTang do we need to update the about box for this?

using System;
using System.Threading;

namespace Dynamo.Wpf.Utilities
{
public class SerialQueue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here - internal unless theres a good reason.

{
class LinkedListNode(Action action)
{
public readonly Action Action = action;
public LinkedListNode Next;
}

public event Action<Action, Exception> UnhandledException = delegate { };

private LinkedListNode _queueFirst;
private LinkedListNode _queueLast;
private bool _isRunning = false;

public void DispatchAsync(Action action)
{
var newNode = new LinkedListNode(action);

lock (this)
{
if (_queueFirst == null)
{
_queueFirst = newNode;
_queueLast = newNode;

if (!_isRunning)
{
_isRunning = true;
ThreadPool.QueueUserWorkItem(Run);
}
}
else
{
_queueLast!.Next = newNode;
_queueLast = newNode;
}
}
}

private void Run(object _)
{
while (true)
{
LinkedListNode firstNode;

lock (this)
{
if (_queueFirst == null)
{
_isRunning = false;
return;
}
firstNode = _queueFirst;
_queueFirst = null;
_queueLast = null;
}

while (firstNode != null)
{
var action = firstNode.Action;
firstNode = firstNode.Next;
try
{
action();
}
catch (Exception error)
{
UnhandledException.Invoke(action, error);
}
}
}
}
}
}
29 changes: 20 additions & 9 deletions src/DynamoCoreWpf/ViewModels/Search/SearchViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Text.RegularExpressions;
using System.Windows;
using System.Windows.Media;
using System.Windows.Threading;
using Dynamo.Configuration;
using Dynamo.Engine;
using Dynamo.Graph.Nodes;
Expand All @@ -18,6 +19,7 @@
using Dynamo.UI;
using Dynamo.Utilities;
using Dynamo.Wpf.Services;
using Dynamo.Wpf.Utilities;
using Dynamo.Wpf.ViewModels;

namespace Dynamo.ViewModels
Expand Down Expand Up @@ -861,14 +863,10 @@ private static string MakeFullyQualifiedName(string path, string addition)
/// </summary>
internal void SearchAndUpdateResults()
{
searchResults.Clear();

if (!String.IsNullOrEmpty(SearchText.Trim()))
{
SearchAndUpdateResults(SearchText);
}

RaisePropertyChanged("IsAnySearchResult");
}

/// <summary>
Expand All @@ -886,13 +884,20 @@ public void SearchAndUpdateResults(string query)

//Passing the second parameter as true will search using Lucene.NET
var foundNodes = Search(query);
searchResults = new List<NodeSearchElementViewModel>(foundNodes);

FilteredResults = searchResults;
//Unit tests don't have an Application.Current
(Application.Current?.Dispatcher ?? Dispatcher.CurrentDispatcher).Invoke(() =>
{
searchResults = new List<NodeSearchElementViewModel>(foundNodes);

FilteredResults = searchResults;

UpdateSearchCategories();

UpdateSearchCategories();
RaisePropertyChanged("FilteredResults");

RaisePropertyChanged("FilteredResults");
RaisePropertyChanged("IsAnySearchResult");
});
}

/// <summary>
Expand Down Expand Up @@ -1164,9 +1169,15 @@ public void OnSearchElementClicked(NodeModel nodeModel, Point position)

#region Commands

private static readonly JobDebouncer.DebounceQueueToken DebounceQueueToken = new();
public void Search(object parameter)
{
SearchAndUpdateResults();
JobDebouncer.EnqueueOptionalJobAsync(SearchAndUpdateResults, DebounceQueueToken);
}

public void AfterLastPendingSearch(Action action)
{
JobDebouncer.EnqueueMandatoryJobAsync(action, DebounceQueueToken);
}

internal bool CanSearch(object parameter)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public SearchResultDataProvider(NodeSearchModel model, IconResourceProvider icon
public override Stream GetResource(string searchText, out string extension)
{
var text = Uri.UnescapeDataString(searchText);
var elements = model.Search(text, LuceneSearch.LuceneUtilityNodeSearch);
var elements = string.IsNullOrWhiteSpace(text) ? new List<NodeSearchElement>() : model.Search(text, LuceneSearch.LuceneUtilityNodeSearch);
extension = "json";
return GetNodeItemDataStream(elements, true);
}
Expand Down
9 changes: 8 additions & 1 deletion src/LibraryViewExtensionWebView2/LibraryViewController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -461,10 +461,17 @@ private void Browser_KeyDown(object sender, KeyEventArgs e)

var synteticEventData = new Dictionary<string, string>
{
[Enum.GetName(typeof(ModifiersJS), e.KeyboardDevice.Modifiers)] = "true",
["key"] = e.Key.ToString()
};

foreach(ModifiersJS modifier in Enum.GetValues(typeof(ModifiersJS)))
{
if (((int)e.KeyboardDevice.Modifiers & (int)modifier) != 0)
{
synteticEventData[Enum.GetName(typeof(ModifiersJS), modifier)] = "true";
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to the debouncing logic right? Would you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Enum.GetName(typeof(ModifiersJS), e.KeyboardDevice.Modifiers)] = "true" throws exception for key combinations with more than one modifier (ex: ctrl+shift+a), its an issue with an easy fix i found along the way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, a good improvement for sure 👍🏻


_ = ExecuteScriptFunctionAsync(browser, "eventDispatcher", synteticEventData);
}

Expand Down
Loading
Loading