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

Fixes and improvements to workflow sort order #1414

Merged
merged 9 commits into from
Jun 13, 2023
Merged

Fixes and improvements to workflow sort order #1414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7d7ded4
Add regression test for disconnecting graph nodes
glopesdev Jun 13, 2023
9df06bc
Fix implementation argument reference
glopesdev Jun 13, 2023
bfcf151
Insert disconnected roots at the end of component
glopesdev Jun 13, 2023
91fc8ab
Add regression test for inserting annotation node
glopesdev Jun 13, 2023
3bd78d4
Create property source as successor
glopesdev Jun 13, 2023
696b914
Refactor related overloads for consistency
glopesdev Jun 13, 2023
82daed2
Reorder method declaration order for readability
glopesdev Jun 13, 2023
075f531
Add node to end of workflow if no target specified
glopesdev Jun 13, 2023
5a90afb
Ensure sort is reversible when adding a new node
glopesdev Jun 13, 2023
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
6 changes: 6 additions & 0 deletions Bonsai.Editor.Tests/EditorHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ internal static GraphNode FindNode(this WorkflowEditor editor, string name)
return editor.FindGraphNode(node.Value);
}

internal static GraphNode FindNode(this WorkflowEditor editor, ExpressionBuilder builder)
{
var node = editor.Workflow.First(n => ExpressionBuilder.Unwrap(n.Value) == builder);
return editor.FindGraphNode(node.Value);
}

internal static void ConnectNodes(this WorkflowEditor editor, string source, string target)
{
var sourceNodes = new[] { editor.FindNode(source) };
Expand Down
27 changes: 27 additions & 0 deletions Bonsai.Editor.Tests/WorkflowEditorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,33 @@ public void UngroupGraphNodes_NestedPassthrough_KeepOuterEdges()
editor.UngroupGraphNodes(nodesToUngroup);
assertIsReversible();
}

[TestMethod]
public void DisconnectGraphNodes_TargetIsNotRoot_InsertAfterClosestRoot()
{
var workflow = EditorHelper.CreateEditorGraph("A", "B", "C", "D");
var (editor, assertIsReversible) = CreateMockEditor(workflow);
editor.ConnectNodes("A", "C");
editor.ConnectNodes("B", "C");
editor.ConnectNodes("C", "D");
var sourceNode = editor.FindNode("B");
var targetNode = editor.FindNode("C");
Assert.AreEqual(expected: editor.Workflow.Count - 1, editor.FindNode("D").Index);
editor.DisconnectGraphNodes(new[] { sourceNode }, targetNode);
Assert.AreEqual(expected: editor.Workflow.Count - 1, editor.FindNode("B").Index);
assertIsReversible();
}

[TestMethod]
public void CreateAnnotation_EmptySelection_InsertAfterClosestRoot()
{
var workflow = EditorHelper.CreateEditorGraph("A");
var (editor, assertIsReversible) = CreateMockEditor(workflow);
var annotationBuilder = new AnnotationBuilder();
editor.CreateGraphNode(annotationBuilder, null, CreateGraphNodeType.Successor, branch: false);
Assert.AreEqual(expected: editor.Workflow.Count - 1, editor.FindNode(annotationBuilder).Index);
assertIsReversible();
}
}

static class WorkflowEditorHelper
Expand Down
106 changes: 61 additions & 45 deletions Bonsai.Editor/GraphModel/WorkflowEditor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ private int GetInsertIndex(
Node<ExpressionBuilder, ExpressionBuilderArgument> target,
CreateGraphNodeType nodeType)
{
var insertOffset = target == null || nodeType == CreateGraphNodeType.Successor ? 1 : 0;
if (target == null) return workflow.Count;
var insertOffset = nodeType == CreateGraphNodeType.Successor ? 1 : 0;
return workflow.IndexOf(target) + insertOffset;
}

Expand Down Expand Up @@ -198,7 +199,7 @@ private void SortWorkflow()

private void SortWorkflow(ExpressionBuilderGraph workflow)
{
Workflow.InsertRange(0, Workflow.TopologicalSort());
workflow.InsertRange(0, workflow.TopologicalSort());
}

private GraphCommand GetSimpleSort()
Expand Down Expand Up @@ -688,7 +689,9 @@ where siblingEdge.Item2.Label.Index.CompareTo(edgeIndex) > 0
}

