From eb780fdd15efb722486a3b63fa3ad13a9c0cecaf Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Sun, 27 Sep 2026 06:26:45 +0000 Subject: [PATCH] fix: resolve a held save boundary to its live position before undoing to it [patch] Trimming replaced each SaveBoundary with a new object at the adjusted position, so a boundary a caller already held (from SaveBoundaryCreated, GetLastSaveBoundary or an earlier SaveBoundaries read) kept its old position. UndoToSaveBoundaryAsync and GetCommandsToUndo used that stale position as given and landed on an unsaved intermediate state. Adjusted boundaries now share an internal identity with the boundary they replace, and the service resolves its argument against the live boundaries. A boundary held across a trim undoes to its save point; one removed by branching or clearing is rejected. Fixes ktsu-dev/UndoRedo#88 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TX4nouJvSXNs7eSvDP13qU --- UndoRedo.Test/UndoRedoStackTests.cs | 53 ++++++++++++++++++++++++ UndoRedo/Models/SaveBoundary.cs | 21 ++++++++++ UndoRedo/Services/SaveBoundaryManager.cs | 5 ++- UndoRedo/Services/UndoRedoService.cs | 28 ++++++++++++- docs/api-reference.md | 4 +- 5 files changed, 105 insertions(+), 6 deletions(-) diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index ce23036..03f185c 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -752,6 +752,59 @@ await Assert.ThrowsExactlyAsync(() => Assert.IsTrue(stack.CanRedo, "C was undone, so it must be redoable"); } + [TestMethod] + public async Task UndoToSaveBoundary_BoundaryHeldAcrossTrim_UndoesToSavedState() + { + // Arrange + UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 3)); + int value = 0; + SaveBoundary? heldBoundary = null; + stack.SaveBoundaryCreated += (_, e) => heldBoundary = e.SaveBoundary; + + stack.Execute(new DelegateCommand("A", () => value++, () => value--)); + stack.MarkAsSaved(); + stack.Execute(new DelegateCommand("B", () => value++, () => value--)); + stack.Execute(new DelegateCommand("C", () => value++, () => value--)); + stack.Execute(new DelegateCommand("D", () => value++, () => value--)); // Trims A, moving the save point to -1 + Assert.IsNotNull(heldBoundary); + Assert.AreEqual(3, stack.GetCommandsToUndo(heldBoundary).Count(), "The held boundary should resolve to the save point's current position"); + + // Act + bool result = await stack.UndoToSaveBoundaryAsync(heldBoundary, navigateToLastChange: false).ConfigureAwait(false); + + // Assert + Assert.IsTrue(result, "UndoToSaveBoundary should resolve a boundary held across a trim"); + Assert.AreEqual(1, value, "The value should be back at the saved state"); + Assert.AreEqual(-1, stack.CurrentPosition); + Assert.IsFalse(stack.HasUnsavedChanges, "The stack should be at the save point"); + } + + [TestMethod] + public async Task UndoToSaveBoundary_BoundaryRemovedByBranching_ReturnsFalse() + { + // Arrange + UndoRedoService stack = CreateService(); + int value = 0; + stack.Execute(new DelegateCommand("A", () => value++, () => value--)); + stack.Execute(new DelegateCommand("B", () => value++, () => value--)); + stack.MarkAsSaved(); + SaveBoundary removedBoundary = stack.SaveBoundaries[0]; + await stack.UndoAsync(navigateToChange: false).ConfigureAwait(false); + await stack.UndoAsync(navigateToChange: false).ConfigureAwait(false); + stack.Execute(new DelegateCommand("C", () => value++, () => value--)); // Branches, discarding the save point + stack.Execute(new DelegateCommand("D", () => value++, () => value--)); + stack.Execute(new DelegateCommand("E", () => value++, () => value--)); + + // Act + bool result = await stack.UndoToSaveBoundaryAsync(removedBoundary, navigateToLastChange: false).ConfigureAwait(false); + + // Assert + Assert.IsFalse(result, "UndoToSaveBoundary should reject a boundary that no longer exists"); + Assert.AreEqual(3, value, "Nothing should have been undone"); + Assert.AreEqual(2, stack.CurrentPosition); + Assert.IsEmpty(stack.GetCommandsToUndo(removedBoundary)); + } + [TestMethod] public async Task UndoToSaveBoundary_WhenAlreadyAtPosition_ReturnsFalse() { diff --git a/UndoRedo/Models/SaveBoundary.cs b/UndoRedo/Models/SaveBoundary.cs index 992e086..4fd566a 100644 --- a/UndoRedo/Models/SaveBoundary.cs +++ b/UndoRedo/Models/SaveBoundary.cs @@ -9,6 +9,17 @@ namespace ktsu.UndoRedo; /// Optional description public sealed class SaveBoundary(int position, string? description = null) { + /// + /// Creates a copy of at a new position that is still the same save point, + /// so a caller holding the original can have it resolved to where the save point is now + /// + internal SaveBoundary(SaveBoundary original, int position) + : this(position, original.Description) + { + Identity = original.Identity; + Timestamp = original.Timestamp; + } + /// /// The position in the stack where this save boundary was created /// @@ -23,4 +34,14 @@ public sealed class SaveBoundary(int position, string? description = null) /// Optional description of what was saved /// public string? Description { get; } = description; + + /// + /// Shared by every copy of one save point as its position is adjusted + /// + internal object Identity { get; } = new(); + + /// + /// Whether this boundary and describe the same save point + /// + internal bool IsSameSavePointAs(SaveBoundary other) => ReferenceEquals(Identity, other.Identity); } diff --git a/UndoRedo/Services/SaveBoundaryManager.cs b/UndoRedo/Services/SaveBoundaryManager.cs index e6e4bd1..3d92029 100644 --- a/UndoRedo/Services/SaveBoundaryManager.cs +++ b/UndoRedo/Services/SaveBoundaryManager.cs @@ -81,8 +81,9 @@ public void AdjustPositions(int adjustment) } else { - // Create a new boundary with adjusted position - _saveBoundaries[i] = new SaveBoundary(newPosition, boundary.Description); + // Create a new boundary with adjusted position that is still the same save point, so a + // boundary a caller already holds can be resolved to it + _saveBoundaries[i] = new SaveBoundary(boundary, newPosition); } } } diff --git a/UndoRedo/Services/UndoRedoService.cs b/UndoRedo/Services/UndoRedoService.cs index 99b0f7a..406345e 100644 --- a/UndoRedo/Services/UndoRedoService.cs +++ b/UndoRedo/Services/UndoRedoService.cs @@ -237,14 +237,31 @@ public void Clear() } /// - public IEnumerable GetCommandsToUndo(SaveBoundary saveBoundary) => - _saveBoundaryManager.GetCommandsToUndo(saveBoundary, _stackManager.CurrentPosition, _stackManager.Commands); + public IEnumerable GetCommandsToUndo(SaveBoundary saveBoundary) + { + Ensure.NotNull(saveBoundary); + + SaveBoundary? liveBoundary = FindLiveSaveBoundary(saveBoundary); + return liveBoundary == null + ? [] + : _saveBoundaryManager.GetCommandsToUndo(liveBoundary, _stackManager.CurrentPosition, _stackManager.Commands); + } /// public async Task UndoToSaveBoundaryAsync(SaveBoundary saveBoundary, bool navigateToLastChange = true, CancellationToken cancellationToken = default) { Ensure.NotNull(saveBoundary); + // A boundary the caller has held since before the stack was trimmed carries a stale position, + // and one removed by branching or clearing no longer marks a reachable saved state + SaveBoundary? liveBoundary = FindLiveSaveBoundary(saveBoundary); + if (liveBoundary == null) + { + return false; + } + + saveBoundary = liveBoundary; + if (_stackManager.CurrentPosition <= saveBoundary.Position) { return false; @@ -285,6 +302,13 @@ public async Task UndoToSaveBoundaryAsync(SaveBoundary saveBoundary, bool return true; } + /// + /// Resolves a save boundary, which may have been obtained before the stack was trimmed, to the live + /// boundary for the same save point, or null if that save point no longer exists + /// + private SaveBoundary? FindLiveSaveBoundary(SaveBoundary saveBoundary) => + _saveBoundaryManager.SaveBoundaries.FirstOrDefault(boundary => boundary.IsSameSavePointAs(saveBoundary)); + /// public IEnumerable GetChangeVisualizations(int maxItems = 50) { diff --git a/docs/api-reference.md b/docs/api-reference.md index 48fdfef..f88c1c8 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -278,7 +278,7 @@ LoadNewDocument(); ```csharp IEnumerable GetCommandsToUndo(SaveBoundary saveBoundary); ``` -Gets commands that would be undone to reach the specified save boundary. +Gets commands that would be undone to reach the specified save boundary. Returns nothing for a boundary whose save point no longer exists. **Parameters:** - `saveBoundary`: The target save boundary @@ -292,7 +292,7 @@ Gets commands that would be undone to reach the specified save boundary. ```csharp Task UndoToSaveBoundaryAsync(SaveBoundary saveBoundary, bool navigateToLastChange = true, CancellationToken cancellationToken = default); ``` -Undoes commands until reaching the specified save boundary. +Undoes commands until reaching the specified save boundary. A boundary obtained earlier, for example from `SaveBoundaryCreated`, still resolves to its save point after `MaxStackSize` trims the stack. A boundary whose save point no longer exists, because a new branch or `Clear()` removed it, is rejected and nothing is undone. **Parameters:** - `saveBoundary`: The target save boundary