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

[chubakueno]

Purpose

Currently the node search hangs when the user types faster than the time it takes Lucene to provide results because it is searching in the main UI thread. This PR provides a debouncing algorithm and performs searches sequentially outside of the main UI thread for a smoother search experience. Currently there is no support for cancelling previous search tasks in lucenenet so this is a fix around that limitation

Before fix:

After fix:

Declarations

Check these if you believe they are true

• The codebase is in a better state after this PR
• Is documented according to the standards
• The level of testing this PR includes is appropriate
• User facing strings, if any, are extracted into *.resx files
• All tests pass using the self-service CI.
• Snapshot of UI changes, if any.
• Changes to the API follow Semantic Versioning and are documented in the API Changes document.
• This PR modifies some build requirements and the readme is updated
• This PR contains no files larger than 50 MB

Release Notes

Prevent node search from hanging during fast typing.

Reviewers

@RobertGlobant20
@QilongTang
@sm6srw

FYIs

@avidit

[github-actions]

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8045

[sm6srw]

LGTM. But I would like to know what @QilongTang thinks about it.

[chubakueno]

Failed on test DocsAreLoadedFromHostPathBeforeCorePath, but that test never goes through Search or SearchAndUpdateResults, so failure is unrelated.

[mjkkirschner]

@QilongTang this seems to overlap with #15726 am I getting that right? How do you want to proceed with these two prs?

[mjkkirschner]

@QilongTang this seems to overlap with #15726 am I getting that right? How do you want to proceed with these two prs?

@QilongTang after taking a look at this I think I prefer the approach in #15726 - it's behind a feature flag, has tests, avoids locks, and I think is more idiomatic.

I don't know if it solves the issue of the hanging search though - @bendikberg can you verify if it fixes this issue?

[chubakueno]

Improvements in last commit:

• Implemented performance improviments in node library search (side panel)
• Fix library search in case the user uses a combination of two or more modifiers (alt+shift,ctrl+shift,etc)
• Fix library search when user types an empty space
• In context menu is now immediately confirmable on enter. This will help eliminate artificial hardcoded delays in UI Automation testing.

@QilongTang this seems to overlap with #15726 am I getting that right? How do you want to proceed with these two prs?

@QilongTang after taking a look at this I think I prefer the approach in #15726 - it's behind a feature flag, has tests, avoids locks, and I think is more idiomatic.

I don't know if it solves the issue of the hanging search though - @bendikberg can you verify if it fixes this issue?

[github-actions]

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

[chubakueno]

Added unit tests for debouncing.

[bendikberg]

@mjkkirschner
The solution I proposed debounces the keypresses so that the search is triggered at a later time, but the searches still run on the UI thread. I have still experienced it blocking the UI while it waits for a search to finish, though not for multiple seconds like the current implementation.

Since @chubakueno 's solution doesn't rely on timeouts and the searches run asynchronously it probably feels even more responsive than mine, which I think is great.

Maybe there is a way to combine the approaches?

[QilongTang]

• 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?

[chubakueno]

Added a class summary description. The PR as currently is already unifies both library (sidebar) and incontext node search into using the debouncer, but I can't find more instances of a debouncer being used in c# code (there are some in html/js webview code though).

[QilongTang]

Overall, looks solid. The things worries me most are that we have a couple instances in Dynamo leveraging debouncer differently in Dynamo (mostly around search), it would be good to unify the usage. Off course, the change in this PR to have a utility class for deboucer is a great start!

[QilongTang]

@QilongTang this seems to overlap with #15726 am I getting that right? How do you want to proceed with these two prs?

Yeah I noticed, do we want to test both in build and pick or combine the approach somehow? I do like the fact the debouncer becomes utility in this PR

[mjkkirschner]

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

[chubakueno]

The lock is in case Search is used in parallel. As of now that could only happen if the user searches in the library and workspace simultaneously, since both have independent serial queues. We could mandate a unified search queue (for library, inCanvasSearch, and in the future) or have this lock to allow several search queues in parallel.

[mjkkirschner]

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

[mjkkirschner]

@QilongTang this seems to overlap with #15726 am I getting that right? How do you want to proceed with these two prs?

Yeah I noticed, do we want to test both in build and pick or combine the approach somehow? I do like the fact the debouncer becomes utility in this PR

yeah, I'm not sure.

I still think that anything that introduces locks, or await etc should be behind a feature flag because Dynamo is not thread safe makes all sorts of assumptions, and runs in different threading contexts (Revit, sandbox etc) and I would want a way to turn this (or the other PR) off in released code. That might be overkill but it's not that hard to do in the other PR, it might be harder here.

A single denouncer utility is useful, I definitely appreciate having less ways of doing things, not more.

The serial queue code seems small enough to take on, but I'd be curious about built in types that could do the same.

[chubakueno]

@QilongTang this seems to overlap with #15726 am I getting that right? How do you want to proceed with these two prs?

Yeah I noticed, do we want to test both in build and pick or combine the approach somehow? I do like the fact the debouncer becomes utility in this PR

yeah, I'm not sure.

I still think that anything that introduces locks, or await etc should be behind a feature flag because Dynamo is not thread safe makes all sorts of assumptions, and runs in different threading contexts (Revit, sandbox etc) and I would want a way to turn this (or the other PR) off in released code. That might be overkill but it's not that hard to do in the other PR, it might be harder here.

A single denouncer utility is useful, I definitely appreciate having less ways of doing things, not more.

The serial queue code seems small enough to take on, but I'd be curious about built in types that could do the same.

Closest built in type that i have evaluated is ActionBlock in System.Threading.Tasks.Dataflow, but it needs an standing thread to Post() jobs to and overall didn't find it a good fit, though I'm open to other options.

[RobertGlobant20]

@mjkkirschner The solution I proposed debounces the keypresses so that the search is triggered at a later time, but the searches still run on the UI thread. I have still experienced it blocking the UI while it waits for a search to finish, though not for multiple seconds like the current implementation.

Since @chubakueno 's solution doesn't rely on timeouts and the searches run asynchronously it probably feels even more responsive than mine, which I think is great.

Maybe there is a way to combine the approaches?

@chubakueno based in this comment, if the search is being executed asynchronously, is there a guarantee that the search results are always from the last character typed (I mean all the characters that you already typed )?

[chubakueno]

@mjkkirschner The solution I proposed debounces the keypresses so that the search is triggered at a later time, but the searches still run on the UI thread. I have still experienced it blocking the UI while it waits for a search to finish, though not for multiple seconds like the current implementation.
Since @chubakueno 's solution doesn't rely on timeouts and the searches run asynchronously it probably feels even more responsive than mine, which I think is great.
Maybe there is a way to combine the approaches?

@chubakueno based in this comment, if the search is being executed asynchronously, is there a guarantee that the search results are always from the last character typed (I mean all the characters that you already typed )?

Yes the last search is always from the last character typed. It may have to wait for a single pending search to execute (without blocking the UI thread), but last character typed is always last search.