DYN-7777 : change workspace hidden eventing#16360
DYN-7777 : change workspace hidden eventing#16360BogdanZavu wants to merge 1 commit intoDynamoDS:masterfrom
Conversation
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7777
There was a problem hiding this comment.
Pull Request Overview
A change to the CurrentWorkspace property so that the OnWorkspaceHidden event fires before updating the workspace reference, ensuring the old workspace remains accessible during event handling.
- Fire OnWorkspaceHidden prior to setting the new workspace
- Remove the temporary
oldvariable and reposition the event call - Retain property change notification after assignment
Comments suppressed due to low confidence (2)
src/DynamoCore/Models/DynamoModel.cs:328
- Since this change affects the public OnWorkspaceHidden event semantics, please update the API changes documentation and include notes on the Semantic Versioning impact.
OnWorkspaceHidden(currentWorkspace);
src/DynamoCore/Models/DynamoModel.cs:328
- Add a unit test that verifies
CurrentWorkspaceremains set to the old workspace inside the OnWorkspaceHidden handler to prevent regressions for this behavior change.
OnWorkspaceHidden(currentWorkspace);
|
@BogdanZavu @johnpierson Can you explain why this is needed? we do feel this as risky this close to release, but we can do a pre-qual before merge. We would like to better understand the need for this and what this will enable for you. |
@zeusongit So we need to perform certain actions/commands ( e.g DeleteNodeCommands for transient nodes ) when current workspace is hidden. The DeleteCommand and I assume others too somewhere during execution rely on the CurrentWorkspace - which was already changed to the new workspace and fails. |
|
I believe @johnpierson found an alternative solution to delete the transient nodes. |
aparajit-pratap
left a comment
There was a problem hiding this comment.
On taking a closer look, this change looks logical to me. LGTM.
Purpose
This is a change in behavior for an existing api but imo it is more of a bug fix.
Basically as an api dev I would expect the current workspace to remain unchanged while I receive and process the OnWorkspaceHidden event.
The reason is that I might need to execute some command that somewhere at some point make use of the current workspace.
We have an alternative as well to introduce OnWorkspaceHiddenStarted or something like that.
Lmk what you think.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@DynamoDS/eidos
FYIs
@DynamoDS/synapse