Do not set execution hint on undo move #11519
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There were a couple of issues that were causing a node to be set to be executed on undoing an action that consisted of just a move: 1. The 'UsingDefaultValue' property was triggering a property changed even if the deserialized value was the same as the current. This was fixed by checking if the value is the same and not doing anything in that case. 2. Even if the 'IsFrozen' property for the current node had not changed an undo would mark the current and all downstream nodes as if they were modified. This was fixed by relying on the 'IsFrozen' property setter, which correctly handles the cases when the previous should be done.
QilongTang
reviewed
Feb 25, 2021
| { | ||
| usingDefaultValue = value; | ||
| RaisePropertyChanged("UsingDefaultValue"); | ||
| RaisePropertyChanged("ToolTip"); |
There was a problem hiding this comment.
I like the fix here. Also curious does similar problem not exist for other properties, e.g. X, Y etc?
There was a problem hiding this comment.
Yes, this pattern is used in most places. For X and Y, I noticed they are deprecated and there is a Position which is a way to update both. Maybe that's the reason why it's different.
QilongTang
reviewed
Feb 25, 2021
| { | ||
| // For Python nodes, changing the Engine property does not set the node Modified flag. | ||
| // This is done externally in all event handlers, so for Undo we do it here. | ||
| OnNodeModified(); |
QilongTang
reviewed
Feb 25, 2021
| //undo is for toggling freeze. This is ONLY modifying the execution hint. | ||
| // this does not run the graph. | ||
| RaisePropertyChanged(nameof(IsFrozen)); | ||
| MarkDownStreamNodesAsModified(this); |
There was a problem hiding this comment.
Hi @mmisol Did you test that downstream node can be also reverted back to the previous state?
There was a problem hiding this comment.
I'm not sure I follow. Could you mention the steps of the test?
There was a problem hiding this comment.
Sure, if a node is frozen, the downstream nodes are also frozen. The original code seems taking care of correctly marking downstream nodes when undo freezing node as well. Just want to make sure the behavior is the same.
There was a problem hiding this comment.
Oh ok. Yes, I tested that and it works
QilongTang
approved these changes
Feb 25, 2021
mmisol
added a commit
to mmisol/Dynamo
that referenced
this pull request
Mar 2, 2021
There were a couple of issues that were causing a node to be set to be executed on undoing an action that consisted of just a move: 1. The 'UsingDefaultValue' property was triggering a property changed even if the deserialized value was the same as the current. This was fixed by checking if the value is the same and not doing anything in that case. 2. Even if the 'IsFrozen' property for the current node had not changed an undo would mark the current and all downstream nodes as if they were modified. This was fixed by relying on the 'IsFrozen' property setter, which correctly handles the cases when the previous should be done.
QilongTang
pushed a commit
that referenced
this pull request
Mar 2, 2021
There were a couple of issues that were causing a node to be set to be executed on undoing an action that consisted of just a move: 1. The 'UsingDefaultValue' property was triggering a property changed even if the deserialized value was the same as the current. This was fixed by checking if the value is the same and not doing anything in that case. 2. Even if the 'IsFrozen' property for the current node had not changed an undo would mark the current and all downstream nodes as if they were modified. This was fixed by relying on the 'IsFrozen' property setter, which correctly handles the cases when the previous should be done.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
There were a couple of issues that were causing a node to be set to be
executed on undoing an action that consisted of just a move:
The 'UsingDefaultValue' property was triggering a property changed
even if the deserialized value was the same as the current. This was
fixed by checking if the value is the same and not doing anything in
that case.
Even if the 'IsFrozen' property for the current node had not changed
an undo would mark the current and all downstream nodes as if they were
modified. This was fixed by relying on the 'IsFrozen' property
setter, which correctly handles the cases when the previous should be
done.
Declarations
Check these if you believe they are true
*.resxfiles