[minor] Delete selected nodes with Delete or Backspace - #456
Merged
Merged
Conversation
Selecting a node and pressing Delete did nothing. ImNodes tracks which nodes are selected but never acts on that selection, and unlike a link there is no IsNodeDestroyed to fall back on, so no user gesture could ask for a node to be removed at all. NodeEditorInputHandler now drains the node selection the same way it drains the link selection, into a new InputEvents.NodeDeletionRequests. The key read moves into one IsDeleteSelectionPressed so a single press clears links and nodes together rather than letting the two kinds drift apart. Removing a node takes its links with it, which is already NodeEditorEngine.RemoveNode's job. CleanImNodesDemo consumes the new requests, draining them after the link requests so a link selected alongside its node is removed by its own request. Six tests in NodeEditorInteractionTests drive the gesture through real frames via ImGuiAppHarness: Delete, Backspace, a multi-node selection, a selection with no key pressed, a second press after the first was handled, and one press with both a node and a link selected. Five of the six fail with the handler change reverted. Fixes #454 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPZJNads6wQ5xqenyAh3mp
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
SonarCloud's quality gate failed this PR on new-code coverage (78.9%, required 80%). Measured locally, the only uncovered new line in the library was the early return in IsDeleteSelectionPressed - the guard that keeps a Delete aimed at a text field from reaching the graph. Nothing had ever driven it, on the node path or the link path it was moved from. That is the one failure of this gesture that destroys work rather than merely doing nothing, so it wanted a test regardless of the gate. The fixture draws a real ImGui.InputText and gives it the keyboard, since WantTextInput is only raised while a text widget is active and cannot be set directly. With a node and a link both selected, Delete now has to leave both alone. Neutralising the guard fails the new test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPZJNads6wQ5xqenyAh3mp
|
matt-edmondson
added a commit
that referenced
this pull request
Sep 24, 2026
#456 and #447 both landed while this branch was open, and both touched what it touches. #456 is the sibling gesture: it added ProcessSelectedNodeDeletion and lifted the key read into IsDeleteSelectionPressed, in the same method this branch adds ProcessSelectedNodeDuplication to. Both halves are kept. It also added the same text-field test fixture this branch grew for its own WantTextInput guard, so the fixture is now declared once and both typing tests use it. #447 is the one that changes behaviour rather than layout. It gave a pin a declared type and a value, living either on an instance the factory bound or in the engine's own store. DuplicateNodes predates all of that: it copied a pin record, so the type came across for free, but the value did not. Duplicating a node whose Threshold the user had set to 50 gave a copy reading 0 - a copy that looks the same and computes something else, which is the failure this branch's whole premise is meant to avoid. So a copied pin's value is now written through SetPinValue, which vets it against the copy's declared type exactly as the original's was vetted. A copy has no instance and no accessor, so a value the original kept on a bound property lands in the engine's store under the new pin id: same value, different home. A copy has no seeded default either, so ResetPinValue on one reports nothing to go back to; that is documented rather than worked around. Three tests cover the interaction. Removing the value carry fails the one that asserts the copy arrives tuned; the other two hold for their own reasons - the declared type rides the record copy, and the copy's value is its own because the store is keyed by pin id - and are there so a later change to how pins are copied cannot quietly alias them. 224 tests pass across ImGui.NodeEditor.Tests, which is this branch's eighteen plus everything #456 and #447 brought. dotnet build ImGui.sln succeeds with 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPZJNads6wQ5xqenyAh3mp
matt-edmondson
added a commit
that referenced
this pull request
Sep 24, 2026
SonarCloud raised a CRITICAL S3776 on the demo: cognitive complexity 16 against a limit of 15. The gestures grew one at a time - link creation, link deletion, then #456's node deletion, then this branch's duplication - and the fourth is what crossed the line, so it is this branch's to fix. A pure extraction: one method per request kind, called in the same order the requests were drained in. That order is the part that carries meaning and it is unchanged, so a link selected alongside the node it hangs off is still removed by its own request rather than silently by RemoveNode. The duplication block's `if (Count > 0)` becomes an early return, which is the same branch read the other way. The S3267 suppression that sat on the combined method moves onto the two loops that actually trip it, with the justification narrowed to each. The loops themselves are untouched: RemoveLink and RemoveNode are the mutation rather than a predicate, and a Where rewrite would hide a graph edit inside a lazily-evaluated filter. dotnet build ImGui.sln succeeds with 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KPZJNads6wQ5xqenyAh3mp
This was referenced Sep 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #454.
Links gained "select, then press Delete" in #452. Nodes did not, so the two halves of the same gesture behaved differently.
Why nothing happened before
ImNodes tracks which nodes are selected but never acts on that selection, and unlike a link there is no
IsNodeDestroyedto fall back on —NodeEditorInputHandlerhad no node-deletion path at all, so no user gesture could put anything in front of the application asking for a node to be removed.What changed
InputEvents.NodeDeletionRequests, the node counterpart ofLinkDeletionRequests. The handler reports; the application removes, exactly as it already did for links.ProcessSelectedNodeDeletiondrainsImNodes.NumSelectedNodes/GetSelectedNodes, de-duplicates, and callsClearNodeSelectionso the same ids are not re-requested on the next press.IsDeleteSelectionPressednow holds the key read thatProcessSelectedLinkDeletionused to do inline, and both selections are drained from one press. Reading the key separately per selection kind would work today but invites the two to drift apart, which is how a Delete ends up taking the links and leaving the node behind.CleanImNodesDemoconsumes the new requests, draining them after the link requests so a link selected alongside its own node is removed by its own request rather than silently byRemoveNode. Either order leaves the same graph.Backspace is included for the same reason it is on links: on a Mac keyboard it is the key labelled Delete. The
WantTextInputguard moves with the key read and is unchanged in behaviour, so a Delete aimed at a text field still is not aimed at the graph — it now has a test, which it never had before (see below).Removing a node removes the links that reach it — that is already
NodeEditorEngine.RemoveNode's job and is not duplicated here. The consequence is that a selected link hanging off a selected node can appear in both lists; whichever the application drains second finds that id already gone, andRemoveLink/RemoveNodeboth returnfalserather than throwing.Not in this PR
No
Ctrl+Dduplication — that is #455, which is now open as #457 and stands alone onmainrather than on this branch.Testing
dotnet test tests/ImGui.NodeEditor.Tests— 125 passed, 0 failed. Eight new tests inNodeEditorInteractionTestsdrive the gesture through real frames viaImGuiAppHarness: Delete, Backspace, a multi-node selection, a selection with no key pressed, a second press after the first was handled, one press with both a node and a link selected, the engine-side result that the node and its links both go, and a Delete pressed while a focused text field holds the keyboard.That last one arrived late. SonarCloud's quality gate failed the first push on new-code coverage (78.9%, required 80%); measured locally, the only uncovered new line in the library was the early return in
IsDeleteSelectionPressed. Nothing had ever exercised that guard, on this path or the link path it was moved from — and it is the one failure of this gesture that destroys work rather than merely doing nothing. The fixture draws a realImGui.InputTextand gives it the keyboard, sinceWantTextInputis only raised while a text widget is active and cannot be set directly. Neutralising the guard fails the test.Reverting only the
ProcessSelectedNodeDeletioncall fails five of the eight; the three that still pass are the negative cases, which are meant to hold either way. The four pre-existing link tests pass unchanged.dotnet build ImGui.slnsucceeds with 0 warnings.Three lines remain uncovered in this diff: the body of the new node-deletion loop in
CleanImNodesDemo, which no test drives because nothing selects a node inside the demo's own UI suite.(The commit message on the first commit says "six tests ... five of the six". It was seven then and is eight now; the counts in this description are the correct ones.)
tests/ImGuiAppDemo.UITestscould not be run here:icon.pngis a Git LFS object that this container's clone did not fetch, so the demo'sOnStart()fails to decode it before any test body runs. That is environmental and predates this branch — CI runs the suite properly and it passed on all three platforms.🤖 Generated with Claude Code
https://claude.ai/code/session_01KPZJNads6wQ5xqenyAh3mp