-
Notifications
You must be signed in to change notification settings - Fork 635
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
Node auto layout for node autocomplete #11177
Node auto layout for node autocomplete #11177
Conversation
|
||
// Clear current selections and select all input nodes | ||
DynamoSelection.Instance.ClearSelection(); | ||
var inputNodes = portModel.Owner.InputNodes.Values.Where(x => x != null).Select(y => y.Item2); |
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.
Can you explain the Item2 part? I think I tried this one before by just calling Select and did not work.. Maybe i missed something. You are doing some smart thing for sure 👍
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.
Haha, nothing smart here. InputNodes
property on NodeModel
is of type IDictionary<int, Tuple<int, NodeModel>>
. I'm just getting the NodeModel
part of the tuple.
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
If there is a chance, I would like to get some knowledge share around undo/redo recorder and what it means to reuse it. That is the only piece I am not sure if the way in this PR will be something we want to promote in the feature. |
@QilongTang I have done some final refactor - I have moved the commands from |
var adjustedX = initialNodeVm.X; | ||
|
||
var createAsDownStreamNode = portModel.PortType == PortType.Output; | ||
// Placing the new node based on which input port it is connecting to. |
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.
Which port?
Model.CreationName, 0, portModel.Index, adjustedX, 0, createAsDownStreamNode, false, true)); | ||
|
||
// Clear current selections and select all input nodes as we need to perform Auto layout on only the input nodes. | ||
DynamoSelection.Instance.ClearSelection(); |
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 Do you know if selection is part of undo/redo sequence? I have been thinking about if there could be any side effects to this but in general feels OK
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 not part of the undo/redo sequence. I think you'll need to use one of the selection related commands to add it to undo/redo queue.
Purpose
https://jira.autodesk.com/browse/DYN-2978
This builds on top of PR #11164.
The node chosen from node autocomplete suggestions is created, connected, and then placed relative to the initial node using the graph auto layout. A single undo-recorder group is initialized for this entire operation so that it can be undone in a single step. The following are the sequence of steps:
CreateAndConnectCommand
to make it use an existing undo-redo recorder group instead of creating a new oneNodeViewReady
event on DynamoViewModel is subscribed to. Only when this event is raised does the size of the new node created get set. Only once the size is set, can graph layout work correctly.NodeViewReady
event handler the graph auto layout is performed using only the input nodes to the initial node. This is to ensure that the initial node does not jump to a new location on graph layout.Please hide whitespace to review this PR.
Declarations
Check these if you believe they are true
*.resx
filesFYIs
@Amoursol