DYN-8896 Code Only Node View #16188
DYN-8896 Code Only Node View #16188
Conversation
# Conflicts: # src/DynamoCoreWpf/Views/Core/NodeView.xaml # src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8896
| // Images are named for ease of use | ||
| // As of Dynamo 3.5, the number of images in a node view is 9 after the addition of the TransientImage | ||
| Assert.AreEqual(9, imgs.Count()); | ||
| Assert.AreEqual(1, imgs.Count()); |
There was a problem hiding this comment.
so did we remove 8 images?
There was a problem hiding this comment.
Yes and no. We removed the Extra Grid wrapper around the glyphs and collapse the images directly instead. In this case however the test was always flawed. It never tested if the actual image control for this node existed. The change also addresses that issue
There was a problem hiding this comment.
Pull Request Overview
This PR converts the Dynamo NodeView and Port controls from XAML to C#-only implementations to improve loading performance and reduce memory usage.
- Implements
OutPorts(and similarlyInPorts) as code-only WPF controls - Removes merged XAML resource dictionaries and updates viewmodel brush accessibility
- Updates existing UI tests to assert on
Opacityinstead ofVisibilityand adjust child counts
Reviewed Changes
Copilot reviewed 9 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| test/DynamoCoreWpf3Tests/NodeViewTests.cs | Updated tests to check Opacity values instead of Visibility |
| test/DynamoCoreWpf3Tests/NodeViewCustomizationTests.cs | Adjusted TextBlock counts and image lookups for code-only views |
| src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs | Removed unused InPortsDictionary and OutPortsDictionary |
| src/DynamoCoreWpf/ViewModels/Core/PortViewModel.cs | Changed brush fields from protected to internal |
| src/DynamoCoreWpf/Controls/OutPorts.xaml.cs | Added full C# implementation of OutPorts control |
Files not reviewed (6)
- src/DynamoCoreWpf/Controls/InPorts.xaml: Language not supported
- src/DynamoCoreWpf/Controls/OutPorts.xaml: Language not supported
- src/DynamoCoreWpf/DynamoCoreWpf.csproj: Language not supported
- src/DynamoCoreWpf/PublicAPI.Unshipped.txt: Language not supported
- src/DynamoCoreWpf/UI/Themes/Modern/DataTemplates.xaml: Language not supported
- src/DynamoCoreWpf/UI/Themes/Modern/OutPortsOld.xaml: Language not supported
Comments suppressed due to low confidence (3)
test/DynamoCoreWpf3Tests/NodeViewCustomizationTests.cs:311
- [nitpick] Using a generic name like "image1" can be unclear; consider giving the Image a more descriptive Name or referencing it via a constant.
var img = imgs.First(x => x.Name == "image1");
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:35
- Removing these merged dictionaries may affect resources (styles, templates) expected by child controls; verify that no templates or brushes are missing at runtime.
Resources.MergedDictionaries.Add(SharedDictionaryManager.InPortsDictionary);
src/DynamoCoreWpf/Controls/OutPorts.xaml.cs:48
- The variable name uses inconsistent capitalization in 'portBackGroundColor'; consider renaming to 'portBackgroundColor' for consistent camelCase.
private SolidColorBrush portBackGroundColor = PortViewModel.PortBackgroundColorDefault;
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.nodeColorOverlayZoomOut.Opacity, 0.5); // The Color State Border overlay | ||
|
|
||
| // Zoom in, more than 0.4 | ||
| wvm.Zoom = 0.6; | ||
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.zoomGlyphsGrid.Visibility, System.Windows.Visibility.Collapsed); | ||
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.nodeColorOverlayZoomOut.Visibility, System.Windows.Visibility.Collapsed); | ||
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.nodeColorOverlayZoomOut.Opacity, 0); |
There was a problem hiding this comment.
Comparing doubles by exact equality can lead to flaky tests; consider using the Assert.AreEqual overload with a tolerance, e.g., Assert.AreEqual(0.5, actualOpacity, 0.01).
Copilot uses AI. Check for mistakes.
|
@saintentropy testing the new copilot PR review |
|
Addressed comments, fixes the white rectangle appearing for right click on ports, reference colors and brushes from SharedDictionary. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Dynamo NodeView and port controls to be code-only WPF controls (removing heavy XAML parsing), updates associated templates, and adjusts the project file and tests to reference the new controls.
- Introduces
InPortsandOutPortsUserControls and hooks them up via DataTemplates. - Removes old XAML resource dictionaries for ports and merges needed resources into code.
- Updates API entries and accessibility of certain brush fields, and adjusts tests to match new properties.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoCoreWpf/Controls/InPorts.xaml | Added new code-only InPorts UserControl |
| src/DynamoCoreWpf/Controls/OutPorts.xaml | Added new code-only OutPorts UserControl |
| src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs | Removed old InPorts/OutPorts resource dictionaries |
| src/DynamoCoreWpf/ViewModels/Core/PortViewModel.cs | Changed port brush fields from protected to internal |
| src/DynamoCoreWpf/PublicAPI.Unshipped.txt | Updated public API entries for new controls and accessor methods |
| src/DynamoCoreWpf/UI/Themes/Modern/DataTemplates.xaml | Updated DataTemplates to use new InPorts and OutPorts controls |
Comments suppressed due to low confidence (3)
src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs:35
- Removing the InPorts and OutPorts resource dictionaries may break style or template resolution for port views; ensure these resources are provided elsewhere or that this removal is intentional.
Resources.MergedDictionaries.Add(SharedDictionaryManager.InPortsDictionary);
src/DynamoCoreWpf/ViewModels/Core/PortViewModel.cs:33
- [nitpick] Changing
PortBackgroundColorDefault(andPortBorderBrushColorDefault) from protected to internal alters the public API and may affect subclassing in external assemblies; verify this matches the intended API design.
internal static readonly SolidColorBrush PortBackgroundColorDefault = new SolidColorBrush(Color.FromRgb(60, 60, 60));
src/DynamoCoreWpf/PublicAPI.Unshipped.txt:462
- These public API changes should be mirrored in the official API Changes document and follow Semantic Versioning; please update the API Changes doc accordingly.
Dynamo.Controls.NodeView.inputGrid.get -> System.Windows.Controls.Grid
|
|
||
| // Zoom in, more than 0.4 | ||
| wvm.Zoom = 0.6; | ||
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.zoomGlyphsGrid.Visibility, System.Windows.Visibility.Collapsed); | ||
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.nodeColorOverlayZoomOut.Visibility, System.Windows.Visibility.Collapsed); | ||
| Assert.AreEqual(nodeViewWarningWarningFrozenHidden.nodeColorOverlayZoomOut.Opacity, 0); |
There was a problem hiding this comment.
This is failing the test
| private Grid _inputGrid = null; | ||
|
|
||
| //Todo add message to mark this as deprecated or ContentGrid? Currently only one item references ContentGrid. Most use inputGrid | ||
| public Grid inputGrid |
There was a problem hiding this comment.
My only comment about moving the xaml code to the code behind is that in the future we should consider to add the controls to NodeView in code behind dinamically based in if they are used or not (so probably this code will need to be re-organized).
e.g. in the image below is shown all the controls inside the NodeView.xaml (we have 21 controls) some of them like customNodeBorder0 or customNodeBorder1 or some Canvas are useless for Out Of The Box nodes (they are used just for Custom Nodes), so I consider we should add them under specific circumstances.
There was a problem hiding this comment.
@RobertGlobant20 yes, I think that is what is happening here when we are using the code behind, we check if the node is a custom node for example, and only then add the border. This happens for most controls in the nodeView.
|
@zeusongit marked tests as failing? how many are failing |
3 are failing: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/17914/ |
|
|
||
| private void OnDataContextChanged(object sender, DependencyPropertyChangedEventArgs e) | ||
| { | ||
| if (null != viewModel) return; |
There was a problem hiding this comment.
The data context is the viewModel, correct? Why does it need to be null for this function to execute?
There was a problem hiding this comment.
This isn't something new, this was here before the change as well, I think we only want to set datacontext once at initialization for nodes
There was a problem hiding this comment.
I think is because we are creating the NodeView.xaml (and now InPorts.xaml and OutPorts.xaml) using a DataTemplate (every time a NodeViewModel is found), so the creation of instances is done automatically and the bound with the ViewModel is also done automatically, then the same mechanism applied for NodeView was applied to InPort and OutPorts.
Purpose
The Purpose of this PR is to convert the Dynamo NodeView Control and InPorts.xaml and OutPorts.xaml to a C# only set of WPF Controls. The reason is to avoid the overhead of the parsing the xaml in cases where the graph has a large number of nodes. The code only option also allows for some simplification of the NodeView and PortView by allowing the NodeViews to be mutated in situations where their Properties vary (ie Output Port for CodeBlock vs Standard Nodes) in if statements versus via embed triggers in the xaml.
This mechanism will vary in terms of performance but generally speaking this PR is 2x to 3x faster in Load time and 50% reduction in RAM memory allocation.
Some Profiling results:
In sample graph with 512 PointByCoordinate Nodes the load time reduces from ~24sec to ~10sec and the total application RAM allocation after opening the file reduces from 1.2gig to 650mb.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Reviewers
TBD
FYIs
@achintyabhat @zeusongit @QilongTang