diff --git a/src/DynamoCore/Graph/Annotations/AnnotationModel.cs b/src/DynamoCore/Graph/Annotations/AnnotationModel.cs index 6abfc66ec47..f6dea824e01 100644 --- a/src/DynamoCore/Graph/Annotations/AnnotationModel.cs +++ b/src/DynamoCore/Graph/Annotations/AnnotationModel.cs @@ -171,53 +171,15 @@ public string Background } } - private HashSet nodes; + private Dictionary nodes; + /// /// Returns collection of models (nodes and notes) which the group contains /// public IEnumerable Nodes { - get { return nodes; } - set - { - // Unsubscribe all content in group before - // overwriting with the new content. - // If we dont do this we end up with - // lots of memory leaks that eventually will - // lead to a stackoverflow exception - if (nodes != null && nodes.Any()) - { - foreach (var model in nodes) - { - model.PropertyChanged -= model_PropertyChanged; - model.Disposed -= model_Disposed; - } - } - - // First separate all pins from the input - var pinModels = value.OfType().ToList(); - var valuesWithoutPins = value.Except(pinModels); - - // Then recalculate which pins belong to the group based on the nodes - var pinsFromNodes = GetPinsFromNodes(valuesWithoutPins.OfType()); - - // Then filter out pins that have been marked as removed - pinsFromNodes = pinsFromNodes.Where(p => !removedPins.Contains(p.GUID)).ToArray(); - - // Combine all - nodes = valuesWithoutPins.Concat(pinModels).Concat(pinsFromNodes).ToHashSet(); - - if (nodes != null && nodes.Any()) - { - foreach (var model in nodes) - { - model.PropertyChanged += model_PropertyChanged; - model.Disposed += model_Disposed; - } - } - UpdateBoundaryFromSelection(); - RaisePropertyChanged(nameof(Nodes)); - } + get { return nodes.Values; } + set { ReplaceNodes(value); } } /// @@ -400,7 +362,7 @@ internal set /// /// Checks if this group contains any nested groups. /// - public bool HasNestedGroups => nodes.OfType().Any(); + public bool HasNestedGroups => nodes.Values.OfType().Any(); private bool isVisible = true; /// @@ -439,36 +401,52 @@ internal set /// The groups that belongs to this group public AnnotationModel(IEnumerable nodes, IEnumerable notes, IEnumerable groups) { - var nodeModels = nodes as NodeModel[] ?? nodes.ToArray(); - var noteModels = notes as NoteModel[] ?? notes.ToArray(); - var groupModels = groups as AnnotationModel[] ?? groups.ToArray(); - DeletedModelBases = new List(); - this.Nodes = nodeModels - .Concat(noteModels.Cast()) - .Concat(groupModels.Cast()) - .ToList(); + ReplaceNodes(nodes.Cast().Concat(notes).Concat(groups)); UpdateBoundaryFromSelection(); UpdateErrorAndWarningIconVisibility(); } - private ConnectorPinModel[] GetPinsFromNodes(IEnumerable nodeModels) + private void ReplaceNodes(IEnumerable newNodes) { - if (nodeModels is null || - !nodeModels.Any()) + // Unsubscribe all content in group before + // overwriting with the new content. + // If we dont do this we end up with + // lots of memory leaks that eventually will + // lead to a stackoverflow exception + if (nodes?.Count > 0) { - return new List().ToArray(); + foreach (var model in nodes.Values) + { + model.PropertyChanged -= model_PropertyChanged; + model.Disposed -= model_Disposed; + } } - var connectorPinsToAdd = nodeModels + // Get the pins from the node models (this will exclude all ConnectorPinModels) + var uniqueNodes = newNodes.OfType().ToHashSet(); + + // Then recalculate which pins belong to the group based on the nodes, + // then filter out pins that have been marked as removed + var pinsFromNodes = uniqueNodes .SelectMany(x => x.AllConnectors) - .Where(x => nodeModels.Contains(x.Start.Owner) && nodeModels.Contains(x.End.Owner)) + .Where(x => uniqueNodes.Contains(x.Start.Owner) && uniqueNodes.Contains(x.End.Owner)) .SelectMany(x => x.ConnectorPinModels) .Distinct() - .ToArray(); + .Where(p => !removedPins.Contains(p.GUID)); + + // Combine all (including all ConnectorPinModels) + nodes = newNodes.Concat(pinsFromNodes).ToDictionary(m => m.GUID); + foreach (var model in nodes.Values) + { + model.PropertyChanged += model_PropertyChanged; + model.Disposed += model_Disposed; + } - return connectorPinsToAdd; + UpdateBoundaryFromSelection(); + + RaisePropertyChanged(nameof(Nodes)); } /// @@ -528,12 +506,10 @@ private void model_Disposed(ModelBase model) /// internal void UpdateBoundaryFromSelection() { - var selectedModelsList = nodes.ToList(); + var groupModels = nodes.Values.OrderBy(x => x.X).ToList(); - if (selectedModelsList.Any()) + if (groupModels.Count > 0) { - var groupModels = selectedModelsList.OrderBy(x => x.X).ToList(); - //Shifting x by 10 and y to the height of textblock var regionX = groupModels.Min(x => x.X) - ExtendSize; //Increase the Y value by 10. This provides the extra space between @@ -604,31 +580,29 @@ private void UpdateErrorAndWarningIconVisibility() return; } - List nodes = Nodes - .OfType() - .ToList(); - - List groups = Nodes - .OfType() - .ToList(); - - // If anything in this group is in an error state, we display an error icon. - if (nodes.Any(x => x.State == ElementState.Error) || - groups.Any(x => x.GroupState == ElementState.Error)) + var hasWarning = false; + foreach(var item in Nodes) { - GroupState = ElementState.Error; - return; - } + var state = item is NodeModel node ? node.State + : item is AnnotationModel annotation ? annotation.groupState + : ElementState.Active; - // If anything in this group is in a warning state, we display a warning icon. - if (nodes.Any(x => x.State == ElementState.Warning) || - groups.Any(x => x.GroupState == ElementState.Warning)) - { - GroupState = ElementState.Warning; - return; + if (state == ElementState.Error) + { + // If anything in this group is in an error state, we display an error icon. + // Since this takes precedence over warning, we can safely exit here + GroupState = ElementState.Error; + return; + } + else if (state == ElementState.Warning) + { + // Set warning flag, don't break out of loop in case a future item is in an error state + hasWarning = true; + } } - GroupState = ElementState.Active; + // If anything in this group is in a warning state, we display a warning icon. + GroupState = hasWarning ? ElementState.Warning : ElementState.Active; } /// @@ -759,7 +733,7 @@ protected override void DeserializeCore(XmlElement element, SaveContext context) var result = mhelper.ReadGuid("ModelGuid", new Guid()); var model = ModelBaseRequested != null ? ModelBaseRequested(result) - : Nodes.FirstOrDefault(x => x.GUID == result); + : (nodes.TryGetValue(result, out var n) ? n : null); if (model != null) { @@ -796,14 +770,38 @@ protected override void DeserializeCore(XmlElement element, SaveContext context) /// completely inside that group internal void AddToTargetAnnotationModel(ModelBase model, bool checkOverlap = false) { - var list = this.Nodes.ToList(); - if (model.GUID == this.GUID) return; - if (list.Where(x => x.GUID == model.GUID).Any()) return; - if (!CheckModelIsInsideGroup(model, checkOverlap)) return; - list.Add(model); - this.Nodes = list; - if (model is AnnotationModel annotationModel) annotationModel.OnAddedToGroup(); - this.UpdateBoundaryFromSelection(); + AddToTargetAnnotationModel([model], checkOverlap); + } + + /// + /// This is called when models are added to or deleted from a group. + /// + /// The models to add. + /// checkoverlap determines whether the selected model is + /// completely inside that group + internal void AddToTargetAnnotationModel(IEnumerable models, bool checkOverlap = false) + { + var added = new List(); + foreach(var model in models) + { + // skip adding group to itself or any already existing + if (model.GUID == GUID || nodes.ContainsKey(model.GUID)) continue; + // skip adding if checking for overlap when no overlap + if (checkOverlap && !Rect.Contains(model.Rect)) continue; + + added.Add(model); + } + + if (added.Count == 0) return; + + ReplaceNodes(Nodes.Concat(added).ToList()); + + foreach(var addedAnnotation in added.OfType()) + { + addedAnnotation.OnAddedToGroup(); + } + + UpdateBoundaryFromSelection(); UpdateErrorAndWarningIconVisibility(); } @@ -817,17 +815,6 @@ private void UnsubscribeRemovedModel(ModelBase model) } } - private bool CheckModelIsInsideGroup(ModelBase model, bool checkOverlap) - { - if (!checkOverlap) return true; - var modelRect = model.Rect; - if (this.Rect.Contains(modelRect)) - { - return true; - } - return false; - } - #endregion /// @@ -871,16 +858,23 @@ public override void Deselect() } /// - /// Checks if the provided modelbase belongs - /// to this group. + /// Checks if the provided modelbase belongs to this group. /// /// modelbase to check if belongs to this group - /// public bool ContainsModel(ModelBase modelBase) { if (modelBase is null) return false; - return nodes.Contains(modelBase); + return nodes.TryGetValue(modelBase.GUID, out var current) && modelBase == current; + } + + /// + /// Checks if the provided modelbase guid belongs to this group. + /// + /// modelbase guid to check if belongs to this group + internal bool ContainsGuid(Guid modelBaseGuid) + { + return nodes.ContainsKey(modelBaseGuid); } /// diff --git a/src/DynamoCore/Graph/Workspaces/LayoutExtensions.cs b/src/DynamoCore/Graph/Workspaces/LayoutExtensions.cs index 92c661b7dd3..1b451df6768 100644 --- a/src/DynamoCore/Graph/Workspaces/LayoutExtensions.cs +++ b/src/DynamoCore/Graph/Workspaces/LayoutExtensions.cs @@ -192,7 +192,7 @@ private static void GenerateCombinedGraph(this WorkspaceModel workspace, bool is } AnnotationModel group = workspace.Annotations - .Where(g => g.Nodes.Contains(note)) + .Where(g => g.ContainsModel(note)) .FirstOrDefault(); GraphLayout.Node nd = null; diff --git a/src/DynamoCore/Models/DynamoModel.cs b/src/DynamoCore/Models/DynamoModel.cs index 1e45473b1a9..c6cd8b62475 100644 --- a/src/DynamoCore/Models/DynamoModel.cs +++ b/src/DynamoCore/Models/DynamoModel.cs @@ -2715,8 +2715,7 @@ internal void DeleteModelInternal(List modelsToDelete) //If there is only one model, then deleting that model should delete the group. In that case, do not record //the group for modification. Until we have one model in a group, group should be recorded for modification //otherwise, undo operation cannot get the group back. - if (annotation.Nodes.Count() > 1 && - annotation.Nodes.Where(x => x.GUID == model.GUID).Any()) + if (annotation.Nodes.Count() > 1 && annotation.ContainsModel(model)) { CurrentWorkspace.RecordGroupModelBeforeUngroup(annotation); } @@ -2759,7 +2758,7 @@ internal void UngroupModel(List modelsToUngroup) { foreach (var annotation in annotations) { - if (annotation.Nodes.Any(x => x.GUID == model.GUID)) + if (annotation.ContainsModel(model)) { var list = annotation.Nodes.ToList(); @@ -2773,7 +2772,6 @@ internal void UngroupModel(List modelsToUngroup) annotation.MarkPinAsRemoved(pinModel); } annotation.Nodes = list; - annotation.UpdateBoundaryFromSelection(); } } else @@ -2805,13 +2803,9 @@ internal void AddToGroup(List modelsToAdd) if (selectedGroup != null) { - foreach (var model in modelsToAdd) - { - CurrentWorkspace.RecordGroupModelBeforeUngroup(selectedGroup); - selectedGroup.AddToTargetAnnotationModel(model); - } + CurrentWorkspace.RecordGroupModelBeforeUngroup(selectedGroup); + selectedGroup.AddToTargetAnnotationModel(modelsToAdd); } - } /// @@ -2831,10 +2825,8 @@ internal void AddGroupsToGroup(List modelsToAdd, Guid hostGroupGuid) // Mark the parent group and groups to add all for undo recorder WorkspaceModel.RecordModelsForModification(modelsToModify, CurrentWorkspace.UndoRecorder); - foreach (var model in modelsToAdd) - { - selectedGroup.AddToTargetAnnotationModel(model); - } + + selectedGroup.AddToTargetAnnotationModel(modelsToAdd); } internal void DumpLibraryToXml(object parameter) @@ -3243,7 +3235,7 @@ from c in connectors var nestedGroup = CreateAnnotationModel( group, modelLookup - .Where(x => group.Nodes.Select(y => y.GUID).Contains(x.Key)) + .Where(x => group.ContainsModel(x.Value)) .ToDictionary(x => x.Key, x => x.Value) ); diff --git a/src/DynamoCoreWpf/ViewModels/Core/AnnotationExtension.cs b/src/DynamoCoreWpf/ViewModels/Core/AnnotationExtension.cs index f32f0014ce5..daacc902f8d 100644 --- a/src/DynamoCoreWpf/ViewModels/Core/AnnotationExtension.cs +++ b/src/DynamoCoreWpf/ViewModels/Core/AnnotationExtension.cs @@ -12,7 +12,7 @@ internal static class AnnotationExtensions { public static bool ContainsModel(this IEnumerable groups, Guid nodeGuid) { - return (groups.SelectMany(m => m.Nodes).Any(m => m.GUID == nodeGuid)); + return groups.Any(g => g.ContainsGuid(nodeGuid)); } } } diff --git a/src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs b/src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs index 6d8f87c6fee..c6d28c9aa23 100644 --- a/src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs +++ b/src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs @@ -469,14 +469,9 @@ private void AddToGroup(object obj) { if (annotationModel.IsSelected) { - var selectedModels = DynamoSelection.Instance.Selection.OfType(); - foreach (var model in selectedModels) - { - if (!(model is AnnotationModel)) - { - this.AnnotationModel.AddToTargetAnnotationModel(model, true); - } - } + var selectedModels = DynamoSelection.Instance.Selection.OfType().Where(m => !(m is AnnotationModel)); + AnnotationModel.AddToTargetAnnotationModel(selectedModels, true); + Analytics.TrackEvent(Actions.AddedTo, Categories.GroupOperations, "Node"); } } @@ -625,7 +620,7 @@ public AnnotationViewModel(WorkspaceViewModel workspaceViewModel, AnnotationMode //https://jira.autodesk.com/browse/QNTM-3770 //Notes and Groups are serialized as annotations. Do not unselect the node selection during //Notes serialization - if (model.Nodes.Count() > 0) + if (model.Nodes.Any()) { // Group is created already.So just populate it. var selectNothing = new DynamoModel.SelectModelCommand(Guid.Empty, System.Windows.Input.ModifierKeys.None.AsDynamoType()); diff --git a/src/DynamoCoreWpf/ViewModels/Core/ConnectorPinViewModel.cs b/src/DynamoCoreWpf/ViewModels/Core/ConnectorPinViewModel.cs index 7c2919c0a0d..bd4ebba1f30 100644 --- a/src/DynamoCoreWpf/ViewModels/Core/ConnectorPinViewModel.cs +++ b/src/DynamoCoreWpf/ViewModels/Core/ConnectorPinViewModel.cs @@ -384,10 +384,10 @@ private void AddPinToGroupIfConnectedNodesInSameGroup() var startNode = connector.Start.Owner; var endNode = connector.End.Owner; - foreach (var group in groups) + foreach (var group in workspace.Annotations) { // Check if both nodes (start and end) are part of the same group - if (group.Nodes.Contains(startNode) && group.Nodes.Contains(endNode)) + if (group.ContainsModel(startNode) && group.ContainsModel(endNode)) { // If both nodes are part of the same group, add the pin to that group group.AddToTargetAnnotationModel(model); diff --git a/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs b/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs index 209a89ea37b..72247d1a335 100644 --- a/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs +++ b/src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs @@ -1173,14 +1173,14 @@ private bool IsModelInCollapsedGroup(ModelBase model) // Check all the collapsed groups and their sub groups foreach (var group in Model.Annotations.Where(x => !x.IsExpanded)) { - if (group.Nodes.Contains(model)) + if (group.ContainsModel(model)) { IsInCollapsedGroup = true; break; } foreach (var nestGroup in group.Nodes.OfType()) { - if (nestGroup.Nodes.Contains(model)) + if (nestGroup.ContainsModel(model)) { IsInCollapsedGroup = true; break;