| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
[DYN-8893] Added search bar in group context menu #16346
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-8893] Added search bar in group context menu #16346
Conversation
github-actions
bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8893
ivaylo-matov
changed the title
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.
Pull Request Overview
Adds a search bar to the group context menu by rebuilding it with a Popup, regenerating items on open, hiding items during search, and auto-adding created nodes to the opened group.
- Updated tests to exercise the new Popup-based ungroup action.
- Exposed
OuterCanvasinWorkspaceViewfor overlay placement. - Refactored
AnnotationViewto use a dynamically built Popup with search, style, and event-handling logic. - Fixed a typo in the "Ungroup" resource strings.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DynamoCoreWpfTests/AnnotationViewTests.cs | Adapted test to raise click on new Popup child instead of ContextMenu |
| src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs | Exposed OuterCanvas property for absolute UI placements |
| src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs | Replaced ContextMenu with Popup, added search bar logic, event wiring |
| src/DynamoCoreWpf/Views/Core/AnnotationView.xaml | Hooked MouseRightButtonUp to open the new Popup |
| src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs | Changed accessors on CanAddGroupToGroup and CanUngroupGroup |
| src/DynamoCoreWpf/Utilities/WpfUtilities.cs | Added FindParent<T> helper |
| src/DynamoCoreWpf/PublicAPI.Unshipped.txt | Registered OuterCanvas getter in public API |
| src/DynamoCoreWpf/Properties/Resources.resx & *.en-US.resx | Corrected the “Ungroup” spelling in resource files |
Files not reviewed (1)
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (1)
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:669
- [nitpick] The search bar visibility toggle logic in
OnSearchViewModelPropertyChangedisn’t covered by existing tests—consider adding unit tests to verify that other menu items hide and show correctly when typing.
if (e.PropertyName != nameof(SearchViewModel.SearchText)) return;
| @@ -206,6 +201,9 @@ private void AnnotationView_Unloaded(object sender, RoutedEventArgs e) | |||
| { | |||
| Loaded -= AnnotationView_Loaded; | |||
| DataContextChanged -= AnnotationView_DataContextChanged; | |||
| ViewModel.WorkspaceViewModel.InCanvasSearchViewModel.PropertyChanged -= OnSearchViewModelPropertyChanged; | |||
| ViewModel.WorkspaceViewModel.Nodes.CollectionChanged -= OnWorkspaceNodesChanged; | |||
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.
Unsubscribe from GroupContextMenuPopup.Closed in the Unloaded handler as well to avoid potential memory leaks from lingering event subscriptions.
Copilot uses AI. Check for mistakes.
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.
Is this an issue?
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.
@ivaylo-matov PTAL
…arch-bar-in-group-context-menu
…ps://github.com/ivaylo-matov/Dynamo into DYN-8893-Added-search-bar-in-group-context-menu
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.
LGTM
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 think there is just one Copilot comment about Unsubscribe from GroupContextMenuPopup.Closed, which might need to be considered. Aside from that, I think it looks good.
@jasonstratton , that's already in
|
…arch-bar-in-group-context-menu
Purpose
This PR partially addresses DYN-8893.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Added a new search bar to the group context menu similar to the one in the workspace menu. Nodes created from it are now added directly to the group.
Reviewers
@DynamoDS/eidos
@jasonstratton
FYIs
@dnenov
@achintyabhat