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.

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Jan 18, 2024

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

  • The code base 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
  • Snapshot of UI changes, if any.

Reviewers

@pinzart90

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

@aparajit-pratap aparajit-pratap changed the title Skip handling Dispatcher unhandled exception if already handled in DynamoCore [DNM] Skip handling Dispatcher unhandled exception if already handled in DynamoCore Jan 18, 2024
@aparajit-pratap aparajit-pratap changed the title [DNM] Skip handling Dispatcher unhandled exception if already handled in DynamoCore [DNM] Remove Dispatcher unhandled exception handler since it's already handled in DynamoCore Jan 22, 2024
finally
{
args.Handled = true;
DynamoRevitApp.DynamoButtonEnabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about the button ?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

@Mikhinja Mikhinja Jan 23, 2024

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

Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Jan 23, 2024

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.

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants