Skip to content

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

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

Merged
merged 3 commits into from
Jan 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/DynamoCore/Models/DynamoModelCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -597,12 +597,15 @@ private void UpdateModelValueImpl(UpdateModelValueCommand command)
WorkspaceModel targetWorkspace = CurrentWorkspace;
if (!command.WorkspaceGuid.Equals(Guid.Empty))
targetWorkspace = Workspaces.FirstOrDefault(w => w.Guid.Equals(command.WorkspaceGuid));

if (targetWorkspace != null)
try
{
targetWorkspace.UpdateModelValue(command.ModelGuids,
targetWorkspace?.UpdateModelValue(command.ModelGuids,
command.Name, command.Value);
}
catch (Exception ex)
{
Logger.LogError(ex.Message);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@QilongTang QilongTang Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

private void ConvertNodesToCodeImpl(ConvertNodesToCodeCommand command)
Expand Down
16 changes: 16 additions & 0 deletions src/DynamoCoreWpf/Commands/WorkspaceCommands.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public partial class WorkspaceViewModel
private DelegateCommand showHideAllGeometryPreviewCommand;
private DelegateCommand showInCanvasSearchCommand;
private DelegateCommand pasteCommand;
private DelegateCommand hideAllPopupCommand;

#endregion

Expand Down Expand Up @@ -214,6 +215,21 @@ public DelegateCommand ShowInCanvasSearchCommand
return showInCanvasSearchCommand;
}
}

/// <summary>
/// View Command to hide all popup in special cases
/// </summary>
[JsonIgnore]
public DelegateCommand HideAllPopupCommand
{
get
{
if (hideAllPopupCommand == null)
hideAllPopupCommand = new DelegateCommand(OnRequestHideAllPopup);

return hideAllPopupCommand;
}
}
#endregion

#region Properties for Command Data Binding
Expand Down
6 changes: 6 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,12 @@ private void OnRequestShowInCanvasSearch(object param)
RequestShowInCanvasSearch?.Invoke(flag);
}

internal event Action<object> RequestHideAllPopup;
private void OnRequestHideAllPopup(object param)
{
RequestHideAllPopup?.Invoke(param);
}

internal event Action<ShowHideFlags> RequestNodeAutoCompleteSearch;
internal event Action<ShowHideFlags, PortViewModel> RequestPortContextMenu;

Expand Down
8 changes: 4 additions & 4 deletions src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1005,7 +1005,7 @@ private void DynamoView_Loaded(object sender, EventArgs e)
// will not work. Instead, we have to check if the Owner Window (DynamoView) is deactivated or not.
if (Application.Current == null)
{
this.Deactivated += (s, args) => { HidePopupWhenWindowDeactivated(); };
this.Deactivated += (s, args) => { HidePopupWhenWindowDeactivated(null); };
}
loaded = true;
}
Expand All @@ -1014,11 +1014,11 @@ private void DynamoView_Loaded(object sender, EventArgs e)
/// Close Popup when the Dynamo window is not in the foreground.
/// </summary>

private void HidePopupWhenWindowDeactivated()
private void HidePopupWhenWindowDeactivated(object obj)
{
var workspace = this.ChildOfType<WorkspaceView>();
if (workspace != null)
workspace.HidePopUp();
workspace.HideAllPopUp(obj);
}

private void TrackStartupAnalytics()
Expand Down Expand Up @@ -2368,7 +2368,7 @@ private void Window_PreviewMouseLeftButtonUp(object sender, MouseButtonEventArgs
//if original sender was scroll bar(i.e Thumb) don't close the popup.
if(!(e.OriginalSource is Thumb))
{
HidePopupWhenWindowDeactivated();
HidePopupWhenWindowDeactivated(sender);
}
}

