diff --git a/Coder.Graph/AstGraph.cs b/Coder.Graph/AstGraph.cs index a425ca3..62e4ba0 100644 --- a/Coder.Graph/AstGraph.cs +++ b/Coder.Graph/AstGraph.cs @@ -315,7 +315,63 @@ public AstLocation LocationOf(AstNode node) /// , so undoing it is the same move in reverse. /// /// - public bool MoveTo(AstNode node, AstLocation location) + public bool MoveTo(AstNode node, AstLocation location) => Place(node, location, putBack: false); + + /// + /// Puts a node back where a location says it was, inserting it into a sequence rather than + /// replacing whatever is there now. + /// + /// The node to put back. + /// Where it was, as reported it before it moved. + /// True if the node was put back. + /// + /// The inverse of , which is what undo needs and what + /// 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. + /// + public bool PutBack(AstNode node, AstLocation location) => Place(node, location, putBack: true); + + /// + /// Reports the node a move to a location would displace, if any. + /// + /// Where a node is about to be moved. + /// + /// The entry of a sequence at that position, which would replace and leave + /// loose; null when the position is free or holds only a placeholder. + /// + public static AstNode? OccupantAt(AstLocation location) + { + if (location.Parent is null || location.Slot is not { Cardinality: AstSlotCardinality.Many } slot) + { + return null; + } + + IReadOnlyList children = AstSchema.ChildrenOf(location.Parent, slot); + return location.Index >= 0 && location.Index < children.Count && !AstSchema.IsUnfilled(children[location.Index]) + ? children[location.Index] + : null; + } + + /// + /// Puts a node at a location, either replacing what is there or inserting before it. + /// + /// The node to move. + /// Where to put it. + /// Whether to insert into a sequence rather than replace an entry of it. + /// True if the node was moved. + /// + /// A replaced entry of a sequence becomes a loose node rather than leaving the graph, the same as + /// the children 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. + /// + /// 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. + /// + /// + 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; } /// diff --git a/Coder.Graph/AstGraphEditor.cs b/Coder.Graph/AstGraphEditor.cs index 00eccff..7571176 100644 --- a/Coder.Graph/AstGraphEditor.cs +++ b/Coder.Graph/AstGraphEditor.cs @@ -29,8 +29,8 @@ namespace ktsu.Coder.Graph; /// /// Undo is ktsu.UndoRedo's, recorded here rather than inside because /// only the caller knows where one user-visible edit begins and ends. Reparenting is expressed as -/// , 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 +/// , whose inverse is 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. /// @@ -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) /// The node being moved. /// Where it is going. /// Where it came from, which is where undo puts it back. - 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)); + /// + /// A move onto a filled entry of a sequence displaces the entry, which + /// 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. + /// + 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); + } + }); + } /// /// Records an edit as an undoable step and performs it. diff --git a/Coder.Graph/AstSchema.cs b/Coder.Graph/AstSchema.cs index 1e49466..0c372ed 100644 --- a/Coder.Graph/AstSchema.cs +++ b/Coder.Graph/AstSchema.cs @@ -212,6 +212,49 @@ public static bool TryAttachAt(AstNode parent, AstSlot slot, int index, AstNode || TryAttachSequence(parent, slot, child); } + /// + /// Attaches a child to a slot, inserting it at a position within a sequence rather than replacing. + /// + /// The parent node. + /// The slot to fill. + /// The position the child should end up at; ignored for a single-valued slot. + /// The node to attach. + /// True if the child was attached; false if the slot will not take it. + /// + /// What putting a node back needs, where 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. + /// + 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; + } + /// /// Fills one of an expression's own operand slots. /// diff --git a/Coder.Test/Graph/AstGraphSequenceUndoTests.cs b/Coder.Test/Graph/AstGraphSequenceUndoTests.cs new file mode 100644 index 0000000..ccc83c8 --- /dev/null +++ b/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; + +/// +/// Tests that edits to a sequence slot — a body, a parameter list, a member list — are undone without +/// losing a sibling. +/// +/// +/// 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. +/// +[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; + } + + /// + /// Tests that removing a middle statement and undoing it restores the body exactly. + /// + [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); + } + + /// + /// Tests that disconnecting a middle statement and undoing it restores the body exactly. + /// + [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); + } + + /// + /// 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. + /// + [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()); + } + + /// + /// 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. + /// + [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"); + } + + /// + /// Tests that inserting into a sequence shifts the later entries up rather than writing over one. + /// + [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; + } +}