Keep list siblings when undoing a move, and keep the node a drop displaces

Coder.Graph/AstGraph.cs

@@ -315,7 +315,63 @@ public AstLocation LocationOf(AstNode node)
 	/// <see cref="AstLocation.Root"/>, so undoing it is the same move in reverse.
 	/// </para>
 	/// </remarks>
-	public bool MoveTo(AstNode node, AstLocation location)
+	public bool MoveTo(AstNode node, AstLocation location) => Place(node, location, putBack: false);
+
+	/// <summary>
+	/// Puts a node back where a location says it was, inserting it into a sequence rather than
+	/// replacing whatever is there now.
+	/// </summary>
+	/// <param name="node">The node to put back.</param>
+	/// <param name="location">Where it was, as <see cref="LocationOf"/> reported it before it moved.</param>
+	/// <returns>True if the node was put back.</returns>
+	/// <remarks>
+	/// The inverse of <see cref="MoveTo"/>, which is what undo needs and what <see cref="MoveTo"/>
+	/// itself is not. Taking a node out of a sequence shifts every later sibling down, so the position
+	/// it came from now holds its next sibling; writing over that one would put the node back by
+	/// deleting its neighbour.
+	/// </remarks>
+	public bool PutBack(AstNode node, AstLocation location) => Place(node, location, putBack: true);
+
+	/// <summary>
+	/// Reports the node a move to a location would displace, if any.
+	/// </summary>
+	/// <param name="location">Where a node is about to be moved.</param>
+	/// <returns>
+	/// The entry of a sequence at that position, which <see cref="MoveTo"/> would replace and leave
+	/// loose; null when the position is free or holds only a placeholder.
+	/// </returns>
+	public static AstNode? OccupantAt(AstLocation location)
+	{
+		if (location.Parent is null || location.Slot is not { Cardinality: AstSlotCardinality.Many } slot)
+		{
+			return null;
+		}
+
+		IReadOnlyList<AstNode> children = AstSchema.ChildrenOf(location.Parent, slot);
+		return location.Index >= 0 && location.Index < children.Count && !AstSchema.IsUnfilled(children[location.Index])
+			? children[location.Index]
+			: null;
+	}
+
+	/// <summary>
+	/// Puts a node at a location, either replacing what is there or inserting before it.
+	/// </summary>
+	/// <param name="node">The node to move.</param>
+	/// <param name="location">Where to put it.</param>
+	/// <param name="putBack">Whether to insert into a sequence rather than replace an entry of it.</param>
+	/// <returns>True if the node was moved.</returns>
+	/// <remarks>
+	/// A replaced entry of a sequence becomes a loose node rather than leaving the graph, the same as
+	/// the children <see cref="Replace"/> cannot move: dropping a node onto a filled pin swaps it in,
+	/// and the user can see what came out and connect it somewhere else.
+	/// <para>
+	/// A location names a position before the node is taken out of wherever it is. When that is
+	/// earlier in the same sequence, taking it out shifts the target down by one, so the index is
+	/// adjusted to keep naming the entry the user dropped onto. Putting back needs no adjustment: the
+	/// position it was at is the position it should end up at.
+	/// </para>
+	/// </remarks>
+	private bool Place(AstNode node, AstLocation location, bool putBack)
 	{
 		Ensure.NotNull(node);
 
@@ -329,23 +385,44 @@ public bool MoveTo(AstNode node, AstLocation location)
 			return false;
 		}
 
+		AstLocation current = LocationOf(node);
+		if (current == location)
+		{
+			return true;
+		}
+
+		AstNode? displaced = putBack ? null : OccupantAt(location);
+		int index = location.Index;
+		if (!putBack
+			&& current.Parent is not null
+			&& ReferenceEquals(current.Parent, location.Parent)
+			&& current.Slot == location.Slot
+			&& current.Index < location.Index)
+		{
+			index--;
+		}
+
 		DetachFromParent(node);
 		detached.Remove(node);
 
-		if (location.Parent is null)
+		bool attached = location.Parent is null
+			|| (putBack
+				? AstSchema.TryInsertAt(location.Parent, location.Slot!, index, node)
+				: AstSchema.TryAttachAt(location.Parent, location.Slot!, index, node));
+
+		// A node the slot will not take any more stays in the graph rather than vanishing, and so does
+		// one it was swapped in for.
+		if (!attached || location.Parent is null)
 		{
 			detached.Add(node);
 		}
-		else if (!AstSchema.TryAttachAt(location.Parent, location.Slot!, location.Index, node))
+		else if (displaced is not null && !ReferenceEquals(displaced, node) && !detached.Contains(displaced))
 		{
-			// The slot will not take it any more, so the node stays in the graph rather than vanishing.
-			detached.Add(node);
-			Rebuild();
-			return false;
+			detached.Add(displaced);
 		}
 
 		Rebuild();
