DYN-6377: Restrict where OnNodeModified is called. #14656

twastvedt · 2023-11-29T20:18:18Z

Purpose

REVIT-215698 & https://jira.autodesk.com/browse/DYN-6377

#14122 fixed an issue where setting a dropdown's value (the SelectedIndex property) programatically didn't cause the node to recompute (OnNodeModified never called, so AST never rebuilt). However, Revit selection nodes change the node's value in BuildOutputAst (populate items and update SelectedIndex), causing an infinite loop. (It's surprising to me that it's ok to modify the node's value in BuildOutputAst. Is this something other nodes do?)

This PR moves the call to OnNodeModified up to UpdateValueCore, which is what is called when changing the node's value programatically, reducing the impact of #14122 and preventing the crash.

Declarations

Check these if you believe they are true

The codebase 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
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

Fix a crash when using Dynamo for Revit dropdown nodes.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

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

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DSDropDownBase.cs

BogdanZavu · 2023-11-29T22:02:18Z

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DSDropDownBase.cs

+
+        private void SelectionChanged(object sender, SelectionChangedEventArgs e)
+        {
+            if (comboBox.SelectedIndex != -1)


Isn't -1 actually a valid value ? Unless it is not allowed to clear the current selection here or in a subclass.

if you want to avoid introducing the field, I think the sender will be the combobox.

yeah, I need the field anyways for Dispose though.

BogdanZavu · 2023-11-29T22:22:09Z

src/Libraries/CoreNodeModels/DropDown.cs

                    selectedString = GetSelectedStringFromItem(Items.ElementAt(value));
                }

-                OnNodeModified();


Is it fair to say that an api dev should not call this function and use updatemodelvaluecommand instead ? otherwise the node will not be marked as modified. Perhaps make the set internal at some point in the future ?

Well, we need to be able to set SelectedIndex from the Revit dropdown nodes. I guess it could be protected. But maybe there are other situations where you want to be able to set the value without triggering updates?

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DSDropDownBase.cs

aparajit-pratap · 2023-11-30T16:30:57Z

src/Libraries/CoreNodeModels/DropDown.cs

                    Warning(Dynamo.Properties.Resources.NothingIsSelectedWarning);
                }

+                OnNodeModified();


Is this function called when SelectedIndex property is set to a different value?

No it's not

~~Sorry, I didn't get it then, how does moving OnNodeModified here work in that case?~~

When exactly is UpdateValueCore called in this case?

Ok, I see, by moving it out of SelectedIndex, you are avoiding chances of an infinite loop, since SelectedIndex is changed in PopulateItems, which is incidentally called here: https://github.com/DynamoDS/DynamoRevit/blob/a1c5384651133ebab113dd0abfef5bd256ef51f0/src/Libraries/RevitNodesUI/GenericClasses.cs#L194 inside the BuildOutputAst function, am I right?
I think I would agree that calling PopulateItems in BuildOutputAst is a bad idea in the first place.

Correct.
Since it's been that way for a while (7 years!), perhaps we could fix it back to working as it did before #14122 and look at improving in a separate task?

mjkkirschner · 2023-12-03T17:54:29Z

@twastvedt can a regression test be written for this or only in DSRevit?

twastvedt · 2023-12-04T14:43:24Z

@twastvedt can a regression test be written for this or only in DSRevit?

I think we don't need DSRevit. We'd just need a test nodeModel that subclasses DSDropdownBase and modifies the selected value in its BuildOutputAst implementation. I guess a stack-overflow exception in a test might cause some downstream issues? But we'll know!

Though, if as @aparajit-pratap suggests, we shouldn't really be allowing this (updating node value in BuildOutputAst) in the first place, should I just put the time for a test into a follow-up task to remove the option of doing what the DSRevit dropdowns do? (Make SelectedIndex call OnNodeModified, and find an alternative to updating the node value in DSRevit's BuildOutputAst.)

mjkkirschner · 2023-12-04T19:27:09Z

@twastvedt can a regression test be written for this or only in DSRevit?

I think we don't need DSRevit. We'd just need a test nodeModel that subclasses DSDropdownBase and modifies the selected value in its BuildOutputAst implementation. I guess a stack-overflow exception in a test might cause some downstream issues? But we'll know!

Though, if as @aparajit-pratap suggests, we shouldn't really be allowing this (updating node value in BuildOutputAst) in the first place, should I just put the time for a test into a follow-up task to remove the option of doing what the DSRevit dropdowns do? (Make SelectedIndex call OnNodeModified, and find an alternative to updating the node value in DSRevit's BuildOutputAst.)

I think it's fine to merge this as is - and file a followup for either adding a test or restricting the API somehow... though I'm not sure how that would work.
I think the idea in this PR is sound, but if you inspect the other UpdateValueCore overrides it's not clear to me that each of them will trigger OnNodeModified (it only makes sense in some cases of course) - but it would be nice to unify these or somehow communicate what updates will trigger graph executions for API clients.
Speaking of followups ... if this is the code we are shipping, I'd prefer a test sooner rather than later, followup testing tasks have a habit of never being done - so perhaps mark it higher priority?

twastvedt · 2023-12-04T21:14:50Z

Follow-up: https://jira.autodesk.com/browse/DYN-6516

* Restrict where OnNodeModified is called. * Clean up. * Dispose * No SuppressFinalize (cherry picked from commit f1f9b74)

Restrict where OnNodeModified is called.

2a3db5f

twastvedt requested review from BogdanZavu and saintentropy November 29, 2023 20:19

mjkkirschner reviewed Nov 29, 2023

View reviewed changes

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DSDropDownBase.cs Outdated Show resolved Hide resolved

twastvedt added 2 commits November 29, 2023 15:15

Clean up.

9d828da

Dispose

e958170

BogdanZavu reviewed Nov 29, 2023

View reviewed changes

mjkkirschner reviewed Nov 30, 2023

View reviewed changes

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/DSDropDownBase.cs Outdated Show resolved Hide resolved

BogdanZavu approved these changes Nov 30, 2023

View reviewed changes

No SuppressFinalize

bc7a742

aparajit-pratap reviewed Nov 30, 2023

View reviewed changes

mjkkirschner approved these changes Dec 4, 2023

View reviewed changes

twastvedt merged commit f1f9b74 into master Dec 4, 2023

twastvedt deleted the REVIT-215698-dropdown-crash branch December 4, 2023 21:15

saintentropy changed the title ~~REVIT-215698: Restrict where OnNodeModified is called.~~ DYN-6377: Restrict where OnNodeModified is called. Dec 8, 2023

saintentropy mentioned this pull request Mar 14, 2024

DYN-6518 - Cherry Picks for 2.19.5 #15019

Closed

9 tasks

DYN-6377: Restrict where OnNodeModified is called. #14656

DYN-6377: Restrict where OnNodeModified is called. #14656

Uh oh!

Conversation

twastvedt commented Nov 29, 2023 • edited by saintentropy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 3, 2023

Uh oh!

twastvedt commented Dec 4, 2023

Uh oh!

mjkkirschner commented Dec 4, 2023

Uh oh!

twastvedt commented Dec 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

twastvedt commented Nov 29, 2023 •

edited by saintentropy

Loading

aparajit-pratap Nov 30, 2023 •

edited

Loading

aparajit-pratap Nov 30, 2023 •

edited

Loading