From 60e9d1a31f9cd98a04efe286e8c1bbfa4651e1bc Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 06:36:42 +0000 Subject: [PATCH 1/2] Offer a node's own menu on right-click, to add a pin (closes #36) [minor] Right-clicking a node opened the create-node palette, the same menu empty canvas offers, so the only way to grow a variadic slot was to select the node and press the inspector's +. The gesture now decides which menu it means: over a node it opens that node's own menu, over empty canvas the palette as before. The menu offers one entry per slot that can hold another child, which is the same undoable edit the inspector's + makes, and says so rather than opening empty when the node has no such slot. Which slots those are is a fact about the node's shape, so AstSchema.GrowableSlotsOf answers it and the draw call holds no rule of its own. Removing a pin is deliberately left out: the slot is an ordered sequence, so which child the user means is a question a node-level menu cannot ask. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011gZgXrkFA4Fy13yuxgmqKz --- Coder.Graph/AstGraphEditor.cs | 128 +++++++++- Coder.Graph/AstSchema.cs | 14 + .../Graph/AstGraphEditorNodeMenuTests.cs | 240 ++++++++++++++++++ Coder.Test/Graph/AstSchemaTests.cs | 28 ++ 4 files changed, 409 insertions(+), 1 deletion(-) create mode 100644 Coder.Test/Graph/AstGraphEditorNodeMenuTests.cs diff --git a/Coder.Graph/AstGraphEditor.cs b/Coder.Graph/AstGraphEditor.cs index 4109429..ca8ac24 100644 --- a/Coder.Graph/AstGraphEditor.cs +++ b/Coder.Graph/AstGraphEditor.cs @@ -67,6 +67,11 @@ public sealed class AstGraphEditor(AstNode root) /// private const string LinkDropPopup = "ast-graph-link-drop"; + /// + /// The popup offering a node's own actions, opened by right-clicking it. + /// + private const string NodeMenuPopup = "ast-graph-node-menu"; + private string statusMessage = string.Empty; private string fieldBuffer = string.Empty; @@ -94,6 +99,26 @@ public sealed class AstGraphEditor(AstNode root) /// private bool linkDropOpening; + /// + /// The node whose own menu is open, while it is. Null when none is. + /// + /// + /// Held as the AST node rather than the editor's identifier for it, for the reason + /// is: adding a child rebuilds the engine graph and reassigns those + /// identifiers, so a menu remembered by number would go on to grow whichever node inherited it. + /// + private AstNode? nodeMenuNode; + + /// + /// Whether the menu for still has to be opened. + /// + /// + /// Separate from the node for the reason is separate from the pin: + /// only means anything inside a frame, and the request to + /// offer the menu can arrive from outside one. + /// + private bool nodeMenuOpening; + /// /// Gets the graph being edited. /// @@ -221,6 +246,7 @@ public void Draw(Vector2 size, float deltaTime) TrackSelection(); ApplyDeletions(); DrawPalette(); + DrawNodeMenu(); DrawLinkDropMenu(); if (ShowDebugOverlays) @@ -1113,6 +1139,95 @@ public bool RequestCreateFrom(int pinId) return true; } + /// + /// Offers a node's own menu, the way right-clicking the node does. + /// + /// The node to offer the menu for. + /// True if the node is one this graph knows, so the menu will open. + /// + /// Separate from the gesture that usually triggers it, for the reason + /// is: a host binding the menu to a key wants the same menu, and it + /// can be driven in a test without the node happening to be under the pointer. + /// + public bool RequestNodeMenu(int nodeId) + { + AstNode? node = Graph.AstNodeFor(nodeId); + if (node is null) + { + statusMessage = "That node is not in this graph."; + return false; + } + + nodeMenuNode = node; + nodeMenuOpening = true; + return true; + } + + /// + /// Draws a node's own menu, and adds to whichever slot the user picks. + /// + /// + /// What the menu offers is one entry per slot that can hold another child, which is the same edit + /// the inspector's + makes — reachable at the node itself rather than only for the node the + /// inspector has selected. A node with no such slot says so rather than opening empty, the same way + /// the dropped-link menu does. + /// + /// Removing a pin is deliberately not here: the slot is an ordered sequence, so which child a user + /// means is a question the node's own menu cannot ask, and disconnecting the child says it already. + /// + /// + private void DrawNodeMenu() + { + if (nodeMenuNode is not AstNode node) + { + return; + } + + if (nodeMenuOpening) + { + ImGui.OpenPopup(NodeMenuPopup); + nodeMenuOpening = false; + } + + if (!ImGui.BeginPopup(NodeMenuPopup)) + { + // Remembered exactly as long as the popup is open: once it has gone the user either picked + // something or dismissed it, and either way the node is no longer the menu's subject. + nodeMenuNode = null; + return; + } + + IReadOnlyList growable = AstSchema.GrowableSlotsOf(node); + if (growable.Count == 0) + { + ImGui.TextDisabled($"{AstSchema.Describe(node)} has no slot to add to."); + ImGuiProbes.MarkItem("Nothing to add"); + ImGui.EndPopup(); + return; + } + + foreach (AstSlot slot in growable) + { + bool picked = ImGui.MenuItem($"Add {slot.Name}"); + + // Marked under a name of its own rather than the label it reads as, because the inspector + // draws a row for the same edit and a test asking for one should not find the other. + ImGuiProbes.MarkItem($"Node add {slot.Name}"); + + if (!picked) + { + continue; + } + + AddChild(node, slot); + nodeMenuNode = null; + ImGui.CloseCurrentPopup(); + break; + } + + ImGui.EndPopup(); + } + /// /// Creates a node from a template and connects it to the pin a link was dragged off. /// @@ -1408,7 +1523,18 @@ private void DrawPalette() { if (ImGui.IsMouseClicked(ImGuiMouseButton.Right) && ImGui.IsWindowHovered(ImGuiHoveredFlags.RootAndChildWindows)) { - ImGui.OpenPopup("ast-graph-palette"); + // A right-click over a node is about that node, so the palette answers only one over empty + // canvas. Both gestures are the same click, and which menu it means is decided in one place + // rather than by two handlers each opening a popup. + int hovered = 0; + if (ImNodes.IsNodeHovered(ref hovered)) + { + RequestNodeMenu(hovered); + } + else + { + ImGui.OpenPopup("ast-graph-palette"); + } } if (!ImGui.BeginPopup("ast-graph-palette")) diff --git a/Coder.Graph/AstSchema.cs b/Coder.Graph/AstSchema.cs index 8fd5f7d..67c9856 100644 --- a/Coder.Graph/AstSchema.cs +++ b/Coder.Graph/AstSchema.cs @@ -107,6 +107,20 @@ public static class AstSchema _ => [], }; + /// + /// Lists the slots of a node that a child can be added to without displacing one already there. + /// + /// The node to describe. + /// The node's variadic slots, in the order the editor draws their pins. Empty when it has none. + /// + /// A slot holding at most one child is left out rather than offered and refused: connecting a + /// second child to one replaces the first, so adding to it is not a thing the node can be asked + /// for. Which slots these are is a fact about the node's shape, so it is answered here rather than + /// by whatever draws the menu. + /// + public static IReadOnlyList GrowableSlotsOf(AstNode node) => + [.. SlotsOf(node).Where(slot => slot.Cardinality == AstSlotCardinality.Many)]; + /// /// Reads the children currently sitting in a slot. /// diff --git a/Coder.Test/Graph/AstGraphEditorNodeMenuTests.cs b/Coder.Test/Graph/AstGraphEditorNodeMenuTests.cs new file mode 100644 index 0000000..e027127 --- /dev/null +++ b/Coder.Test/Graph/AstGraphEditorNodeMenuTests.cs @@ -0,0 +1,240 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Coder.Test.Graph; + +using System.Linq; +using System.Numerics; +using Hexa.NET.ImGui; +using Hexa.NET.ImNodes; +using ktsu.Coder.Ast; +using ktsu.Coder.Graph; +using ktsu.ImGui.App; +using ktsu.ImGui.App.Testing; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Covers the menu a right-clicked node opens as the user meets it: entries reached with the mouse, +/// by the label they read as, through the headless rasterizer. +/// +/// +/// Which slots the menu offers is a fact about the node's shape and is covered headlessly in +/// . What is covered here is that the popup draws an entry per slot at +/// all, that picking one reaches the edit, and that a right-click over a node opens this menu rather +/// than the create-node palette — the half that only a rendered frame can establish, since ImGui +/// menus fault inside native code rather than throwing when misnested. +/// +/// The menu is opened through in every test but the +/// gesture's own, because where a node lands on screen depends on the force-directed layout. The one +/// test that does click the node stops the layout first and asks ImNodes where the node ended up. +/// +/// +/// ImGui contexts are process-global, so only one harness can be live at a time and this class must +/// not run its methods in parallel. +/// +/// +[TestClass] +[DoNotParallelize] +public sealed class AstGraphEditorNodeMenuTests +{ + private static readonly HarnessOptions Options = new() { Width = 1280, Height = 800 }; + + private static FunctionDeclaration SampleFunction() + { + FunctionDeclaration function = new("total") { ReturnType = "int" }; + function.Parameters.Add(new Parameter("a", "int")); + function.Body.Add(new ReturnStatement( + new BinaryExpression(new VariableReference("a"), BinaryOperator.Add, Literal.Number(1)))); + return function; + } + + private static ImGuiAppConfig ConfigFor(AstGraphEditor editor) => new() + { + Title = "AST node menu", + OnRender = delta => + { + ImGui.Begin("graph"); + editor.Draw(new Vector2(1000, 600), delta); + ImGui.End(); + }, + }; + + private static bool IsVisible(ImGuiAppHarness harness, string name) => + harness.Probe.WasSeenInFrame(name, harness.FrameCount - 1); + + /// + /// Finds the editor node standing for an AST node, the way the editor does. + /// + /// The graph under test. + /// The AST node to find. + /// The editor node's id. + private static int NodeId(AstGraph graph, AstNode node) => + graph.Engine.Nodes.Single(n => ReferenceEquals(graph.AstNodeFor(n.Id), node)).Id; + + /// + /// Starts a harness with a node's own menu already open. + /// + /// The editor to draw. + /// The node to open the menu for. + /// The running harness, for the caller to dispose. + private static ImGuiAppHarness Offering(AstGraphEditor editor, AstNode node) + { + ImGuiAppHarness harness = ImGuiAppHarness.Start(ConfigFor(editor), Options); + harness.Step(2); + + Assert.IsTrue(editor.RequestNodeMenu(NodeId(editor.Graph, node)), "the node to offer a menu for should be in the graph"); + harness.Step(2); + return harness; + } + + /// + /// Tests that the menu offers one entry per slot the node can hold another child in. + /// + [TestMethod] + public void Menu_OffersAnEntryPerSlotThatCanGrow() + { + FunctionDeclaration function = SampleFunction(); + AstGraphEditor editor = new(function); + + using ImGuiAppHarness harness = Offering(editor, function); + + Assert.IsTrue(IsVisible(harness, "Node add Parameters"), "a function's parameter list can take another"); + Assert.IsTrue(IsVisible(harness, "Node add Body"), "a function's body can take another statement"); + Assert.IsFalse(IsVisible(harness, "Nothing to add"), "a function has slots to add to, so the menu is not empty"); + } + + /// + /// Tests that picking an entry adds a child to that slot, through the menu rather than by calling + /// the edit directly, and that the step is undoable. + /// + [TestMethod] + public void Menu_AddsToThePickedSlot() + { + FunctionDeclaration function = SampleFunction(); + AstGraphEditor editor = new(function); + + using ImGuiAppHarness harness = Offering(editor, function); + + harness.Click("Node add Parameters"); + harness.Step(2); + + Assert.HasCount(2, function.Parameters); + Assert.HasCount(1, function.Body); + Assert.IsTrue(editor.History.CanUndo, "adding through the menu should be undoable"); + + editor.Undo(); + Assert.HasCount(1, function.Parameters); + } + + /// + /// Tests that a node whose every slot holds one child says so, rather than opening an empty menu. + /// + [TestMethod] + public void Menu_SaysSoWhenTheNodeHasNoSlotToAddTo() + { + BinaryExpression sum = new(new VariableReference("a"), BinaryOperator.Add, Literal.Number(1)); + AstGraphEditor editor = new(sum); + + using ImGuiAppHarness harness = Offering(editor, sum); + + Assert.IsTrue(IsVisible(harness, "Nothing to add"), "both of a binary expression's slots hold one child"); + Assert.IsFalse(IsVisible(harness, "Node add Left"), "a slot holding one child is not offered"); + } + + /// + /// Tests that dismissing the menu without picking anything leaves the document alone and forgets the + /// node, rather than offering the menu again on the next frame. + /// + [TestMethod] + public void Menu_DismissedWithoutPicking_ChangesNothing() + { + FunctionDeclaration function = SampleFunction(); + AstGraphEditor editor = new(function); + + using ImGuiAppHarness harness = Offering(editor, function); + Assert.IsTrue(IsVisible(harness, "Node add Parameters"), "the menu should be open to begin with"); + + int before = editor.Graph.Nodes.Count; + + // Clicked away from the popup rather than dismissed with Escape: a click outside is what closes + // an ImGui popup, and it is also what a user who changed their mind actually does. + harness.Mouse.Click(1220, 60); + harness.Step(3); + + Assert.IsFalse(IsVisible(harness, "Node add Parameters"), "the menu should be gone once dismissed"); + Assert.HasCount(before, editor.Graph.Nodes); + Assert.IsFalse(editor.History.CanUndo, "dismissing should record nothing"); + Assert.HasCount(1, function.Parameters); + } + + /// + /// Tests that right-clicking a node opens that node's menu, where right-clicking empty canvas still + /// opens the create-node palette. + /// + /// + /// The layout is stopped and the view left where the first frames fitted it, so the node is where + /// ImNodes last put it rather than somewhere the simulation has since moved it. + /// + [TestMethod] + public void RightClickOnANode_OpensThatNodesMenu() + { + FunctionDeclaration function = SampleFunction(); + AstGraphEditor editor = new(function) { LayoutRunning = false }; + + Vector2 topLeft = Vector2.Zero; + Vector2 size = Vector2.Zero; + int nodeId = 0; + + ImGuiAppConfig config = new() + { + Title = "AST node menu gesture", + OnRender = delta => + { + ImGui.Begin("graph"); + editor.Draw(new Vector2(1000, 600), delta); + + // Read inside the frame that drew it: the node's screen position is ImNodes' own, and it + // is only meaningful while its context is the current one. + if (nodeId != 0) + { + topLeft = ImNodes.GetNodeScreenSpacePos(nodeId); + size = ImNodes.GetNodeDimensions(nodeId); + } + + ImGui.End(); + }, + }; + + using ImGuiAppHarness harness = ImGuiAppHarness.Start(config, Options); + harness.Step(4); + + nodeId = NodeId(editor.Graph, function); + harness.Step(2); + + Assert.IsGreaterThan(0f, size.X, "the node should have been measured by now"); + + // The title bar rather than the middle of the body: a node's rows are the pins, and a click + // landing on one is a different gesture. + harness.Mouse.Click(topLeft.X + (size.X * 0.5f), topLeft.Y + 4f, 1); + harness.Step(2); + + Assert.IsTrue(IsVisible(harness, "Node add Parameters"), "right-clicking the node should open its own menu"); + } + + /// + /// Tests that a node that is not in the graph is refused, so no menu is offered for it. + /// + [TestMethod] + public void RequestNodeMenu_UnknownNode_IsRefused() + { + AstGraphEditor editor = new(SampleFunction()); + + using ImGuiAppHarness harness = ImGuiAppHarness.Start(ConfigFor(editor), Options); + harness.Step(2); + + Assert.IsFalse(editor.RequestNodeMenu(-1)); + harness.Step(2); + + Assert.IsFalse(IsVisible(harness, "Node add Parameters")); + Assert.IsFalse(IsVisible(harness, "Nothing to add")); + } +} diff --git a/Coder.Test/Graph/AstSchemaTests.cs b/Coder.Test/Graph/AstSchemaTests.cs index 0ebd115..f9dd902 100644 --- a/Coder.Test/Graph/AstSchemaTests.cs +++ b/Coder.Test/Graph/AstSchemaTests.cs @@ -424,6 +424,34 @@ public void Describe_FallsBackToTheNodeTypeName() => public void Describe_HandlesANullLiteralValue() => Assert.AreEqual("\"\"", AstSchema.Describe(new LiteralExpression())); + /// + /// Tests that the slots a child can be added to are the variadic ones, in the order the editor + /// draws their pins. + /// + [TestMethod] + public void GrowableSlotsOf_KeepsOnlyTheSlotsThatHoldASequence() => + CollectionAssert.AreEqual( + FunctionSlots, + AstSchema.GrowableSlotsOf(new FunctionDeclaration("f")).Select(s => s.Name).ToArray()); + + /// + /// Tests that a node whose every slot holds one child has none to add to, since connecting a second + /// child to such a slot replaces the first rather than joining it. + /// + [TestMethod] + public void GrowableSlotsOf_IsEmptyWhenEverySlotHoldsOneChild() + { + Assert.IsEmpty(AstSchema.GrowableSlotsOf(NewBinary())); + Assert.IsEmpty(AstSchema.GrowableSlotsOf(new ReturnStatement())); + } + + /// + /// Tests that a leaf, which has no slots at all, has none to add to either. + /// + [TestMethod] + public void GrowableSlotsOf_IsEmptyForALeaf() => + Assert.IsEmpty(AstSchema.GrowableSlotsOf(new VariableReference("a"))); + private static BinaryExpression NewBinary() => new(new VariableReference("a"), BinaryOperator.Add, new VariableReference("b")); From 73085d7511fd86e1a7f507febbac16c450f090f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 06:47:53 +0000 Subject: [PATCH 2/2] style(test): take the assertion Sonar names for the new test [patch] MSTEST0068 on the test this PR adds: Assert.AreSequenceEqual rather than CollectionAssert.AreEqual. The rest of the file keeps CollectionAssert, since Sonar reads only new code and rewriting the file is not this PR's business. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011gZgXrkFA4Fy13yuxgmqKz --- Coder.Test/Graph/AstSchemaTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Coder.Test/Graph/AstSchemaTests.cs b/Coder.Test/Graph/AstSchemaTests.cs index f9dd902..a9a82e5 100644 --- a/Coder.Test/Graph/AstSchemaTests.cs +++ b/Coder.Test/Graph/AstSchemaTests.cs @@ -430,9 +430,9 @@ public void Describe_HandlesANullLiteralValue() => /// [TestMethod] public void GrowableSlotsOf_KeepsOnlyTheSlotsThatHoldASequence() => - CollectionAssert.AreEqual( + Assert.AreSequenceEqual( FunctionSlots, - AstSchema.GrowableSlotsOf(new FunctionDeclaration("f")).Select(s => s.Name).ToArray()); + AstSchema.GrowableSlotsOf(new FunctionDeclaration("f")).Select(s => s.Name)); /// /// Tests that a node whose every slot holds one child has none to add to, since connecting a second