-
Notifications
You must be signed in to change notification settings - Fork 188
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
[DNM] Remove Dispatcher unhandled exception handler since it's already handled in DynamoCore #3029
base: master
Are you sure you want to change the base?
Conversation
finally | ||
{ | ||
args.Handled = true; | ||
DynamoRevitApp.DynamoButtonEnabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about the button ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The button is reenabled even without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it somewhere in the view.CLose events ?
THere would be a chance that if an exception is thrown in the exception handler, then the button close will not be called at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is re-enabled in OnDynamoViewClosed
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why the API breaking change so late, in a minor build (3.0.2 vs 3.0.0), and why after Revit API freeze?
This would require more testing on Revit side, that various CER scenarios still work and we are not missing crash reports, possibly even more code changed.
Is this API change in 3.0.2 mandatory or can we revert it, or make other builds based on 3.0.0 with only the critical fixes? @QilongTang FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mikhinja this is not an API breaking change - the API change is in this PR: #3032
This PR is about removing the API call entirely in D4R and moving it to DynamoCore instead so other host integrations can benefit from it without needing to call it explicitly in their integration code.
Having said that, the Dynamo team is only experimenting with this so far and we are not ready to merge this for global launch, which is why I've marked this PR as DNM. We will continue refining this approach post 3.0.x.
Note that PR #3032 does need to be merged into D4R when it begins to consume 3.0.1 or 3.0.2 and higher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that change is what I was actually referring to. I merged it together with Dynamo 3.0.2.
Purpose
This PR depends on DynamoDS/Dynamo#14840 first being merged into Dynamo 3.0.3. If not for 3.0.3, it can wait until the Dynamo PR is merged into 3.1.
It skips certain steps in the Dispatcher's unhandled exception handler if the corresponding handler is already invoked in DynamoViewModel.Correction: The Dispatcher's unhandled exception event handler has been moved to DynamoViewModel in DynamoDS/Dynamo#14840. It's no longer needed here.
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@pinzart90
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of