Hide All context menu in workspace when displaying node context menu#12565
Conversation
| var command = new DynCmd.UpdateModelValueCommand(Guid.Empty, new Guid[] { }, "", ""); | ||
| Assert.Throws<ArgumentNullException>(() => CurrentDynamoModel.ExecuteCommand(command)); | ||
| Assert.Throws<ArgumentNullException>(() => CurrentDynamoModel.CurrentWorkspace.UpdateModelValue(command.ModelGuids, | ||
| command.Name, command.Value)); |
There was a problem hiding this comment.
@aparajit-pratap Let me know if you have a strong opinion on this. In my opinion, this should not crash Dynamo but currently, it is
There was a problem hiding this comment.
Can you remind me how these exceptions can be reproduced exactly? Looking at that old PR, I see that these exceptions are thrown when either the nodemodel is not found in the workspace or the list of nodemodels passed is null or empty, but how exactly can these situations be reproduced.
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logger.LogError(ex.Message); |
There was a problem hiding this comment.
Do we know what kinds of exceptions are these and why are they thrown?
There was a problem hiding this comment.
There is code inside the function UpdateModelValue that throw the invalid operation exception, but they are not caught anywhere. In that case, Dynamo will simply crash
| { | ||
| ShowHidePopup(ShowHideFlags.Hide, PortContextMenu); | ||
| ShowHidePopup(ShowHideFlags.Hide, NodeAutoCompleteSearchBar); | ||
| } |
There was a problem hiding this comment.
So if the sender is not the nodeview (if it's the workspace), the node level popups can still be open?
There was a problem hiding this comment.
Yes, this is the intended design from UX originally. e.g. if node autocomplete is triggered, but user clicking on canvas, the node autocomplete dialog should remain open
Yes, @aparajit-pratap You can refer to #12532, although it should be avoided by my PR but I think catching the exception would be better than just leaving it as it is. Let me know |
…12565) * Hide All context menu in workspace when displaying node context menu * Fix the crash for lacing menu * fix unit tests
3 participants
Please Note:
DynamoRevitrepo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after aLGTMlabel is added to the PR.Purpose
Per https://jira.autodesk.com/browse/DYN-4509, https://jira.autodesk.com/browse/DYN-4459 and #12532, fixing the issue that other popups are not hidden when displaying the node context menu. This fact will create all kinds of weird interactions.
UpdateModelValueImpshould not crash Dynamo. Catching the exception and logging it to Dynamo consoleDeclarations
Check these if you believe they are true
*.resxfilesRelease Notes
Hide All context menu in workspace when displaying node context menu
Reviewers
@reddyashish @zeusongit
FYIs
@johnpierson