From 9463772077b2a2da0c9b58e3c72c1aad2f15030c Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 02:27:44 +0000 Subject: [PATCH 1/2] fix: stop treating position -1 as clean after trimming or saving [patch] HasUnsavedChanges assumed position -1 was the untouched initial state whenever no boundaries existed, so it reported false after MaxStackSize trimming shifted a never-saved edit there, or after a branch removed the only save point. AdjustPositions also discarded a boundary shifted to exactly -1, although -1 is a reachable position. Track whether the initial state is still clean explicitly: it is on construction and Clear(), and stops being so once anything is saved or commands are trimmed. Keep boundaries that land on -1. Fixes ktsu-dev/UndoRedo#76 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01L2BLMsT5ih3DnMNxTyGUHh --- UndoRedo.Test/UndoRedoStackTests.cs | 65 ++++++++++++++++++++++++ UndoRedo/Services/SaveBoundaryManager.cs | 26 ++++++++-- 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index 0cdee0d..548c871 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -545,6 +545,71 @@ public void Execute_MergedCommandThrowsAndRestoreAlsoThrows_SurfacesTheOriginalF Assert.IsTrue(stack.CanUndo, "CanUndo should remain true after a failed merge"); } + [TestMethod] + public void HasUnsavedChanges_AtStartAfterTrimming_IsTrue() + { + // Arrange: B trims A, so position -1 now holds A's never-saved result + UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 1)); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 1)); + + // Act + stack.Undo(); + + // Assert + Assert.AreEqual(-1, stack.CurrentPosition); + Assert.AreEqual(1, value); + Assert.IsTrue(stack.HasUnsavedChanges, "Position -1 is not the initial state once trimming has shifted A's result there"); + } + + [TestMethod] + public void HasUnsavedChanges_AtStartAfterBranchRemovesTheOnlyBoundary_IsTrue() + { + // Arrange: A is saved, then undone and branched away from + UndoRedoService stack = CreateService(); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.MarkAsSaved(); + stack.Undo(); + Assert.IsTrue(stack.HasUnsavedChanges, "The saved file holds A, not the initial state"); + + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 0)); + Assert.IsEmpty(stack.SaveBoundaries, "The branch invalidates the boundary at A"); + + // Act + stack.Undo(); + + // Assert: the file on disk still holds A + Assert.AreEqual(-1, stack.CurrentPosition); + Assert.AreEqual(0, value); + Assert.IsTrue(stack.HasUnsavedChanges, "The initial state was never what was saved"); + } + + [TestMethod] + public void AdjustPositions_BoundaryShiftedToStart_IsKept() + { + // Arrange + UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 2)); + + stack.Execute(new DelegateCommand("A", () => { }, () => { })); + stack.MarkAsSaved("after A"); + stack.Execute(new DelegateCommand("B", () => { }, () => { })); + + // Act: C trims A, which shifts the save point after A to -1 + stack.Execute(new DelegateCommand("C", () => { }, () => { })); + + // Assert + Assert.HasCount(1, stack.SaveBoundaries, "A save point shifted to -1 is still reachable and must be kept"); + Assert.AreEqual(-1, stack.SaveBoundaries[0].Position); + + stack.Undo(); + stack.Undo(); + Assert.IsFalse(stack.HasUnsavedChanges, "Undoing back to the save point after A must report it as saved"); + } + [TestMethod] public async Task UndoToSaveBoundary_WhenAlreadyAtPosition_ReturnsFalse() { diff --git a/UndoRedo/Services/SaveBoundaryManager.cs b/UndoRedo/Services/SaveBoundaryManager.cs index b6a1ebb..e6e4bd1 100644 --- a/UndoRedo/Services/SaveBoundaryManager.cs +++ b/UndoRedo/Services/SaveBoundaryManager.cs @@ -11,16 +11,20 @@ public sealed class SaveBoundaryManager : ISaveBoundaryManager { private readonly List _saveBoundaries = []; + // Whether position -1 still holds the untouched initial state, which is clean without a boundary. + // It stops being true once anything is saved, since the saved state replaces it, and once trimming + // shifts later commands' results down to -1. + private bool _initialStateIsClean = true; + /// public IReadOnlyList SaveBoundaries => _saveBoundaries.AsReadOnly(); /// public bool HasUnsavedChanges(int currentPosition) { - // If no save boundaries exist, we have unsaved changes unless at initial position - if (_saveBoundaries.Count == 0) + if (currentPosition == -1 && _initialStateIsClean) { - return currentPosition >= 0; + return false; } // No unsaved changes if we're exactly at a save boundary position @@ -32,6 +36,7 @@ public SaveBoundary CreateSaveBoundary(int position, string? description = null) { SaveBoundary saveBoundary = new(position, description); _saveBoundaries.Add(saveBoundary); + _initialStateIsClean = false; return saveBoundary; } @@ -58,12 +63,19 @@ public void AdjustPositions(int adjustment) return; } + if (adjustment < 0) + { + // Commands were trimmed from the bottom, so -1 is now the state after them, not the initial one + _initialStateIsClean = false; + } + for (int i = _saveBoundaries.Count - 1; i >= 0; i--) { SaveBoundary boundary = _saveBoundaries[i]; int newPosition = boundary.Position + adjustment; - if (newPosition < 0) + // -1 is a reachable position, so a boundary shifted exactly there is still a valid save point + if (newPosition < -1) { _saveBoundaries.RemoveAt(i); } @@ -90,5 +102,9 @@ public IEnumerable GetCommandsToUndo(SaveBoundary saveBoundary, int cu } /// - public void Clear() => _saveBoundaries.Clear(); + public void Clear() + { + _saveBoundaries.Clear(); + _initialStateIsClean = true; + } } From 8cad8bbc42255155bd5cb37e7bcf444a7e34e964 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 26 Sep 2026 02:29:59 +0000 Subject: [PATCH 2/2] test: group the unsaved-changes tests with the stack-size tests [patch] Keeps them clear of the exception-safety tests added alongside, so the two PRs merge independently. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01L2BLMsT5ih3DnMNxTyGUHh --- UndoRedo.Test/UndoRedoStackTests.cs | 130 ++++++++++++++-------------- 1 file changed, 65 insertions(+), 65 deletions(-) diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index 548c871..202862f 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -273,6 +273,71 @@ public void Execute_MaxStackSizeReached_RemovesOldestCommands() Assert.AreEqual("Command 5", stack.Commands[2].Description); // Newest command } + [TestMethod] + public void HasUnsavedChanges_AtStartAfterTrimming_IsTrue() + { + // Arrange: B trims A, so position -1 now holds A's never-saved result + UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 1)); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 1)); + + // Act + stack.Undo(); + + // Assert + Assert.AreEqual(-1, stack.CurrentPosition); + Assert.AreEqual(1, value); + Assert.IsTrue(stack.HasUnsavedChanges, "Position -1 is not the initial state once trimming has shifted A's result there"); + } + + [TestMethod] + public void HasUnsavedChanges_AtStartAfterBranchRemovesTheOnlyBoundary_IsTrue() + { + // Arrange: A is saved, then undone and branched away from + UndoRedoService stack = CreateService(); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.MarkAsSaved(); + stack.Undo(); + Assert.IsTrue(stack.HasUnsavedChanges, "The saved file holds A, not the initial state"); + + stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 0)); + Assert.IsEmpty(stack.SaveBoundaries, "The branch invalidates the boundary at A"); + + // Act + stack.Undo(); + + // Assert: the file on disk still holds A + Assert.AreEqual(-1, stack.CurrentPosition); + Assert.AreEqual(0, value); + Assert.IsTrue(stack.HasUnsavedChanges, "The initial state was never what was saved"); + } + + [TestMethod] + public void AdjustPositions_BoundaryShiftedToStart_IsKept() + { + // Arrange + UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 2)); + + stack.Execute(new DelegateCommand("A", () => { }, () => { })); + stack.MarkAsSaved("after A"); + stack.Execute(new DelegateCommand("B", () => { }, () => { })); + + // Act: C trims A, which shifts the save point after A to -1 + stack.Execute(new DelegateCommand("C", () => { }, () => { })); + + // Assert + Assert.HasCount(1, stack.SaveBoundaries, "A save point shifted to -1 is still reachable and must be kept"); + Assert.AreEqual(-1, stack.SaveBoundaries[0].Position); + + stack.Undo(); + stack.Undo(); + Assert.IsFalse(stack.HasUnsavedChanges, "Undoing back to the save point after A must report it as saved"); + } + [TestMethod] public void CommandMerging_ConsecutiveCommands_MergesCorrectly() { @@ -545,71 +610,6 @@ public void Execute_MergedCommandThrowsAndRestoreAlsoThrows_SurfacesTheOriginalF Assert.IsTrue(stack.CanUndo, "CanUndo should remain true after a failed merge"); } - [TestMethod] - public void HasUnsavedChanges_AtStartAfterTrimming_IsTrue() - { - // Arrange: B trims A, so position -1 now holds A's never-saved result - UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 1)); - int value = 0; - - stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); - stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 1)); - - // Act - stack.Undo(); - - // Assert - Assert.AreEqual(-1, stack.CurrentPosition); - Assert.AreEqual(1, value); - Assert.IsTrue(stack.HasUnsavedChanges, "Position -1 is not the initial state once trimming has shifted A's result there"); - } - - [TestMethod] - public void HasUnsavedChanges_AtStartAfterBranchRemovesTheOnlyBoundary_IsTrue() - { - // Arrange: A is saved, then undone and branched away from - UndoRedoService stack = CreateService(); - int value = 0; - - stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); - stack.MarkAsSaved(); - stack.Undo(); - Assert.IsTrue(stack.HasUnsavedChanges, "The saved file holds A, not the initial state"); - - stack.Execute(new DelegateCommand("B", () => value = 2, () => value = 0)); - Assert.IsEmpty(stack.SaveBoundaries, "The branch invalidates the boundary at A"); - - // Act - stack.Undo(); - - // Assert: the file on disk still holds A - Assert.AreEqual(-1, stack.CurrentPosition); - Assert.AreEqual(0, value); - Assert.IsTrue(stack.HasUnsavedChanges, "The initial state was never what was saved"); - } - - [TestMethod] - public void AdjustPositions_BoundaryShiftedToStart_IsKept() - { - // Arrange - UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), UndoRedoOptions.Create(maxStackSize: 2)); - - stack.Execute(new DelegateCommand("A", () => { }, () => { })); - stack.MarkAsSaved("after A"); - stack.Execute(new DelegateCommand("B", () => { }, () => { })); - - // Act: C trims A, which shifts the save point after A to -1 - stack.Execute(new DelegateCommand("C", () => { }, () => { })); - - // Assert - Assert.HasCount(1, stack.SaveBoundaries, "A save point shifted to -1 is still reachable and must be kept"); - Assert.AreEqual(-1, stack.SaveBoundaries[0].Position); - - stack.Undo(); - stack.Undo(); - Assert.IsFalse(stack.HasUnsavedChanges, "Undoing back to the save point after A must report it as saved"); - } - [TestMethod] public async Task UndoToSaveBoundary_WhenAlreadyAtPosition_ReturnsFalse() {