-		return true;
+		return attached;
 	}
 
 	/// <summary>

Coder.Graph/AstGraphEditor.cs

@@ -29,8 +29,8 @@ namespace ktsu.Coder.Graph;
 /// <para>
 /// Undo is <c>ktsu.UndoRedo</c>'s, recorded here rather than inside <see cref="AstGraph"/> because
 /// only the caller knows where one user-visible edit begins and ends. Reparenting is expressed as
-/// <see cref="AstGraph.MoveTo"/>, whose inverse is the same call with the location the node came
-/// from, so connecting, disconnecting and dragging a node between slots share one implementation of
+/// <see cref="AstGraph.MoveTo"/>, whose inverse is <see cref="AstGraph.PutBack"/> with the location the
+/// node came from, so connecting, disconnecting and dragging a node between slots share one implementation of
 /// undo rather than needing one each. The stack therefore holds a description of what each step did
 /// rather than an opaque snapshot, and it knows where the document was last saved.
 /// </para>
@@ -1539,7 +1539,7 @@ public bool RemoveLastChild(AstNode parent, AstSlot slot)
 			ChangeType.Delete,
 			child,
 			() => Graph.RemoveNode(child),
-			() => Graph.MoveTo(child, from));
+			() => Graph.PutBack(child, from));
 
 		statusMessage = $"Removed {AstSchema.Describe(child)} from {slot.Name}.";
 		return true;
@@ -1598,7 +1598,7 @@ private bool RemoveSubtree(AstNode? node)
 			ChangeType.Delete,
 			node,
 			() => Graph.RemoveNode(node),
-			() => Graph.MoveTo(node, from));
+			() => Graph.PutBack(node, from));
 
 		statusMessage = $"Removed {AstSchema.Describe(node)}.";
 		return true;
@@ -1612,8 +1612,36 @@ private bool RemoveSubtree(AstNode? node)
 	/// <param name="node">The node being moved.</param>
 	/// <param name="to">Where it is going.</param>
 	/// <param name="from">Where it came from, which is where undo puts it back.</param>
-	private void RecordMove(string description, ChangeType changeType, AstNode node, AstLocation to, AstLocation from) =>
-		Record(description, changeType, node, () => Graph.MoveTo(node, to), () => Graph.MoveTo(node, from));
+	/// <remarks>
+	/// A move onto a filled entry of a sequence displaces the entry, which <see cref="AstGraph.MoveTo"/>
+	/// leaves loose, so undo puts that back too. Both go back by insertion, in the order of the
+	/// positions they came from: each position was recorded with the other node still in the sequence,
+	/// so filling the earlier one first is what makes the later one mean the same place again.
+	/// </remarks>
+	private void RecordMove(string description, ChangeType changeType, AstNode node, AstLocation to, AstLocation from)
+	{
+		AstNode? displaced = AstGraph.OccupantAt(to);
+		if (ReferenceEquals(displaced, node))
+		{
+			displaced = null;
+		}
+
+		Record(description, changeType, node, () => Graph.MoveTo(node, to), () =>
+		{
+			List<(AstNode Node, AstLocation Where)> restores = [(node, from)];
+			if (displaced is not null)
+			{
+				restores.Add((displaced, to));
+			}
+
+			// A node going back to being loose goes first, so it is out of the sequence before anything
+			// is inserted into it.
+			foreach ((AstNode restored, AstLocation where) in restores.OrderBy(r => r.Where.Parent is null ? -1 : r.Where.Index))
+			{
+				Graph.PutBack(restored, where);
+			}
+		});
+	}
 
 	/// <summary>
 	/// Records an edit as an undoable step and performs it.

Coder.Graph/AstSchema.cs

