diff --git a/UndoRedo.Test/CompositeCommandTests.cs b/UndoRedo.Test/CompositeCommandTests.cs index 3223990..be8769d 100644 --- a/UndoRedo.Test/CompositeCommandTests.cs +++ b/UndoRedo.Test/CompositeCommandTests.cs @@ -6,6 +6,7 @@ namespace ktsu.UndoRedo.Test; using System.Collections.Generic; using System.Linq; using ktsu.UndoRedo; +using ktsu.UndoRedo.Core.Services; [TestClass] public class CompositeCommandTests @@ -74,7 +75,7 @@ public void CompositeCommand_NestedFailure_RollsBackCompleteHierarchy() private static readonly string[] expected = ["A", "B", "C"]; [TestMethod] - public void CompositeCommand_UndoFailure_DoesNotAffectOtherCommands() + public void CompositeCommand_UndoFailure_LeavesEveryCommandApplied() { // Arrange List values = []; @@ -104,12 +105,50 @@ public void CompositeCommand_UndoFailure_DoesNotAffectOtherCommands() // Make undo fail for middle command shouldFailUndo = true; - // Assert - Undo should throw but still attempt to undo all commands + // Assert - Undo should throw and restore C, which it had already undone Assert.ThrowsExactly(composite.Undo); + CollectionAssert.AreEqual(expected, values, "A failed Undo should leave the composite fully applied"); + } + + [TestMethod] + public void CompositeCommand_UndoFailureThenRetry_UndoesEachCommandOnce() + { + // Arrange: b's undo fails once + UndoRedoService stack = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger()); + int a = 0; + int b = 0; + bool failUndo = true; + + CompositeCommand composite = new("Move and resize", + [ + new DelegateCommand("a", () => a++, () => a--), + new DelegateCommand("b", () => b++, () => + { + if (failUndo) + { + throw new InvalidOperationException("b undo fails"); + } + + b--; + }), + ]); + + stack.Execute(composite); + + // Act + Assert.ThrowsExactly(() => stack.Undo()); + + // Assert: the stack did not move, so nothing may have been undone + Assert.AreEqual(0, stack.CurrentPosition); + Assert.AreEqual(1, a, "A failed Undo should leave a applied"); + Assert.AreEqual(1, b, "A failed Undo should leave b applied"); + + failUndo = false; + stack.Undo(); - // Commands should still be partially undone (C and A undone from end, B failed) - Assert.HasCount(1, values); - Assert.AreEqual("A", values[0]); // A remains because B's undo failed, C was undone, A tried to undo but only removes from end + Assert.AreEqual(-1, stack.CurrentPosition); + Assert.AreEqual(0, a, "Retrying Undo should undo a exactly once"); + Assert.AreEqual(0, b, "Retrying Undo should undo b exactly once"); } [TestMethod] diff --git a/UndoRedo/CompositeCommand.cs b/UndoRedo/CompositeCommand.cs index 5f66bf7..821e14f 100644 --- a/UndoRedo/CompositeCommand.cs +++ b/UndoRedo/CompositeCommand.cs @@ -75,28 +75,37 @@ public override void Execute() /// public override void Undo() { - List undoExceptions = []; + List undoneCommands = []; - // Undo in reverse order, collecting any exceptions - for (int i = _commands.Count - 1; i >= 0; i--) + try { -#pragma warning disable CA1031 // Do not catch general exception types - try + // Undo in reverse order + for (int i = _commands.Count - 1; i >= 0; i--) { _commands[i].Undo(); + undoneCommands.Add(_commands[i]); } - catch (Exception ex) - { - undoExceptions.Add(ex); - } -#pragma warning restore CA1031 // Do not catch general exception types } - - // If any undo operations failed, throw the first exception - if (undoExceptions.Count > 0) +#pragma warning disable CA1031 // Do not catch general exception types + catch (Exception) { - throw undoExceptions[0]; + // Re-apply what this call already undid, in forward order, so a failed Undo leaves the + // composite fully applied. The stack keeps its position when Undo throws, so it must be + // able to rely on nothing having been undone. This mirrors the rollback in Execute(). + for (int i = undoneCommands.Count - 1; i >= 0; i--) + { + try + { + undoneCommands[i].Execute(); + } + catch (Exception) + { + // Continue restoring even if an individual re-execute fails + } + } + throw; // Re-throw the original exception } +#pragma warning restore CA1031 // Do not catch general exception types } ///