Hide All context menu in workspace when displaying node context menu #12565

QilongTang · 2022-01-20T23:03:28Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo 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 a LGTM label 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.

Hide All other popups in workspace when displaying node context menu
Even if it fail, UpdateModelValueImp should not crash Dynamo. Catching the exception and logging it to Dynamo console

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

Release Notes

Hide All context menu in workspace when displaying node context menu

Reviewers

@reddyashish @zeusongit

FYIs

@johnpierson

QilongTang · 2022-01-21T21:38:52Z

test/DynamoCoreTests/CoreTests.cs

        }

        [Test]
        [Category("UnitTests")]
        public void UpdateModelValue_EmptyList_ThrowsException()
        {
            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));


@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

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.

aparajit-pratap · 2022-01-21T22:30:24Z

src/DynamoCore/Models/DynamoModelCommands.cs

                    command.Name, command.Value);
            }
+            catch (Exception ex)
+            {
+                Logger.LogError(ex.Message);


Do we know what kinds of exceptions are these and why are they thrown?

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

aparajit-pratap · 2022-01-21T22:41:35Z

src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs

+            {
+                ShowHidePopup(ShowHideFlags.Hide, PortContextMenu);
+                ShowHidePopup(ShowHideFlags.Hide, NodeAutoCompleteSearchBar);
+            }


So if the sender is not the nodeview (if it's the workspace), the node level popups can still be open?

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

aparajit-pratap

I think overall it's better to catch unhandled exceptions that would otherwise lead to a crash. Just one question as to steps that a user would follow in Dynamo that would lead to these crashes earlier?

QilongTang · 2022-01-21T22:56:50Z

I think overall it's better to catch unhandled exceptions that would otherwise lead to a crash. Just one question as to steps that a user would follow in Dynamo that would lead to these crashes earlier?

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

* Hide All context menu in workspace when displaying node context menu (#12565) * Hide All context menu in workspace when displaying node context menu * Fix the crash for lacing menu * fix unit tests * Update Color (#12572)

QilongTang added 2 commits January 20, 2022 18:02

Hide All context menu in workspace when displaying node context menu

9be6b68

Fix the crash for lacing menu

eb7a13f

QilongTang mentioned this pull request Jan 21, 2022

Crash report from Dynamo 2.13.0.3485 #12532

Closed

QilongTang marked this pull request as ready for review January 21, 2022 16:29

QilongTang requested review from reddyashish and zeusongit January 21, 2022 16:32

zeusongit approved these changes Jan 21, 2022

View reviewed changes

fix unit tests

7242bdb

QilongTang commented Jan 21, 2022

View reviewed changes

aparajit-pratap reviewed Jan 21, 2022

View reviewed changes

aparajit-pratap approved these changes Jan 22, 2022

View reviewed changes

QilongTang merged commit 738ab89 into RC2.13.1_master Jan 22, 2022

QilongTang deleted the HideAllPopupWhenDisplayingNodeContextMenu branch January 22, 2022 18:40

QilongTang added a commit that referenced this pull request Jan 24, 2022

Hide All context menu in workspace when displaying node context menu (#…

6554501

…12565) * Hide All context menu in workspace when displaying node context menu * Fix the crash for lacing menu * fix unit tests

QilongTang mentioned this pull request Jan 24, 2022

Cherry pick #12572 and #12565 #12575

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide All context menu in workspace when displaying node context menu #12565

Hide All context menu in workspace when displaying node context menu #12565

QilongTang commented Jan 20, 2022 •

edited

Loading

QilongTang Jan 21, 2022

aparajit-pratap Jan 21, 2022

aparajit-pratap Jan 21, 2022

QilongTang Jan 21, 2022 •

edited

Loading

aparajit-pratap Jan 21, 2022

QilongTang Jan 21, 2022 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading

QilongTang commented Jan 21, 2022

Hide All context menu in workspace when displaying node context menu #12565

Hide All context menu in workspace when displaying node context menu #12565

Conversation

QilongTang commented Jan 20, 2022 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

QilongTang Jan 21, 2022

Choose a reason for hiding this comment

aparajit-pratap Jan 21, 2022

Choose a reason for hiding this comment

aparajit-pratap Jan 21, 2022

Choose a reason for hiding this comment

QilongTang Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap Jan 21, 2022

Choose a reason for hiding this comment

QilongTang Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap left a comment • edited Loading

Choose a reason for hiding this comment

QilongTang commented Jan 21, 2022

QilongTang commented Jan 20, 2022 •

edited

Loading

QilongTang Jan 21, 2022 •

edited

Loading

QilongTang Jan 21, 2022 •

edited

Loading

aparajit-pratap left a comment •

edited

Loading