| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
[DYN-6236] Save operation after Save-As shouldn't revert node GUID's #16365
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
![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-6236
|
The only failing test TestReconnectionUndo looks flaky and is passing locally. Running the job again: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/18165/ |
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 ensures that after performing a Save-As, a subsequent Save does not revert node GUIDs by fully reloading the workspace.
- Add and run a unit test verifying GUIDs change after Save-As then Save
- Update workspace save logic to clear and reopen the new file on Save-As
- Fix null-reference and access modifiers to support the reload flow
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/UI/BasicAdditionDuplicate.dyn | New test graph file used for Save-As GUID uniqueness verification |
| test/DynamoCoreWpf3Tests/WorkspaceSaving.cs | Added WorkspaceSaveAfterSaveAsNewFile test and fixed typo |
| src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs | Made DelayNodePreviewControl.Cancel() null-safe |
| src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs | After Save-As, clear and reopen workspace to apply new GUIDs |
| src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs | Assign filePath from current workspace during deserialization |
| src/DynamoCore/Models/DynamoModel.cs | Changed OpenJsonFileFromPath to internal for reuse |
Comments suppressed due to low confidence (4)
test/DynamoCoreWpf3Tests/WorkspaceSaving.cs:20
- The
Microsoft.VisualBasicnamespace import appears unused. Consider removing it to reduce clutter and avoid unnecessary dependencies.
using Microsoft.VisualBasic;
src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs:2151
- The local identifier
viewModelmay not be defined in this scope—previous code usesViewModel. This will fail to compile; replaceviewModelwith the correct reference (e.g.ViewModel).
viewModel.WorkspaceViewModel.DelayNodePreviewControl?.Cancel();
src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs:865
- The variable
saveContentis not defined in this method. It looks like you intended to use the JSON string that was just saved (likelyfileContents); please correct the variable name to avoid a compile error.
DynamoViewModel.Model.OpenJsonFileFromPath(saveContent, filePath, false);
src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs:2348
- The identifier
filePathmay not be declared in this scope. If this is intended to set a field, ensure it's declared at class level; otherwise declare a new local variable.
filePath = Model.CurrentWorkspace.FileName;
| { | ||
| DynamoViewModel.Model.ClearCurrentWorkspace(); | ||
| DynamoViewModel.Model.OpenJsonFileFromPath(saveContent, filePath, false); | ||
| } |
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 happens for .dyf files?
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.
testing with dyf's
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.
Don't see the issue with custom nodes. They are uniquely generated and not reverted to the old ones.
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.
Does OpenJsonFileFromPath update WorkspaceModel with the current saveContent?
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 looks counterintuitive to clear the current workspace and reopen the json file when one is doing a save-as. Is there a better way to update the workspace model on a save-as? It feels more appropriate to update the workspace model after updating saveContent in line 845.
|
Merging this to get the testing started. If there are any comments from the team, I can address it in a different PR. |
| @@ -2345,6 +2345,7 @@ private void model_ComputeModelDeserialized() | |||
| { | |||
| try | |||
| { | |||
| filePath = Model.CurrentWorkspace.FileName; | |||
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.
filepath is assigned but never used?
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.
It is used in the next line to read the text from this new current workspace.
Purpose
https://jira.autodesk.com/browse/DYN-6236.
The issue was that after save-as operation, we were just updating the workspace name and copying other attributes but the old nodes were linked to the workspace. After this change, we will now be opening the workspace with the new nodes and any subsequent save operation will consider the new nodes in the workspace and the GUIDs won't be reverted back to the original node.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
[DYN-6236] Save operation after Save-As shouldn't revert node GUID's
Reviewers
@DynamoDS/eidos