@@ -26,12 +26,12 @@
 /// </remarks>
 public static class AstSchema
 {
 	private static readonly AstSlot ExpressionSlot = new("Expression", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot LeftSlot = new("Left", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot RightSlot = new("Right", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot OperandSlot = new("Operand", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot InitialValueSlot = new("InitialValue", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot TargetSlot = new("Target", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot ValueSlot = new("Value", AstSlotCardinality.One, AstSlotKind.Expression);
 	private static readonly AstSlot ParametersSlot = new("Parameters", AstSlotCardinality.Many, AstSlotKind.Parameter);
 	private static readonly AstSlot BodySlot = new("Body", AstSlotCardinality.Many, AstSlotKind.Statement);
@@ -212,6 +212,49 @@
 			|| TryAttachSequence(parent, slot, child);
 	}
 
+	/// <summary>
+	/// Attaches a child to a slot, inserting it at a position within a sequence rather than replacing.
+	/// </summary>
+	/// <param name="parent">The parent node.</param>
+	/// <param name="slot">The slot to fill.</param>
+	/// <param name="index">The position the child should end up at; ignored for a single-valued slot.</param>
+	/// <param name="child">The node to attach.</param>
+	/// <returns>True if the child was attached; false if the slot will not take it.</returns>
+	/// <remarks>
+	/// What putting a node back needs, where <see cref="TryAttachAt"/> is what dropping one onto a pin
+	/// needs. Taking an entry out of a sequence shifts every later one down, so the position it came
+	/// from now holds its next sibling, and writing over that would lose the sibling. Later entries are
+	/// shifted up instead, by taking each out and appending it again, which keeps this to the
+	/// sequence operations the schema already has.
+	/// </remarks>
+	public static bool TryInsertAt(AstNode parent, AstSlot slot, int index, AstNode child)
+	{
+		Ensure.NotNull(parent);
+		Ensure.NotNull(slot);
+		Ensure.NotNull(child);
+
+		int position = Math.Max(index, 0);
+		int count = slot.Cardinality == AstSlotCardinality.Many ? ChildrenOf(parent, slot).Count : 0;
+		if (position >= count)
+		{
+			return TryAttachAt(parent, slot, index, child);
+		}
+
+		if (!Accepts(slot, child) || !TryAttachSequence(parent, slot, child))
+		{
+			return false;
+		}
+
+		for (int shifted = position; shifted < count; shifted++)
+		{
+			AstNode following = ChildrenOf(parent, slot)[position];
+			TryDetachFromSequence(parent, slot, position);
+			TryAttachSequence(parent, slot, following);
+		}
+
+		return true;
+	}
+
 	/// <summary>
 	/// Fills one of an expression's own operand slots.
 	/// </summary>

Coder.Test/Graph/AstGraphSequenceUndoTests.cs

@@ -0,0 +1,159 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.Coder.Test.Graph;
+
+using System.Numerics;
+using ktsu.Coder.Ast;
+using ktsu.Coder.Graph;
+using ktsu.ImGui.NodeEditor;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Tests that edits to a sequence slot — a body, a parameter list, a member list — are undone without
+/// losing a sibling.
+/// </summary>
+/// <remarks>
+/// Taking a node out of a sequence shifts every later sibling down by one, so putting it back means
+/// inserting it at its old position rather than writing over whatever has moved into it. And a node
+/// dropped onto a filled pin displaces the occupant, which has to stay in the graph as a loose node
+/// and go back on undo, rather than drop out of the document with no warning.
+/// </remarks>
+[TestClass]
+public class AstGraphSequenceUndoTests
+{
+	private static FunctionDeclaration ThreeStatements()
+	{
+		FunctionDeclaration function = new("run");
+		function.Body.Add(new ExpressionStatement(new CallExpression("s0")));
+		function.Body.Add(new ExpressionStatement(new CallExpression("s1")));
+		function.Body.Add(new ExpressionStatement(new CallExpression("s2")));
+		return function;
+	}
+
+	/// <summary>
+	/// Tests that removing a middle statement and undoing it restores the body exactly.
+	/// </summary>
+	[TestMethod]
+	public void RemovingAMiddleStatement_IsUndoneWithoutLosingTheNextOne()
+	{
+		AstGraphEditor editor = new(ThreeStatements());
+		FunctionDeclaration function = (FunctionDeclaration)editor.Graph.Root;
+		AstNode[] original = [.. function.Body];
+
+		Assert.IsTrue(editor.Remove(FindId(editor, original[1])));
+		Assert.AreSequenceEqual((AstNode[])[original[0], original[2]], [.. function.Body]);
+
+		editor.Undo();
+
+		Assert.AreSequenceEqual(original, [.. function.Body]);
+		Assert.IsEmpty(editor.Graph.Detached);
+	}
+
+	/// <summary>
+	/// Tests that disconnecting a middle statement and undoing it restores the body exactly.
+	/// </summary>
+	[TestMethod]
+	public void DisconnectingAMiddleStatement_IsUndoneWithoutLosingTheNextOne()
+	{
+		AstGraphEditor editor = new(ThreeStatements());
+		FunctionDeclaration function = (FunctionDeclaration)editor.Graph.Root;
+		AstNode[] original = [.. function.Body];
+
+		Assert.IsTrue(editor.Disconnect(LinkInto(editor, original[1])));
+		Assert.AreSequenceEqual((AstNode[])[original[0], original[2]], [.. function.Body]);
+		Assert.AreSame(original[1], editor.Graph.Detached.Single());
+
+		editor.Undo();
+
+		Assert.AreSequenceEqual(original, [.. function.Body]);
+		Assert.IsEmpty(editor.Graph.Detached);
+	}
+
+	/// <summary>
+	/// Tests that connecting a statement onto a later pin of its own body replaces that pin's
+	/// occupant, keeps the occupant in the graph, and is undone exactly — and redone the same way.
+	/// </summary>
+	[TestMethod]
+	public void ConnectingWithinTheSameBody_ReplacesThePinsOccupantAndIsUndone()
+	{
+		AstGraphEditor editor = new(ThreeStatements());
+		FunctionDeclaration function = (FunctionDeclaration)editor.Graph.Root;
+		AstNode[] original = [.. function.Body];
+
+		AstConnectResult result = editor.Connect(OutputPinOf(editor, original[0]), InputPinOf(editor, function, "Body", 1));
+
+		Assert.IsTrue(result.Success, result.Message);
+		Assert.AreSequenceEqual((AstNode[])[original[0], original[2]], [.. function.Body], "s1 held the pin, so s1 is the one replaced");
+		Assert.AreSame(original[1], editor.Graph.Detached.Single(), "the displaced statement stays in the graph");
+
+		editor.Undo();
+
+		Assert.AreSequenceEqual(original, [.. function.Body]);
+		Assert.IsEmpty(editor.Graph.Detached);
+
+		editor.Redo();
+
+		Assert.AreSequenceEqual((AstNode[])[original[0], original[2]], [.. function.Body]);
+		Assert.AreSame(original[1], editor.Graph.Detached.Single());
+	}
+
+	/// <summary>
+	/// Tests that connecting a loose node onto a filled sequence pin keeps the occupant it displaces,
+	/// and that undoing it puts both back where they were.
+	/// </summary>
+	[TestMethod]
+	public void ConnectingALooseNodeOntoAFilledPin_KeepsTheOccupantAndIsUndone()
+	{
+		AstGraphEditor editor = new(ThreeStatements());
+		FunctionDeclaration function = (FunctionDeclaration)editor.Graph.Root;
+		AstNode[] original = [.. function.Body];
+		ExpressionStatement loose = new(new CallExpression("x"));
+		editor.Add(loose, new Vector2(400, 400));
+
+		AstConnectResult result = editor.Connect(OutputPinOf(editor, loose), InputPinOf(editor, function, "Body", 0));
+
+		Assert.IsTrue(result.Success, result.Message);
+		Assert.AreSequenceEqual((AstNode[])[loose, original[1], original[2]], [.. function.Body]);
+		Assert.AreSame(original[0], editor.Graph.Detached.Single(), "the displaced statement stays in the graph");
+
+		editor.Undo();
+
+		Assert.AreSequenceEqual(original, [.. function.Body]);
+		Assert.AreSame(loose, editor.Graph.Detached.Single(), "the loose node goes back to being loose");
+	}
+
+	/// <summary>
+	/// Tests that inserting into a sequence shifts the later entries up rather than writing over one.
+	/// </summary>
+	[TestMethod]
+	public void TryInsertAt_ShiftsLaterEntriesUp()
+	{
+		FunctionDeclaration function = ThreeStatements();
+		AstNode[] original = [.. function.Body];
+		AstSlot body = AstSchema.SlotsOf(function).Single(s => s.Name == "Body");
+		ExpressionStatement inserted = new(new CallExpression("x"));
+
+		Assert.IsTrue(AstSchema.TryInsertAt(function, body, 1, inserted));
+
+		Assert.AreSequenceEqual((AstNode[])[original[0], inserted, original[1], original[2]], [.. function.Body]);
+	}
+
+	private static int FindId(AstGraphEditor editor, AstNode node) =>
+		editor.Graph.Nodes.Single(pair => ReferenceEquals(pair.Value, node)).Key;
+
+	private static int OutputPinOf(AstGraphEditor editor, AstNode node) =>
+		editor.Graph.Engine.Nodes.Single(n => n.Id == FindId(editor, node)).OutputPins[0].Id;
+
+	private static int InputPinOf(AstGraphEditor editor, AstNode parent, string slotName, int index)
+	{
+		Node parentNode = editor.Graph.Engine.Nodes.Single(n => n.Id == FindId(editor, parent));
+		AstSlot slot = AstSchema.SlotsOf(parent).Single(s => s.Name == slotName);
+		return parentNode.InputPins.Single(p => p.EffectiveDisplayName == AstGraph.PinLabel(slot, index)).Id;
+	}
+
+	private static int LinkInto(AstGraphEditor editor, AstNode child)
+	{
+		int outputPin = OutputPinOf(editor, child);
+		return editor.Graph.Engine.Links.Single(l => l.OutputPinId == outputPin).Id;
+	}
+}