[DYN-8997] CBN output port to show the name of the variable #16300
Conversation
…ut-port-to-show-the-name-of-the-variable
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8997
|
This looks weird and is a deviation from current output port alignment: The output port should always be placed against the identifier. |
There was a problem hiding this comment.
I suggest removing the special condition for a single statement CBN to have the output port always placed at the topmost line. I don't see why it's needed. I agree with the idea of showing the single output port be bigger than when there are multiple ports but I strongly feel that the output port should always align with the code statement, even when there are whitespaces or comments.
In other words, the placement of the output port should be like it is currently:
There was a problem hiding this comment.
Pull Request Overview
This PR updates the CBN output port behavior so that it displays the variable name used on the left-hand side of code statements, ensuring proper alignment and tooltip display for both single and multiple statement code blocks. It also fixes a connector restoration issue by enforcing unique port identification.
- Outport controls are updated to adjust styling and margins based on the number of code block outputs.
- A new property (IsOnlyOutputPortInCodeBlock) has been introduced to differentiate single from multiple output ports.
- Layout adjustments and API documentation updates have been applied across several files to support these changes.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs | Adjusted outport margins and styling for CodeBlockNodes based on port count. |
| src/DynamoCoreWpf/ViewModels/Core/PortViewModel.cs | Added property to check for a single output port in code blocks. |
| src/DynamoCoreWpf/ViewModels/Core/OutPortViewModel.cs | Updated condensed port condition to incorporate single port logic. |
| src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs | Revised port positioning and introduced a constant for collapsed port height. |
| src/DynamoCoreWpf/PublicAPI.Unshipped.txt | Exposed the new IsOnlyOutputPortInCodeBlock property. |
| src/DynamoCoreWpf/Controls/OutPorts.xaml.cs | Modified UI element margins and sizes based on the port type. |
| src/DynamoCore/Graph/Nodes/PortModel.cs | Adjusted center position calculation to correctly offset code block outputs. |
| src/DynamoCore/Graph/Nodes/CodeBlockNode.cs | Cleared and built output port labels with adjusted line indexing for single vs. multiple ports. |
| OutPorts.Clear(); | ||
| InPorts.Clear(); | ||
| inputPortNames.Clear(); | ||
| outputPortNames.Clear(); |
There was a problem hiding this comment.
[nitpick] There are multiple calls to outputPortNames.Clear() in the ProcessCode method. Consolidating these calls may prevent potential redundancy and clarify the port name collection process.
jasonstratton
left a comment
There was a problem hiding this comment.
This change appears pretty straightforward. I do understand Aparajit's objection and barring a change in the requirements there, I think this looks good to go.
|
And just to validate the current state can you share an updated gif? |
…ut-port-to-show-the-name-of-the-variable
|
@zeusongit , the cbn related test should pass now. |
Dismissing review, to enable merge, please leave comments if you feel the need for more changes. Merging now.
| case IdentifierNode idNode: | ||
| return idNode.Value; | ||
| case IdentifierListNode: | ||
| return "[identifier list]"; |
There was a problem hiding this comment.
I don't believe this is a user-friendly description. I'm confident that many users will not understand this technical wording.
There was a problem hiding this comment.
makes sense, what will be your suggestion in this case?
There was a problem hiding this comment.
There's no straightforward answer, unfortunately. An IdentifierListNode can be a function call (DSCore.List.Sort()), or property invocation (circle.CenterPoint.X). It's essentially any complex expression containing a chain (list) of identifiers, where each identifier can either be a namespace, class, variable, function, or property. I suppose the ideal solution here would be to break down the identifier list and inspect its rightmost AST to see if it's a function or property (a getter function in DS), then to label it as either a function or property.
| case InlineConditionalNode: | ||
| return "[conditional]"; | ||
| default: | ||
| return "[unknown]"; |
There was a problem hiding this comment.
What about imperative language blocks?
[Imperative]
{ ... }
|
|
||
| foreach (var def in allDefs) | ||
| { | ||
| if (i >= outputPortNames.Count) break; |
There was a problem hiding this comment.
This is a finite loop. It runs for each def in allDefs. Why are you breaking out of it in between?
| .Where(expr => !string.IsNullOrEmpty(expr)) | ||
| .Select(expr => expr + ";") | ||
| .ToList(); | ||
| } |
There was a problem hiding this comment.
I'm afraid this function needs to be entirely rewritten, as it shouldn't be using custom string manipulation when we have a full-fledged parser for DesignScript code and syntax. Will send another PR to clean this up.
Purpose
This PR aims to address DYN-8997.
Changes:
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
The output port of code block node now displays the left side of the statement.
Reviewers
@DynamoDS/eidos
@jasonstratton.
FYIs
@dnenov
@achintyabhat