Deleting assignment statements after function definition in CBN now updates output ports#10736
Conversation
mmisol
left a comment
mmisol
left a comment
There was a problem hiding this comment.
Code change looks good. Can we add a unit test?
|
I find it strange that the node preview for the CBN containing the function definition continues to show the previous value (see gif). I'm trying to fix that as well. |
aparajit-pratap
changed the title
Also fixed (see updated gif) |
| // requested to update its value. When the QueryMirrorDataAsyncTask | ||
| // returns, it will update cachedMirrorData with the latest value. | ||
| // | ||
| lock (cachedValueMutex) |
There was a problem hiding this comment.
can the mutex be removed? What else locks on it? Why remove it?
There was a problem hiding this comment.
is there behavior that depended on cachedValue being set null?
There was a problem hiding this comment.
CachedValue is still being set to null in certain cases, one such case being this one where code is deleted from the CBN, leaving only the function definition in place. This is now happening in line: 2354 below. Earlier only the field was being set to null and not the property because of which the property changed event wasn't being raised. As a result, the preview of the node didn't update to null upon updating the CBN.
As CachedValue is being set to a non-null value outside of the lock anyway, I thought it would be safe to set it to null in cases where the mirror is null. Also the lock was in place specifically for QueryMirrorDataAsyncTask as per the comments, which has now been removed.
There was a problem hiding this comment.
I can check if the mutex is being used elsewhere and if it can be removed altogether.
There was a problem hiding this comment.
Ok, I just checked the cachedValueMutex is being used implicitly in the CachedValue getter and setter itself so it is safe to remove from here. Nothing more to do here.
| Assert.AreEqual(0, cbn.OutPorts.Count); | ||
| Assert.AreEqual(0, cbn.AllConnectors.Count()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add a test for a case in which we have the same initial state but after the edit we also have the assignment a = [ 1, 2 ];?
| Assert.AreEqual(0, cbn.OutPorts.Count); | ||
| Assert.AreEqual(0, cbn.AllConnectors.Count()); | ||
| } | ||
|
|
| //Check the CBN for input/output ports | ||
| Assert.AreNotEqual(ElementState.Error, cbn.State); | ||
| Assert.AreEqual(1, cbn.OutPorts.Count); | ||
| Assert.AreEqual(0, cbn.OutPorts.Count); |
There was a problem hiding this comment.
Was this test doing an assertion that was incorrect and is now fixed by the change here?
There was a problem hiding this comment.
Correct. Now that we remove the output ports for a function def, the count was updated to zero for this particular test.
4 participants
Purpose
QueryMirrorDataAsyncTaskas all usages were removed in Fix MAGN-9568 Crash in preview control #6240Declarations
Check these if you believe they are true
*.resxfilesReviewers
@DynamoDS/dynamo