Skip to content
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

Python script editor should provide a workspace ID when updating model #9988

Merged
merged 7 commits into from
Nov 20, 2019

Conversation

Dewb
Copy link
Contributor

@Dewb Dewb commented Sep 18, 2019

Purpose

The Python node's code editor sends an UpdateModelValueCommand with an empty workspace GUID, meaning the current workspace. Since the Python editor is no longer modal, this assumption (that the parent workspace is current) no longer holds. This change explicitly provides the workspace ID. This fixes #9479, avoiding a crash when a modeless Python node editor is opened from a custom node workspace and "Run" or "Save" is clicked while the home workspace is current.

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • 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
  • All tests pass using the self-service CI.
  • [n/a] Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

@Dewb Dewb added the DNM Do not merge. For PRs. label Sep 18, 2019
@Dewb
Copy link
Contributor Author

Dewb commented Sep 18, 2019

Need to add a test for #9479 and confirm that the API change to add a parameter to the internal method ScriptEditorWindow.Initialize doesn't violate our API policies.

@Dewb Dewb changed the title Python node should provide a workspace ID when updating model Python script editor should provide a workspace ID when updating model Sep 18, 2019
@@ -47,8 +48,9 @@ public partial class ScriptEditorWindow : ModelessChildWindow
Dynamo.Logging.Analytics.TrackScreenView("Python");
}

internal void Initialize(Guid nodeGuid, string propName, string propValue)
internal void Initialize(Guid workspaceGuid, Guid nodeGuid, string propName, string propValue)
Copy link
Member

Choose a reason for hiding this comment

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

this should be fine as it's an internal method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The entire class probably ought to be internal instead of public, as it's not useful without being able to call the Initialize method.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dewb In this case, can you make a comment to the class, e.g. TODO: Make this class internal in Dynamo 3.0? By the time we tackle it, we would remember to do this.

@mjkkirschner
Copy link
Member

@Dewb this seems like a good crash fix, do need help finishing up the tests so that it might make it into 2.5?

@Dewb
Copy link
Contributor Author

Dewb commented Nov 13, 2019

Tests are still blocked by the undo bug. Let's talk tomorrow about a strategy to get these both resolved.

@mjkkirschner
Copy link
Member

tracked with https://jira.autodesk.com/browse/DYN-2155

@Dewb Dewb removed the DNM Do not merge. For PRs. label Nov 20, 2019
@Dewb
Copy link
Contributor Author

Dewb commented Nov 20, 2019

I believe this is ready to merge.

@mjkkirschner
Copy link
Member

@Dewb thanks! this looks good to me, we can take cherry picking this to 2.5 RC branch.

@mjkkirschner mjkkirschner added this to the 2.5.0 milestone Nov 20, 2019
@mjkkirschner mjkkirschner added the LGTM Looks good to me label Nov 20, 2019
@mjkkirschner mjkkirschner merged commit e775794 into DynamoDS:master Nov 20, 2019
mjkkirschner pushed a commit to mjkkirschner/Dynamo that referenced this pull request Nov 26, 2019
DynamoDS#9988)

* Python node should provide a workspace ID when calling UpdateModelValueCommand, fixing DynamoDS#9479

* Draft test for DynamoDS#9479 (not passing yet due to undo issues)

* Disable section of python custom node save test that uses undo

* Remove file added by accident

* Fix null dereference in UpdateModelValueImpl

(cherry picked from commit e775794)
mjkkirschner added a commit that referenced this pull request Nov 26, 2019
#9988) (#10173)

* Python node should provide a workspace ID when calling UpdateModelValueCommand, fixing #9479

* Draft test for #9479 (not passing yet due to undo issues)

* Disable section of python custom node save test that uses undo

* Remove file added by accident

* Fix null dereference in UpdateModelValueImpl

(cherry picked from commit e775794)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Leaving Python Window Open from Custom Node Workspace, then Saving Causes Crash
4 participants