Skip to content

Back SimpleUndoRedoProvider with ktsu.UndoRedo [minor] - #68

Merged
matt-edmondson merged 1 commit into
mainfrom
claude/undoredo-adapter
Sep 29, 2026
Merged

matt-edmondson merged 1 commit into
mainfrom
claude/undoredo-adapter

Conversation

@matt-edmondson

Copy link
Copy Markdown
Contributor

This implements the 2026-09-28 decision on #46 (option 2: accept the DI dependency). SimpleUndoRedoProvider is now an adapter that keeps IUndoRedoProvider and is backed by ktsu.UndoRedo.Core 1.1.4's UndoRedoService. The public contract is unchanged: IUndoRedoProvider, IUndoableAction, the constructor and GetUndoStack/GetRedoStack.

⚠️ One finding that changes what the decision accepted

The decision accepted that "a throwing Undo loses its history entry". I re-measured this against 1.1.4, and the entry is not lost. UndoRedoService moves its position before it runs the command. So on its own:

  • A failed undo leaves the action on the redo stack, as though it had worked. The next Redo() then runs Execute() on an action that was never undone.
  • A failed redo likewise leaves the action on the undo stack.

The 2026-09-22 probe read "CanUndo is false afterward" as the command being gone. With a single command on the stack, that is what the cursor moving past it looks like.

That is a state inconsistency, not the history loss that was accepted. So the adapter compensates: on a throw, it moves the position back with a skipped Execute/Undo, then rethrows. The provider's failed-undo and failed-redo behaviour is therefore exactly what it was before: the action stays where it was. Both cases are pinned by tests.

This is local handling of an upstream gap, which the reuse rule would prefer fixed in ktsu.UndoRedo. It is about 20 lines. If UndoRedoService later restores its position on a throw, the compensation can go and the tests will still hold. If you would rather ship the raw 1.1.4 behaviour instead, remove the two catch blocks and flip the two tests.

The adapter

  • Skip-first-Execute wrapper: each registered IUndoableAction is wrapped in a private ICommand. UndoRedoService.Execute runs a command as it records it, but Navigation<T>.NavigateTo has already navigated by the time it registers.
  • Options: MaxStackSize = maxHistorySize, AutoMergeCommands off (navigation actions are never merged), EnableNavigation off.
  • Stack accessors: GetUndoStack() and GetRedoStack() are derived from Commands and CurrentPosition. They keep their old order: each list ends with the action the next undo or redo would run.
  • StateChanged: still raised once per register, undo, redo and clear, and not on a failed one.

Package graph (UndoRedo#109)

ktsu.UndoRedo.Core 1.1.4 depends on Microsoft.Extensions.DependencyInjection(.Abstractions), System.Memory, System.Text.Json and System.Threading.Tasks.Extensions. It has no reference to ktsu.Navigation, so Navigation → UndoRedo creates no cycle. ktsu.UndoRedo already defines its own INavigationProvider. That leaves both routes the decision names open for UndoRedo#109, and neither needs anything from this PR.

#57 (locking for NavigationStack) touches NavigationStack.cs, which this PR does not change.

Tests

SimpleUndoRedoProviderTests has 11 new tests:

  • registering without executing
  • empty undo and redo return false
  • stack order
  • a new action clears redo
  • history trimming drops the oldest
  • stack accessor order
  • Clear
  • StateChanged
  • constructor validation
  • failed undo keeps the action
  • failed redo keeps the action

The suite has 29 tests, all passing locally (net10.0). The full solution builds across every target, netstandard2.0 through net10.0.

This is a refactor, so all 29 tests also pass against the old List-based implementation, which is the point. Removing each piece of the adapter makes them fail:

  • Without the skip-first-Execute wrapper: 4 fail, including RegisterAction_DoesNotExecuteTheAction.
  • Without the failure compensation: both failed-undo and failed-redo tests fail.

docs/API.md and docs/Architecture.md now describe the adapter and its dependency.

Fixes #46

🤖 Generated with Claude Code

https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16


Generated by Claude Code

SimpleUndoRedoProvider now adapts IUndoRedoProvider onto ktsu.UndoRedo's
UndoRedoService instead of keeping its own List-based stacks. The public
contract (IUndoRedoProvider, IUndoableAction, the constructor, and the
GetUndoStack/GetRedoStack accessors) is unchanged.

- Each registered action is wrapped in a command that skips its first
  Execute. UndoRedoService.Execute runs a command as it records it, but
  Navigation<T>.NavigateTo has already navigated by the time it registers.
- UndoRedoService 1.1.4 moves its position before running a command. On its
  own, a failed undo would leave the action on the redo stack as though it
  had worked, and a failed redo would leave it on the undo stack. The adapter
  moves the position back without running the action again, so a throwing
  undo or redo still leaves the history where it was.
- Merging and change navigation are switched off. Navigation actions are
  never merged.

ktsu.UndoRedo.Core does not reference ktsu.Navigation, so no package cycle
exists.

New SimpleUndoRedoProviderTests pin the provider's behaviour: registering
without executing, stack order, redo truncation, history trimming, stack
accessors, StateChanged, and failed undo and redo.

Fixes #46

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TAvt7dvjcukkH3y6mtUL16
@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Delegate undo/redo to ktsu.UndoRedo

2 participants