Skip to content
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.

Sign up for GitHub

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

DYN-5018 wires disappear when moving group #12997

Merged
merged 8 commits into from
Jun 25, 2022
Merged

DYN-5018 wires disappear when moving group #12997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2e803c7
Clear inports and outports.; report nodes position;
filipeotero Jun 10, 2022
8fd6f95
merge from master
filipeotero Jun 10, 2022
90af9e0
Repositioning the collapsed groups of the wires based on the height o…
filipeotero Jun 14, 2022
57dc338
Update the proxy ports if the Height or Width of the annotation is ch…
filipeotero Jun 14, 2022
3343021
Remove from update proxy port on width and height property changed
filipeotero Jun 15, 2022
d70576d
retrigger checks
filipeotero Jun 17, 2022
9cc120c
Removing modelBase property changed
filipeotero Jun 23, 2022
eedca24
assigning comments
filipeotero Jun 23, 2022
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
67 changes: 43 additions & 24 deletions src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,11 @@ public bool IsExpanded
set
{
annotationModel.IsExpanded = value;
Copy link
Contributor

@pinzart90 pinzart90 Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As a possible improvement:
If the IsExpanded flag can be changed from other places in Dynamo ....I think would be better to listen to the model.IsExpanded OnPropertyChanged...and only keep the annotationModel.IsExpanded = value; in the setter.

annotationModel.PropertyChanged += OnModelPropertyChanged;
OnModelPropertyChanged(sender, e) {
if (e.PropertyName == "IsExpanded") {
InPorts.Clear();
OutPorts.Clear();
if (value)
{
this.ShowGroupContents();
}
else
{
this.SetGroupInputPorts();
this.SetGroupOutPorts();
this.CollapseGroupContents(true);
RaisePropertyChanged(nameof(NodeContentCount));
}
WorkspaceViewModel.HasUnsavedChanges = true;
.............the whole setter content............
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, let's do that on another PR

InPorts.Clear();
OutPorts.Clear();
if (value)
{
this.ShowGroupContents();
this.ShowGroupContents();
}
else
{
Expand All @@ -260,6 +262,15 @@ public bool IsExpanded
AddGroupToGroupCommand.RaiseCanExecuteChanged();
RaisePropertyChanged(nameof(IsExpanded));
RedrawConnectors();
ReportNodesPosition();
Copy link
Contributor

Choose a reason for hiding this comment

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

node.ReportPosition is an expensive operation when dealing with a lot of nodes under the same group.
Please check the performance with (there should be a .dyn you can use in this jira taks

Also take a look at what code paths might be calling the IsExpanded setter (to get a better idea what to test) - could be direct function calls or RaisePropertyChanges...

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not call UpdateProxyPortsPosition directly...instead of having to go through the nodeModel's position ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is for when the group is uncollapsed and the annotation inside it needs to report the position. Otherwise, that's what happens:
annotationNode

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no performance penalty in doing "node.ReportPosition" then it is fine.
The fix would be to make the Connectors redraw when the corresponding NodeModels(start or end) change their IsProxyPort flag
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoCoreWpf/ViewModels/Core/ConnectorViewModel.cs#L1209

}
}

private void ReportNodesPosition()
{
foreach (var node in Nodes.OfType<AnnotationModel>())
{
node.ReportPosition();
}
}

Expand Down Expand Up @@ -536,11 +547,13 @@ private bool CanChangeFontSize(object obj)
public AnnotationViewModel(WorkspaceViewModel workspaceViewModel, AnnotationModel model)
{
annotationModel = model;

this.WorkspaceViewModel = workspaceViewModel;
this.preferenceSettings = WorkspaceViewModel.DynamoViewModel.PreferenceSettings;
model.PropertyChanged += model_PropertyChanged;
model.RemovedFromGroup += OnModelRemovedFromGroup;
model.AddedToGroup += OnModelAddedToGroup;

DynamoSelection.Instance.Selection.CollectionChanged += SelectionOnCollectionChanged;

//https://jira.autodesk.com/browse/QNTM-3770
Expand Down Expand Up @@ -581,14 +594,15 @@ public AnnotationViewModel(WorkspaceViewModel workspaceViewModel, AnnotationMode
LoadGroupStylesFromPreferences(preferenceSettings.GroupStyleItemsList);
}


/// <summary>
/// Creates input ports for the group based on its Nodes.
/// Input ports that either is connected to a Node outside of the
/// group, or has a port that is not connected will be used for the group.
/// This function appends to the inputs
/// </summary>
internal void SetGroupInputPorts()
private void SetGroupInputPorts()
{
InPorts.Clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove InPorts.Clear(); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here

List<PortViewModel> newPortViewModels;

// we need to store the original ports here
Expand Down Expand Up @@ -622,10 +636,10 @@ internal void SetGroupInputPorts()
/// <summary>
/// Creates output ports for the group based on its Nodes.
/// Output ports that are not connected will be used for the group.
/// This function appends to the outports
/// </summary>
internal void SetGroupOutPorts()
private void SetGroupOutPorts()
{
OutPorts.Clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove OutPorts.Clear(); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because now it is called at the beginning of the isExpanded property.

Copy link
Contributor

Choose a reason for hiding this comment

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

so now SetGroupInputPorts and SetGroupOutPorts will always append ports to OutPorts and InPorts. I hope nobody uses these functions expecting the port arrays to be re-set ...as the func names suggest. Can you make these functions private and maybe add to the description that they always append to the respective port arrays ?

Copy link
Contributor Author

@filipeotero filipeotero Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, they were edited.

List<PortViewModel> newPortViewModels;

// we need to store the original ports here
Expand Down Expand Up @@ -716,13 +730,10 @@ private double GetPortVerticalOffset(PortModel portModel, int proxyPortIndex)
return verticalOffset + (proxyPortIndex * portHeight) + portVerticalMidPoint;
}

private Point2D CalculatePortPosition(PortModel portModel, int proxyPortIndex)
private Point2D CalculatePortPosition(PortModel portModel, double verticalPosition)
{
double groupHeaderHeight = Height - ModelAreaRect.Height;

double offset = GetPortVerticalOffset(portModel, proxyPortIndex);
double y = Top + groupHeaderHeight + offset;

double y = Top + groupHeaderHeight + verticalPosition + verticalOffset + portVerticalMidPoint;
switch (portModel.PortType)
{
case PortType.Input:
Expand All @@ -731,7 +742,7 @@ private Point2D CalculatePortPosition(PortModel portModel, int proxyPortIndex)
if (portModel.Owner is CodeBlockNodeModel)
{
// Special case because code block outputs are smaller than regular outputs.
return new Point2D(Left + Width, y + 6);
return new Point2D(Left + Width, y - 8);
}
return new Point2D(Left + Width, y);
}
Expand All @@ -746,15 +757,19 @@ private List<PortViewModel> CreateProxyInPorts(IEnumerable<PortModel> groupPortM
.ToList();

var newPortViewModels = new List<PortViewModel>();
for (int i = 0; i < groupPortModels.Count(); i++)
double verticalPosition = 0;
foreach (var groupPort in groupPortModels)
{
var model = groupPortModels.ElementAt(i);
newPortViewModels.Add(originalPortViewModels[i].CreateProxyPortViewModel(model));

// calculate new position for the proxy inports.
model.Center = CalculatePortPosition(model, i);
var originalPort = originalPortViewModels.FirstOrDefault(x => x.PortModel.GUID == groupPort.GUID);
if (originalPort != null)
{
var portViewModel = originalPort.CreateProxyPortViewModel(groupPort);
newPortViewModels.Add(portViewModel);
// calculate new position for the proxy outports
groupPort.Center = CalculatePortPosition(groupPort, verticalPosition);
verticalPosition += originalPort.Height;
}
}

return newPortViewModels;
}

Expand All @@ -766,7 +781,7 @@ private List<PortViewModel> CreateProxyOutPorts(IEnumerable<PortModel> groupPort
.ToList();

var newPortViewModels = new List<PortViewModel>();
int index = 0;
double verticalPosition = 0;
foreach (var group in groupPortModels)
{
var originalPort = originalPortViewModels.FirstOrDefault(x => x.PortModel.GUID == group.GUID);
Expand All @@ -775,9 +790,9 @@ private List<PortViewModel> CreateProxyOutPorts(IEnumerable<PortModel> groupPort
var portViewModel = originalPort.CreateProxyPortViewModel(group);
newPortViewModels.Add(portViewModel);
// calculate new position for the proxy outports
group.Center = CalculatePortPosition(group, index);
group.Center = CalculatePortPosition(group, verticalPosition);
verticalPosition += originalPort.Height;
}
index++;
}

return newPortViewModels;
Expand All @@ -790,23 +805,28 @@ internal void UpdateProxyPortsPosition()

if (parent != null && !parent.IsExpanded) return;

double verticalPosition = 0;

for (int i = 0; i < inPorts.Count(); i++)
{
var model = inPorts[i]?.PortModel;
if (model != null && model.IsProxyPort)
{
// calculate new position for the proxy inports.
model.Center = CalculatePortPosition(model, i);
model.Center = CalculatePortPosition(model, verticalPosition);
verticalPosition += model.Height;
}
}

verticalPosition = 0;
for (int i = 0; i < outPorts.Count(); i++)
{
var model = outPorts[i]?.PortModel;
if (model != null && model.IsProxyPort)
{
// calculate new position for the proxy outports.
model.Center = CalculatePortPosition(model, i);
model.Center = CalculatePortPosition(model, verticalPosition);
verticalPosition += model.Height;
}
}
}
Expand Down Expand Up @@ -898,7 +918,6 @@ private void RedrawConnectors()

foreach (var connector in allNodes.SelectMany(x=>x.AllConnectors))
{

var connectorViewModel = WorkspaceViewModel
.Connectors
.Where(x => connector.GUID == x.ConnectorModel.GUID)
Expand Down
2 changes: 2 additions & 0 deletions src/DynamoCoreWpf/Views/Core/AnnotationView.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,8 @@ private void GroupTextBlock_SizeChanged(object sender, SizeChangedEventArgs e)
SetTextMaxWidth();
SetTextHeight();
}

ViewModel.UpdateProxyPortsPosition();
}


Expand Down