-
Notifications
You must be signed in to change notification settings - Fork 668
Selective shift + click output connections grabs connectors for selected nodes only #9585
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
Conversation
|
All tests passing on the self-serve. It originally showed 3 failing tests but that was because #9580 was not pulled into this branch. After pulling in the latest additions from master I verified these do pass with the fix. |
|
one comment thne LGTM, thank you for all the detailed comments |
| { | ||
| ConnectorModel connector = selectedPort.Connectors[i]; | ||
| ConnectorModel connector = selectedConnectors[i]; | ||
| connectorsForDeletion.Add(connector); |
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.
connectorsForDeletion seems a dup to me after the refactor? What do you think
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 it's still required as removing it will break undo/redo. Below we call CurrentWorkspace.SaveAndDeleteModels(connectorsForDeletion) where SaveAndDeleteModels() is a method in UndoRedo.cs which is a partial class of the WorkspaceModel. Several new and old recorded tests break as well.
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 understand before the refactor it is required, but after the refactor, it contains exactly all the connectors in selectedConnectors.
Can we call code below to preserve the undo/redo behavior?
CurrentWorkspace.SaveAndDeleteModels(selectedConnectors);
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 we can do this:
CurrentWorkspace.SaveAndDeleteModels(selectedConnectors.ToList<ModelBase>());Since connectorsForDeletion = List<ModelBase> while selectedConnectors = List<ConnectorModel> and ConnectorModel is derived from ModelBase
|
LGTM to me now. Thank you for addressing the comment |
Purpose
This PR pulls in the work originally completed by @yeexinc in #7900 and adds several tests that leverage recorded commands. The recorded tests verify the proper connectors are grabbed when a
shift+clickoperation occurs. Only selected downstream nodes that are connected to the given output port will be removed uponshift+clicking. Testing coverage also verifies the graph remains in a known state after several undo/redo commands are conducted after using selective shift-click operations. The last test performs a large series ofshift+clicksandctrl+clickswith and without selected nodes to test these commands in successive combinations verifying the graph's final state produce a specific numeric combination. All tests pass on the self-serve CI.Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner @QilongTang