DYN-5018 wires disappear when moving group #12997

filipeotero · 2022-06-14T14:05:38Z

Purpose

In this PR, the position of the wires is calculated based on the height of the port that is created after the group is collapsed.
Previously, we were assuming that all the ports have the same height, causing a mispositioning of the wires.

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

Reviewers

@QilongTang @pinzart @reddyashish

…f the ports

pinzart90 · 2022-06-14T14:24:28Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

@@ -260,6 +262,15 @@ public bool IsExpanded
                AddGroupToGroupCommand.RaiseCanExecuteChanged();
                RaisePropertyChanged(nameof(IsExpanded));
                RedrawConnectors();
+                ReportNodesPosition();


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...

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

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

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

mjkkirschner · 2022-06-14T14:33:09Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

-
-                // calculate new position for the proxy inports.
-                model.Center = CalculatePortPosition(model, i);
+                var originalPort = originalPortViewModels.FirstOrDefault(x => x.PortModel.GUID == group.GUID);


I'm, confused by this, but not super familiar with this code, when would the guid of a group equal the guid of a port model?

It's the group's port. I will change the variable to groupPort to avoid confusion. Thanks.

…anged

pinzart90 · 2022-06-14T17:47:44Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

@@ -245,9 +245,11 @@ public bool IsExpanded
            set
            {
                annotationModel.IsExpanded = value;


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............
}
}

Ok, let's do that on another PR

pinzart90 · 2022-06-14T17:51:25Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

@@ -1081,10 +1097,12 @@ private void model_PropertyChanged(object sender, System.ComponentModel.Property
                    RaisePropertyChanged("Width");
                    RaisePropertyChanged(nameof(ModelAreaRect));
                    UpdateAllGroupedGroups();
+                    UpdateProxyPortsPosition();


check performance here...
When moving a group (drag with mouse) ...these properties can get changed a lot of times

Ok, I will check an alternative for this.

check if performance is an issue first...then check alternatives
No use in doing premature optimization

filipeotero · 2022-06-15T21:13:33Z

Instead of calling the UpdateProxyPortsPosition method every time the width changes, the method is being called on GroupTextBox_OnTextChanged which was updating of the width of the group, but not the center of its ports.

pinzart90 · 2022-06-17T14:41:44Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

@@ -536,11 +547,15 @@ private bool CanChangeFontSize(object obj)
        public AnnotationViewModel(WorkspaceViewModel workspaceViewModel, AnnotationModel model)
        {
            annotationModel = model;
+            var modelBase = (ModelBase)model;


why do you need to cast it to ModelBase ?

pinzart90 · 2022-06-17T14:42:20Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

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


there already is a handler for the model PropertyChange event

model.PropertyChanged += model_PropertyChanged;

pinzart90 · 2022-06-17T14:43:13Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

+            switch (e.PropertyName)
+            {
+                case nameof(annotationModel.Position):
+                    UpdateProxyPortsPosition();


You should be able to move this to model_PropertyChanged

pinzart90 · 2022-06-17T14:45:06Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

@@ -625,7 +649,6 @@ internal void SetGroupInputPorts()
        /// </summary>
        internal void SetGroupOutPorts()
        {
-            OutPorts.Clear();


why did you remove OutPorts.Clear(); ?

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

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 ?

Sure, they were edited.

pinzart90 · 2022-06-17T14:45:18Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

        internal void SetGroupInputPorts()
        {
-            InPorts.Clear();


why did you remove InPorts.Clear(); ?

pinzart90 · 2022-06-23T15:01:52Z

src/DynamoCoreWpf/ViewModels/Core/AnnotationViewModel.cs

+            /// 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.
+            /// </summary>


looks like the formatting got messed up

filipeotero added 3 commits June 10, 2022 17:44

Clear inports and outports.; report nodes position;

2e803c7

merge from master

8fd6f95

Repositioning the collapsed groups of the wires based on the height o…

90af9e0

…f the ports

pinzart90 reviewed Jun 14, 2022

View reviewed changes

mjkkirschner reviewed Jun 14, 2022

View reviewed changes

Update the proxy ports if the Height or Width of the annotation is ch…

57dc338

…anged

pinzart90 reviewed Jun 14, 2022

View reviewed changes

Remove from update proxy port on width and height property changed

3343021

retrigger checks

d70576d

pinzart90 reviewed Jun 17, 2022

View reviewed changes

Removing modelBase property changed

9cc120c

pinzart90 reviewed Jun 23, 2022

View reviewed changes

assigning comments

eedca24

pinzart90 approved these changes Jun 24, 2022

View reviewed changes

QilongTang approved these changes Jun 25, 2022

View reviewed changes

QilongTang merged commit 0174e38 into DynamoDS:master Jun 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-5018 wires disappear when moving group #12997

DYN-5018 wires disappear when moving group #12997

filipeotero commented Jun 14, 2022 •

edited

Loading

pinzart90 Jun 14, 2022

pinzart90 Jun 14, 2022

filipeotero Jun 14, 2022

pinzart90 Jun 15, 2022

mjkkirschner Jun 14, 2022 •

edited

Loading

filipeotero Jun 14, 2022

pinzart90 Jun 14, 2022 •

edited

Loading

filipeotero Jun 14, 2022

pinzart90 Jun 14, 2022

filipeotero Jun 14, 2022

pinzart90 Jun 15, 2022

filipeotero commented Jun 15, 2022

pinzart90 Jun 17, 2022

filipeotero Jun 23, 2022

pinzart90 Jun 17, 2022

filipeotero Jun 23, 2022

pinzart90 Jun 17, 2022

filipeotero Jun 23, 2022

pinzart90 Jun 17, 2022

filipeotero Jun 23, 2022

pinzart90 Jun 23, 2022

filipeotero Jun 23, 2022 •

edited

Loading

pinzart90 Jun 17, 2022

filipeotero Jun 23, 2022

pinzart90 Jun 23, 2022 •

edited

Loading

DYN-5018 wires disappear when moving group #12997

DYN-5018 wires disappear when moving group #12997

Conversation

filipeotero commented Jun 14, 2022 • edited Loading

Purpose

Declarations

Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 Jun 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipeotero commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipeotero Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinzart90 Jun 23, 2022 • edited Loading

Choose a reason for hiding this comment

filipeotero commented Jun 14, 2022 •

edited

Loading

mjkkirschner Jun 14, 2022 •

edited

Loading

pinzart90 Jun 14, 2022 •

edited

Loading

filipeotero Jun 23, 2022 •

edited

Loading

pinzart90 Jun 23, 2022 •

edited

Loading