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

Set the default color in the color palette when using node autocomplete. #12819

Merged
merged 2 commits into from
Apr 26, 2022

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Apr 21, 2022

Purpose

This is a followup to the task: https://jira.autodesk.com/browse/DYN-4753.

When the color palette node is placed using the node autocomplete, the default color(black) was not being set.

The reason for this was that there was an exception being thrown in here: https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/ColorPalette.cs#L42 and the view customization would not finish.
The exception is a custom one that is related to UndoRedo operation:

const string message = "An existing open undo group detected";

The undo redo operation around this node works fine and I see this WorkspaceModel.RecordModelForModification() being called only for couple other nodes. There is a TODO on that API, saying we should refactor that and move the code to the WorkspaceViewModel. For now, I made sure that this would be the last step in node view customization so that everything else will be executed even if this throws an exception. We could also catch this exception and handle it accordingly. Any thoughts?

Declarations

Check these if you believe they are true

  • The codebase 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.
  • 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

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

@QilongTang @zeusongit

@QilongTang
Copy link
Contributor

I am fine with this being a temp fix, it seems this issue exist for all node customization applied to node from autocomplete though. @zeusongit what do you think?

@QilongTang QilongTang added this to the 2.15.0 milestone Apr 22, 2022
@zeusongit
Copy link
Contributor

I think the fix is good enough for now, but we should file a task to handle the exception and not add another undo recorder that throws the exception.

@reddyashish
Copy link
Contributor Author

@QilongTang @zeusongit I have removed that API call and tested it locally. This is specific to the color palette node.

@reddyashish
Copy link
Contributor Author

@reddyashish reddyashish merged commit 2476003 into DynamoDS:master Apr 26, 2022
@reddyashish reddyashish deleted the nodeautocompletecrash branch November 23, 2022 07:33
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.

3 participants