-
Notifications
You must be signed in to change notification settings - Fork 634
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
Introduce NodeAutoComplete Feature #11120
Conversation
…o connection mode
src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs
Outdated
Show resolved
Hide resolved
src/DynamoCoreWpf/Controls/NodeAutoCompleteSearchControl.xaml.cs
Outdated
Show resolved
Hide resolved
* update code comment * add APIs to return matching nodes for input ports * revert temporary changes made for testing * cleanup * merge api with UI * revert unwanted changes
* add null-check
* Moving GetMatchingNodes API to search view model * Clean Up * Comments * Create a dedicated test file to prepare for more unit tests
@aparajit-pratap @mmisol @reddyashish @reddyashish Any other comments I can address for you? |
{ | ||
ListBoxItem HighlightedItem; | ||
|
||
internal event Action<ShowHideFlags> RequestShowNodeAutoCompleteSearch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QilongTang where is this event registered? How does it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the call but what method is it registered with? Which method gets called eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it does anything currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aparajit-pratap Sorry it is registered in a different file, see: https://github.com/DynamoDS/Dynamo/blob/NodeAutoComplete/src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs#L110
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need to be done on workspace level because we are monitoring all the left click on WS level, so when user left click on anywhere else will dismiss the node autocomplete search bar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different from RequestNodeAutoCompleteSearch
defined on WorkspaceViewModel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry been so long since I wrote this, I debugged and believe it is leveraged as event driven here : https://github.com/DynamoDS/Dynamo/blob/NodeAutoComplete/src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml#L355
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My memory faded so my answer may not be clear before.. There are several kinds of show/hide request handled by different events on different level
- request to show by alt+ left click
- request to hide by left click on canvas
- request to hide when window deactivated, e.g. user switched back to Revit, etc
These requests and events was handled on different level for in-canvas search so I inherited the same architecture, hopefully this answers your questions
@aparajit-pratap Let me know if you have more thoughts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QilongTang just one last question and then I think this looks good for now. Are you going to merge this into master?
@aparajit-pratap Addressed your comments! No wonder the window deactivate -> hide node autocomplete search bar workflow did not work for me.. The workflow itself is hard to debug so I thought it was debugging issue. Yes I intend to merge to master, since this is guarded by the preference setting and default to false without introducing new dll to our core delivery, I think these changes should be safe. |
All tests passed, merging |
Please Note:
DynamoRevit
repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after aLGTM
label is added to the PR.Purpose
Introduce NodeAutoComplete Feature leveraging type matching
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@DynamoDS/dynamo
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of