-
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
Keep the revit element with Node when Disable Transaction #2761
base: master
Are you sure you want to change the base?
Keep the revit element with Node when Disable Transaction #2761
Conversation
@@ -100,8 +100,13 @@ public int UnRegisterAssociation(T elementID, Object wrapper) | |||
//ID already existed, check we're not over adding | |||
if (existingWrappers.Contains(wrapper)) | |||
{ | |||
int index = existingWrappers.FindIndex((x) => object.ReferenceEquals(x, wrapper)); | |||
existingWrappers.RemoveAt(index); | |||
var removeList = existingWrappers.FindAll((x) => object.ReferenceEquals(x, wrapper)); |
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 case triggered this change? Could index ever be -1? (not found?)
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.
When I debug disable/enable transaction, I found that existingWrappers contains multiple wrapper, but no one can be referenceEqueals.
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.
This may be debugging code, I will solve this, but I think it would be better to add a judgment protection to the index.
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.
Thank you @ZiyunShang for the update.
I still have concern about this implementation. A lot of nodes need to check the state of DisableTransaction, which requires Dynamo developers to keep in mind that the node may run in DisableTransaction mode, and need to deal with the behavior. And it also requires Dynamo developers to be familiar with the API internal implementation, know if the API calls regen or starts Transaction internally (API can call regen itself, and UI API may start and commit transactions)
Moreover, even doing all above, this only works for the built-in nodes. It doesn't work for package nodes, python nodes, custom nodes, which are also in the AC list.
@saintentropy @QilongTang any ideas?
Purpose
After add Transaction control, an issue was found that element binding was broken when disable and enable transaction. So we need to keep the Revit elementbinder with D4R nodes.
According to design, when a node runs a Revit element, then disable transaction, the node should not turn yellow and display warning because it's not user error.
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@ShengxiZhang @wangyangshi @saintentropy
FYIs
@QilongTang @mjkkirschner @Amoursol