diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index 0cdee0d..910e2d4 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -545,6 +545,99 @@ public void Execute_MergedCommandThrowsAndRestoreAlsoThrows_SurfacesTheOriginalF Assert.IsTrue(stack.CanUndo, "CanUndo should remain true after a failed merge"); } + [TestMethod] + public void Undo_CommandThrowsException_LeavesPositionUnchanged() + { + // Arrange: B's Undo fails the first time it is attempted + UndoRedoService stack = CreateService(); + int value = 0; + bool failUndo = true; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.Execute(new DelegateCommand("B", () => value = 2, () => + { + if (failUndo) + { + throw new InvalidOperationException(); + } + + value = 1; + })); + + // Act + Assert.ThrowsExactly(() => stack.Undo()); + + // Assert: nothing was undone, so the stack must not have moved + Assert.AreEqual(1, stack.CurrentPosition, "A failed Undo must not move the stack position"); + Assert.IsTrue(stack.CanUndo, "CanUndo should be unchanged after a failed Undo"); + Assert.IsFalse(stack.CanRedo, "A command that failed to undo must not become redoable"); + Assert.AreEqual(2, value); + + // And the next Undo must retry B rather than skip over it to A + failUndo = false; + stack.Undo(); + Assert.AreEqual(1, value, "Undo after a failed Undo must undo B, not skip to A"); + Assert.AreEqual(0, stack.CurrentPosition); + } + + [TestMethod] + public void Redo_CommandThrowsException_LeavesPositionUnchanged() + { + // Arrange: A's Execute fails once it is redone + UndoRedoService stack = CreateService(); + int value = 0; + bool failExecute = false; + + stack.Execute(new DelegateCommand("A", () => + { + if (failExecute) + { + throw new InvalidOperationException(); + } + + value = 1; + }, () => value = 0)); + stack.Undo(); + failExecute = true; + + // Act + Assert.ThrowsExactly(() => stack.Redo()); + + // Assert: nothing was reapplied, so the stack must not have moved + Assert.AreEqual(-1, stack.CurrentPosition, "A failed Redo must not move the stack position"); + Assert.IsFalse(stack.CanUndo, "A command that failed to redo must not become undoable"); + Assert.IsTrue(stack.CanRedo, "The command that failed to redo must still be redoable"); + Assert.AreEqual(0, value); + + // And the failed command can be redone once it succeeds + failExecute = false; + stack.Redo(); + Assert.AreEqual(1, value); + Assert.AreEqual(0, stack.CurrentPosition); + } + + [TestMethod] + public async Task UndoToSaveBoundary_CommandThrowsException_StopsWithAccuratePosition() + { + // Arrange: save at A, then B and C, where B's Undo throws + UndoRedoService stack = CreateService(); + int value = 0; + + stack.Execute(new DelegateCommand("A", () => value = 1, () => value = 0)); + stack.MarkAsSaved("after A"); + stack.Execute(new DelegateCommand("B", () => value = 2, () => throw new InvalidOperationException())); + stack.Execute(new DelegateCommand("C", () => value = 3, () => value = 2)); + + // Act + await Assert.ThrowsExactlyAsync(() => + stack.UndoToSaveBoundaryAsync(stack.SaveBoundaries[0], navigateToLastChange: false)).ConfigureAwait(false); + + // Assert: C was undone, B was not, and the position says exactly that + Assert.AreEqual(2, value); + Assert.AreEqual(1, stack.CurrentPosition, "The position must stop on the command that failed to undo"); + Assert.IsTrue(stack.CanRedo, "C was undone, so it must be redoable"); + } + [TestMethod] public async Task UndoToSaveBoundary_WhenAlreadyAtPosition_ReturnsFalse() { diff --git a/UndoRedo/Services/UndoRedoService.cs b/UndoRedo/Services/UndoRedoService.cs index 0eb0f14..6f7ed9b 100644 --- a/UndoRedo/Services/UndoRedoService.cs +++ b/UndoRedo/Services/UndoRedoService.cs @@ -149,13 +149,16 @@ public void Execute(ICommand command) /// public async Task UndoAsync(bool navigateToChange = true, CancellationToken cancellationToken = default) { - ICommand? command = _stackManager.MovePrevious(); + ICommand? command = _stackManager.GetCurrentCommand(); if (command == null) { return false; } + // Undo the real state change before moving the position, so a command that throws leaves the + // stack describing what is actually applied. This mirrors Execute() above. command.Undo(); + _stackManager.MovePrevious(); CommandUndone?.Invoke(this, new CommandUndoneEventArgs(command, _stackManager.CurrentPosition)); if (navigateToChange && _options.EnableNavigation && _navigationProvider != null && !string.IsNullOrEmpty(command.NavigationContext)) @@ -182,13 +185,16 @@ public async Task UndoAsync(bool navigateToChange = true, CancellationToke /// public async Task RedoAsync(bool navigateToChange = true, CancellationToken cancellationToken = default) { - ICommand? command = _stackManager.MoveNext(); - if (command == null) + if (!_stackManager.CanRedo) { return false; } + // Reapply the real state change before moving the position, so a command that throws can still + // be redone and is never undone without having been applied. This mirrors Execute() above. + ICommand command = _stackManager.Commands[_stackManager.CurrentPosition + 1]; command.Execute(); + _stackManager.MoveNext(); CommandRedone?.Invoke(this, new CommandRedoneEventArgs(command, _stackManager.CurrentPosition)); if (navigateToChange && _options.EnableNavigation && _navigationProvider != null && !string.IsNullOrEmpty(command.NavigationContext)) @@ -243,13 +249,15 @@ public async Task UndoToSaveBoundaryAsync(SaveBoundary saveBoundary, bool ICommand? lastCommand = null; while (_stackManager.CurrentPosition > saveBoundary.Position) { - ICommand? command = _stackManager.MovePrevious(); + ICommand? command = _stackManager.GetCurrentCommand(); if (command == null) { break; } + // Stop at the first failure with the position still on the command that failed to undo command.Undo(); + _stackManager.MovePrevious(); lastCommand = command; CommandUndone?.Invoke(this, new CommandUndoneEventArgs(command, _stackManager.CurrentPosition)); }