Skip to content

Conversation

BogdanZavu
Copy link
Contributor

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

  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB
  • This PR introduces new feature code involve network connecting and is tested with no-network mode.

Reviewers

@DynamoDS/eidos

FYIs

@DynamoDS/synapse

Copy link

@github-actions github-actions bot left a 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-7777

@BogdanZavu BogdanZavu requested a review from mjkkirschner July 10, 2025 15:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 old variable 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 CurrentWorkspace remains set to the old workspace inside the OnWorkspaceHidden handler to prevent regressions for this behavior change.
                OnWorkspaceHidden(currentWorkspace);

@zeusongit
Copy link
Contributor

zeusongit commented Jul 11, 2025

@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.
fyi @aparajit-pratap

@BogdanZavu
Copy link
Contributor Author

BogdanZavu commented Jul 11, 2025

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. fyi @aparajit-pratap

@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.
More generically as an API developer I want to perform certain actions/commands when the current workspace is changed. When I receive OnWorkspaceHidden my expectation is that things are still in place (mostly) related to the workspace (the old one) where I want to execute those commands.

@BogdanZavu
Copy link
Contributor Author

BogdanZavu commented Jul 11, 2025

I believe @johnpierson found an alternative solution to delete the transient nodes.
This change is still good I think but we can postpone it after 3.6.

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On taking a closer look, this change looks logical to me. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants