Enumerate a CompositeCommand's commands once [patch] - #110
Merged
matt-edmondson merged 1 commit intoSep 28, 2026
Merged
Conversation
The constructor enumerated its commands argument three times: once for the affected items, once for the total size and once for the command list. A single-pass sequence was empty by the third pass and threw "must contain at least one command", and a Select factory ran three times, so the metadata could describe different command instances from the ones that execute. The public constructor now materializes the sequence once and chains to a private constructor that computes the metadata from that list. A null element is rejected with ArgumentException instead of failing later. Fixes #108 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M9aefrfAYJpanQVrUuFpJh
|
matt-edmondson
deleted the
claude/undoredo-108-composite-single-enumeration
branch
September 28, 2026 01:39
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
2 participants
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 #108
Problem
The
CompositeCommandconstructor enumerated itscommandsargument three times:GetAffectedItemsGetTotalSize_commandsThis caused two bugs:
Selectfactory ran three times. The metadata could then describe different command instances from the ones thatExecuteandUndorun.Change
All in
UndoRedo/CompositeCommand.cs:Materializehelper, which reads the sequence once into a list. It then chains to a private constructor that computes the affected items and size from that same list and keeps it as_commands.Materializedoes the null check and the empty check before the base constructor runs. It also rejects a null element withArgumentException, as the triage asked. Before, a null element failed later with aNullReferenceExceptionwhile the metadata was being computed.GetAffectedItemsandGetTotalSizeare removed. The public API is unchanged.Tests
New tests in
CompositeCommandTests:CompositeCommand_SinglePassSequence_KeepsEveryCommandAndItsMetadatauses a sequence that yields only on its first enumeration. It checks the commands, that they execute, the affected items and the size.CompositeCommand_LazyFactory_RunsOncePerCommandchecks that a countingSelectfactory runs exactly 3 times for 3 commands.CompositeCommand_NullElement_ThrowsArgumentExceptionWith
CompositeCommand.csreverted, all 3 new tests fail. With the fix, the full suite passes (97/97 on net10.0), and the library builds for every target.This PR doesn't touch the files changed by the open PRs #101, #106 or #107.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M9aefrfAYJpanQVrUuFpJh
Generated by Claude Code