Expand Down
1 change: 1 addition & 0 deletions src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,7 @@ private void StashNodeViewCustomizationMenuItems()
private void DisplayNodeContextMenu(object sender, RoutedEventArgs e)
{
Guid nodeGuid = ViewModel.NodeModel.GUID;
ViewModel.WorkspaceViewModel.HideAllPopupCommand.Execute(sender);
ViewModel.DynamoViewModel.ExecuteCommand(
new DynCmd.SelectModelCommand(nodeGuid, Keyboard.Modifiers.AsDynamoType()));

Expand Down
22 changes: 16 additions & 6 deletions src/DynamoCoreWpf/Views/Core/WorkspaceView.xaml.cs
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,17 @@ void ViewModel_PropertyChanged(object sender, PropertyChangedEventArgs e)
private void RemoveViewModelsubscriptions(WorkspaceViewModel ViewModel)
{
ViewModel.RequestShowInCanvasSearch -= ShowHideInCanvasControl;
ViewModel.RequestHideAllPopup -= HideAllPopUp;
ViewModel.RequestNodeAutoCompleteSearch -= ShowHideNodeAutoCompleteControl;
ViewModel.RequestPortContextMenu -= ShowHidePortContextMenu;
ViewModel.DynamoViewModel.PropertyChanged -= ViewModel_PropertyChanged;

ViewModel.ZoomChanged -= vm_ZoomChanged;
ViewModel.RequestZoomToViewportCenter -= vm_ZoomAtViewportCenter;
ViewModel.RequestZoomToViewportPoint -= vm_ZoomAtViewportPoint;
ViewModel.RequestZoomToFitView -= vm_ZoomToFitView;
ViewModel.RequestCenterViewOnElement -= CenterViewOnElement;

ViewModel.RequestAddViewToOuterCanvas -= vm_RequestAddViewToOuterCanvas;
ViewModel.WorkspacePropertyEditRequested -= VmOnWorkspacePropertyEditRequested;
ViewModel.RequestSelectionBoxUpdate -= VmOnRequestSelectionBoxUpdate;
Expand All @@ -151,6 +152,7 @@ private void RemoveViewModelsubscriptions(WorkspaceViewModel ViewModel)
private void AttachViewModelsubscriptions(WorkspaceViewModel ViewModel)
{
ViewModel.RequestShowInCanvasSearch += ShowHideInCanvasControl;
ViewModel.RequestHideAllPopup += HideAllPopUp;
ViewModel.RequestNodeAutoCompleteSearch += ShowHideNodeAutoCompleteControl;
ViewModel.RequestPortContextMenu += ShowHidePortContextMenu;
ViewModel.DynamoViewModel.PropertyChanged += ViewModel_PropertyChanged;
Expand All @@ -172,7 +174,7 @@ private void AttachViewModelsubscriptions(WorkspaceViewModel ViewModel)
}

private void ShowHideNodeAutoCompleteControl(ShowHideFlags flag)
{
{
ShowHidePopup(flag, NodeAutoCompleteSearchBar);
}

Expand Down Expand Up @@ -239,7 +241,7 @@ private void ShowHidePopup(ShowHideFlags flag, Popup popup)
// If the dispatcher is not used in this scenario when switching
// from inputPort context menu to Output port context menu,
// the popup will display before the new content is fully rendered
this.Dispatcher.BeginInvoke(System.Windows.Threading.DispatcherPriority.Background, new Action(() =>{
this.Dispatcher.BeginInvoke(System.Windows.Threading.DispatcherPriority.Background, new Action(() => {
popup.Child.Visibility = Visibility.Visible;
popup.Child.UpdateLayout();
popup.IsOpen = displayPopup;
Expand All @@ -253,15 +255,23 @@ private void ShowHidePopup(ShowHideFlags flag, Popup popup)
}

/// <summary>
/// Hides Context Menu as well as InCanvasControl (Right Click PopUp)
/// Hides all popups in the view, the amount of popup hidden will be different depending on
/// if the hide view command is triggered on node level or workspace level
/// </summary>
public void HidePopUp()
public void HideAllPopUp(object sender)
{
// First make sure workspace level popups are hidden
if (InCanvasSearchBar.IsOpen || ContextMenuPopup.IsOpen)
{
ShowHideContextMenu(ShowHideFlags.Hide);
ShowHideInCanvasControl(ShowHideFlags.Hide);
}
// If triggered on node level, make sure node popups are also hidden
if(sender is NodeView && (PortContextMenu.IsOpen || NodeAutoCompleteSearchBar.IsOpen) )
{
ShowHidePopup(ShowHideFlags.Hide, PortContextMenu);
ShowHidePopup(ShowHideFlags.Hide, NodeAutoCompleteSearchBar);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@QilongTang QilongTang Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

internal Point GetCenterPoint()
Expand Down
6 changes: 4 additions & 2 deletions test/DynamoCoreTests/CoreTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -431,15 +431,17 @@ public void UpdateModelValue_MissingNode_ThrowsException()
CurrentDynamoModel.CurrentWorkspace.RemoveAndDisposeNode(addNode);

var command = new DynCmd.UpdateModelValueCommand(Guid.Empty, addNode.GUID, "Code", "");
Assert.Throws<InvalidOperationException>(() => CurrentDynamoModel.ExecuteCommand(command));
Assert.Throws<InvalidOperationException>(() => CurrentDynamoModel.CurrentWorkspace.UpdateModelValue(command.ModelGuids,
command.Name, command.Value));
}

[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));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

[Test]
Expand Down