GraphCommand reorder;
var targetIndex = workflow.IndexOf(target) + 1;
var components = workflow.FindConnectedComponents();
var targetComponent = components.First(component => component.Contains(target));
var targetIndex = LastIndexOfComponentNode(workflow, targetComponent) + 1;
if (sinkNodes != null)
{
reorder = GetReversibleSort();
Expand Down Expand Up @@ -962,12 +965,56 @@ static string MakeGenericType(string typeName, ExpressionBuilder builder, out El
return genericType.MakeGenericType(inspectBuilder.ObservableType).AssemblyQualifiedName;
}

public void InsertGraphNode(string typeName, ElementCategory elementCategory, CreateGraphNodeType nodeType, bool branch, bool group)
private void ConfigureBuilder(ExpressionBuilder builder, GraphNode selectedNode, string arguments)
{
InsertGraphNode(typeName, elementCategory, nodeType, branch, group, null);
if (string.IsNullOrEmpty(arguments)) return;
// TODO: This special case for binary operator operands should be avoided in the future
if (builder is BinaryOperatorBuilder binaryOperator && selectedNode != null)
{
if (GetGraphNodeTag(selectedNode).Value is InspectBuilder inputBuilder &&
inputBuilder.ObservableType != null)
{
binaryOperator.Build(Expression.Parameter(typeof(IObservable<>).MakeGenericType(inputBuilder.ObservableType)));
}
}

var workflowElement = ExpressionBuilder.GetWorkflowElement(builder);
var defaultProperty = TypeDescriptor.GetDefaultProperty(workflowElement);
if (defaultProperty != null &&
!defaultProperty.IsReadOnly &&
defaultProperty.Converter != null &&
defaultProperty.Converter.CanConvertFrom(typeof(string)))
{
try
{
var context = new TypeDescriptorContext(workflowElement, defaultProperty, serviceProvider);
var propertyValue = defaultProperty.Converter.ConvertFromString(context, arguments);
defaultProperty.SetValue(workflowElement, propertyValue);
}
catch (Exception ex)
{
throw new SystemException(ex.Message, ex);
}
}
}

public void InsertGraphNode(string typeName, ElementCategory elementCategory, CreateGraphNodeType nodeType, bool branch, bool group, string arguments)
public void CreateGraphNode(
string typeName,
ElementCategory elementCategory,
CreateGraphNodeType nodeType,
bool branch,
bool group)
{
CreateGraphNode(typeName, elementCategory, nodeType, branch, group, arguments: null);
}

public void CreateGraphNode(
string typeName,
ElementCategory elementCategory,
CreateGraphNodeType nodeType,
bool branch,
bool group,
string arguments)
{
if (string.IsNullOrEmpty(typeName))
{
Expand Down Expand Up @@ -1017,11 +1064,6 @@ public void InsertGraphNode(string typeName, ElementCategory elementCategory, Cr

builder = CreateBuilder(typeName, elementCategory, group);
ConfigureBuilder(builder, selectedNode, arguments);
if (typeName == typeof(ExternalizedMappingBuilder).AssemblyQualifiedName ||
typeName == typeof(AnnotationBuilder).AssemblyQualifiedName)
{
nodeType = CreateGraphNodeType.Predecessor;
}
var commands = GetCreateGraphNodeCommands(builder, selectedNodes.Select(GetGraphNodeTag), nodeType, branch);
commandExecutor.BeginCompositeCommand();
commandExecutor.Execute(EmptyAction, commands.updateLayout.Undo);
Expand All @@ -1031,39 +1073,6 @@ public void InsertGraphNode(string typeName, ElementCategory elementCategory, Cr
commandExecutor.EndCompositeCommand();
}

private void ConfigureBuilder(ExpressionBuilder builder, GraphNode selectedNode, string arguments)
{
if (string.IsNullOrEmpty(arguments)) return;
// TODO: This special case for binary operator operands should be avoided in the future
if (builder is BinaryOperatorBuilder binaryOperator && selectedNode != null)
{
if (GetGraphNodeTag(selectedNode).Value is InspectBuilder inputBuilder &&
inputBuilder.ObservableType != null)
{
binaryOperator.Build(Expression.Parameter(typeof(IObservable<>).MakeGenericType(inputBuilder.ObservableType)));
}
}

var workflowElement = ExpressionBuilder.GetWorkflowElement(builder);
var defaultProperty = TypeDescriptor.GetDefaultProperty(workflowElement);
if (defaultProperty != null &&
!defaultProperty.IsReadOnly &&
defaultProperty.Converter != null &&
defaultProperty.Converter.CanConvertFrom(typeof(string)))
{
try
{
var context = new TypeDescriptorContext(workflowElement, defaultProperty, serviceProvider);
var propertyValue = defaultProperty.Converter.ConvertFromString(context, arguments);
defaultProperty.SetValue(workflowElement, propertyValue);
}
catch (Exception ex)
{
throw new SystemException(ex.Message, ex);
}
}
}

public void CreateGraphNode(
ExpressionBuilder builder,
GraphNode selectedNode,
Expand Down Expand Up @@ -1154,6 +1163,13 @@ void ConfigureWorkflowBuilder(
var inspectParameter = new ExpressionBuilderArgument();

var targetNodes = selectedNodes.ToArray();
if (targetNodes.Length > 0 &&
(builder is ExternalizedMappingBuilder ||
builder is AnnotationBuilder))
{
nodeType = CreateGraphNodeType.Predecessor;
}

var restoreSelectedNodes = CreateUpdateSelectionDelegate(targetNodes);
if (insertIndex < 0)
{
Expand Down Expand Up @@ -1193,7 +1209,7 @@ void ConfigureWorkflowBuilder(
_ => false
}));

var reorder = GetSimpleSort();
var reorder = GetReversibleSort();
var updateGraphLayout = CreateUpdateGraphLayoutDelegate();
var updateSelectedNode = CreateUpdateSelectionDelegate(builder);
var insertCommands = GetInsertGraphNodeCommands(inspectNode, inspectNode, targetNodes, nodeType, branch, validateInsert);
Expand Down
4 changes: 2 additions & 2 deletions Bonsai.Editor/GraphView/WorkflowGraphView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ public void CreateGraphNode(
bool group,
string arguments)
{
try { Editor.InsertGraphNode(typeName, elementCategory, nodeType, branch, group, arguments); }
try { Editor.CreateGraphNode(typeName, elementCategory, nodeType, branch, group, arguments); }
catch (TargetInvocationException e)
{
var message = string.Format(Resources.CreateTypeNode_Error, name, e.InnerException.Message);
Expand Down Expand Up @@ -1692,7 +1692,7 @@ private ToolStripMenuItem CreatePropertySourceMenuItem(
valueProperty.SetValue(propertySource, memberValue);
propertySource.MemberName = memberName;

Editor.CreateGraphNode(propertySource, default(GraphNode), CreateGraphNodeType.Predecessor, branch: true);
Editor.CreateGraphNode(propertySource, default(GraphNode), CreateGraphNodeType.Successor, branch: true);
contextMenuStrip.Close(ToolStripDropDownCloseReason.ItemClicked);
});

Expand Down