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.

Sign up for GitHub

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

Jump to bottom

DYN-2381 Exclude code block nodes from SetArgumentLacing #11294

Merged
merged 4 commits into from
Dec 4, 2020
Merged

DYN-2381 Exclude code block nodes from SetArgumentLacing #11294

merged 4 commits into from
Dec 4, 2020

Conversation

StudioLE
Copy link
Contributor

Purpose

JIRA: DYN-2381

Exclude Code Block Nodes when setting the lacing by selecting multiple nodes and right clicking on the canvas.

The previous behaviour did nothing but causes some visual anomalies and can be confusing to a user.

Previous Behaviour:

DYN-2381-CBN-Lacing-Before

Revised Behaviour

DYN-2381-CBN-Lacing-Complete

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

Reviewers

@mjkkirschner @QilongTang @mmisol

FYIs

@Amoursol

mmisol
mmisol reviewed Nov 30, 2020
View reviewed changes
Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

While this PR does what it was asked for, this is not only a problem with Code Block nodes. Python Script nodes suffer the same issue and probably others do.

By taking a look at the code, I could see NodeModel has a property named ArgumentLacing that should be set to LacingStrategy.Disabled when lacing is not supported by the node. Using that, rather than excluding by type like in https://github.com/DynamoDS/Dynamo/pull/11294/files#diff-14fa24535a2d0aa34f993496cc35eb02dbadcffb7576a54cbeef2e9821edba8cR1131, should be the right way to solve this IMO.

@StudioLE
Copy link
Contributor Author

StudioLE commented Dec 1, 2020

While this PR does what it was asked for, this is not only a problem with Code Block nodes. Python Script nodes suffer the same issue and probably others do.

By taking a look at the code, I could see NodeModel has a property named ArgumentLacing that should be set to LacingStrategy.Disabled when lacing is not supported by the node. Using that, rather than excluding by type like in https://github.com/DynamoDS/Dynamo/pull/11294/files#diff-14fa24535a2d0aa34f993496cc35eb02dbadcffb7576a54cbeef2e9821edba8cR1131, should be the right way to solve this IMO.

Thanks @mmisol. I've addressed that in def9416

mmisol
mmisol approved these changes Dec 2, 2020
View reviewed changes
Copy link
Collaborator

@mmisol mmisol left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @StudioLE

@QilongTang QilongTang merged commit 44ebf6e into DynamoDS:master Dec 4, 2020
QilongTang pushed a commit that referenced this pull request Dec 4, 2020
@StudioLE @QilongTang
DYN-2381 Exclude code block nodes from SetArgumentLacing (#11294)
16b58dd 
* Exclude code block nodes from SetArgumentLacing

* Added test to check code block lacing doesn't change

* Restructured code block lacing test

* Exclude all LacingStrategy.Disabled nodes from SetArgumentLacing
@QilongTang QilongTang mentioned this pull request Dec 4, 2020
Merged
8 tasks
@QilongTang QilongTang added this to the 2.10.0 milestone Dec 4, 2020
QilongTang added a commit that referenced this pull request Dec 4, 2020
@QilongTang @StudioLE
DYN-2381 Exclude code block nodes from SetArgumentLacing (#11294) (#1…
e74212a 
…1327)

* Exclude code block nodes from SetArgumentLacing

* Added test to check code block lacing doesn't change

* Restructured code block lacing test

* Exclude all LacingStrategy.Disabled nodes from SetArgumentLacing

Co-authored-by: Laurence Elsdon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmisol mmisol mmisol approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@StudioLE @mmisol @QilongTang