Keep the revit element with Node when Disable Transaction#2761
Keep the revit element with Node when Disable Transaction#2761ZiyunShang wants to merge 4 commits intoDynamoDS:masterfrom
Conversation
| { | ||
| 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.
what case triggered this change? Could index ever be -1? (not found?)
There was a problem hiding this comment.
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.
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.
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
*.resxfilesReviewers
@ShengxiZhang @wangyangshi @saintentropy
FYIs
@QilongTang @mjkkirschner @Amoursol