Duplicate connectors using ctrl + left click on input ports with testing#9567
Duplicate connectors using ctrl + left click on input ports with testing#9567alfarok merged 9 commits intoDynamoDS:masterfrom
ctrl + left click on input ports with testing#9567Conversation
| } | ||
| } | ||
|
|
||
| void BeginDuplicateConnection(Guid nodeId, int portIndex, PortType portType) |
There was a problem hiding this comment.
please mark this with an access modifier.
There was a problem hiding this comment.
interestingly almost none of the methods DynamoModelCommands.cs have access modifiers, I guess because they are all expected to be internal?
There was a problem hiding this comment.
internal seems reasonable
There was a problem hiding this comment.
I think if not specified the default is actually private.
There was a problem hiding this comment.
I differs if it is interface or class or methods, see https://stackoverflow.com/questions/2521459/what-are-the-default-access-modifiers-in-c
| /// Begin duplicate connection. | ||
| /// </summary> | ||
| EndAndStartCtrlConnection, | ||
| BeginDuplicateConnection, |
There was a problem hiding this comment.
oye... public breaking change. can you keep the old enum, obsolete it, and add the new ones at the end without changing the order?
| /// followed by ctrl + clicking to copy this connection to 2 additional nodes. | ||
| /// The final assertion will only be true if all connections were successfully established. | ||
| /// </summary> | ||
| [Test, RequiresSTA] |
There was a problem hiding this comment.
just curious, what happens if you remove the requiresSTA?
There was a problem hiding this comment.
doesn't seem to matter but is used in all the previous implementations, I can remove from the tests I added
|
|
@mjkkirschner @QilongTang This should be ready for a final look whenever you guys get a free moment |
| } | ||
|
|
||
| void RunCancelImpl(RunCancelCommand command) | ||
| internal void RunCancelImpl(RunCancelCommand command) |
There was a problem hiding this comment.
@alfarok - did you checkout the link @QilongTang sent - I interpret this to mean these should have been private by default?
|
@alfarok looks good except for the access modifier - if it was private before (by default) lets not change it unless theres a reason to. |
|
Nicely wrapped up, LGTM |
3 participants
Purpose
This PR pulls in the work originally completed by @yeexinc in #7850 and adds several tests that leverage recorded commands. The original work was already reviewed and all active comments were addressed. The recorded tests verify the proper connections are made when using
ctrl + left clickin order to duplicate a connector that is already currently connected to another nodes input port. Additionally undo/redo was verified using recorded commands but copy/paste was manually as verified as they are not currently supported as recorded commands.Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner @aparajit-pratap @QilongTang
FYIs
@Amoursol