DYN-1899: Node Execution Events #9823
Conversation
| @@ -135,6 +135,38 @@ private void OnDispatchedToUI(object sender, UIDispatcherEventArgs e) | |||
| /// </summary> | |||
| public event Action<PortModel> PortDisconnected; | |||
|
|
|||
| public class NodeExecutionEventArgs : EventArgs | |||
There was a problem hiding this comment.
all the public properties and the class itself should have some ///summary tags please.
| } | ||
|
|
||
| /// <summary> | ||
| /// Event triggered before a node is executed. |
There was a problem hiding this comment.
I think this comment should indicate that these events are only fired when profiling is enabled.
There was a problem hiding this comment.
alternatively we could rename them to be less general.
There was a problem hiding this comment.
Notes have been added to both events to indicate that they will only be fired when profiling is turned on. My thought is that the names should remain general in the case that it is decided to make the event firing non-dependent on profiling being turned on so to not break the API.
|
|
||
| internal void OnNodeExecutionBegin(object data) | ||
| { | ||
| NodeExecutionBegin?.Invoke(this, new NodeExecutionEventArgs(this, data)); |
There was a problem hiding this comment.
it appears this passes the nodes evaluated output to this handler?
Can you confirm that in one of the tests?
To clarify, I mean, what is data here?
| } | ||
|
|
||
| [Test] | ||
| public void TestNodeExecutionEvents() |
There was a problem hiding this comment.
can we add test that asserts the value of the data object? Or get rid of that property if we think it should not be passed.
There was a problem hiding this comment.
After looking into the tests for the data object it seemed to me that the data object was redundant because the NodeModel object was the important information which was already being passed in. I removed the class that contained the NodeModel GUID and the now extraneous data and just rely on the NodeModel object that was being passed through already.
|
LGTM |
|
LGTM but what's up with so many merge commits.. |
|
I'm not sure about the number of merge commits. If there is a way to squash them please let me know. The only thing I have heard is that I should delete my fork and re-create it, but it seems to me that there should be a better way to do it. |
* Update changes from librarie.js to fix QNTM-3710 * Add new events and tests * Remove unnecessary arguments * Remove unneeded arguments
* Update changes from librarie.js to fix QNTM-3710 * Add new events and tests * Remove unnecessary arguments * Remove unneeded arguments
Please Note:
DynamoRevitrepo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after aLGTMlabel is added to the PR.Purpose
This PR adds the ability to fire events at the beginning and end of node execution when profiling.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner