| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
[DYN-8997] CBN output port to show the name of the variable #16300
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-8997] CBN output port to show the name of the variable #16300
Conversation
…ut-port-to-show-the-name-of-the-variable
![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-8997
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
| @@ -805,6 +806,7 @@ private void ProcessCode(out string errorMessage, out string warningMessage, | |||
| OutPorts.Clear(); | |||
| InPorts.Clear(); | |||
| inputPortNames.Clear(); | |||
| outputPortNames.Clear(); | |||
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.
[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.
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.
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. |
zeusongit
dismissed
aparajit-pratap’s stale review
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, what will be your suggestion in this case?
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'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about imperative language blocks?
[Imperative]
{ ... }
| foreach (var def in allDefs) | ||
| { | ||
| if (i >= outputPortNames.Count) break; |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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