What's wrong
SaveBoundaryManager.HasUnsavedChanges (UndoRedo/Services/SaveBoundaryManager.cs, ~line 23) returns false whenever the current position matches any save boundary:
return !_saveBoundaries.Any(boundary => boundary.Position == currentPosition);
MarkAsSaved adds a new boundary each time and never retires the earlier ones. After a second save, the first save point still counts as clean, even though the file on disk now holds the second save.
Failure scenario
int value = 0;
UndoRedoService s = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger());
s.Execute(new DelegateCommand("A", () => value = 1, () => value = 0));
s.MarkAsSaved("save 1"); // disk: 1
s.Execute(new DelegateCommand("B", () => value = 2, () => value = 1));
s.MarkAsSaved("save 2"); // disk: 2
s.Undo(); // document: 1
Assert.IsTrue(s.HasUnsavedChanges); // fails: HasUnsavedChanges == falseAn app that asks "Save changes?" only when HasUnsavedChanges is true, which is the pattern in docs/api-reference.md, closes without asking. The undo is lost, and the next time the file is opened it contains the change the user reverted. UndoToSaveBoundaryAsync(olderBoundary) fails the same way. Both cases were confirmed with a temporary test against main (71a1527).
The class's own reasoning says otherwise. The comment on _initialStateIsClean says the initial state stops being clean "once anything is saved, since the saved state replaces it". The same logic applies to earlier save points, but the code never applies it.
Note for triage: the existing test SaveBoundaries_MultipleUndoRedoOperations_MaintainsCorrectState (UndoRedo.Test/UndoRedoStackTests.cs:532) asserts the current behavior. That test and the documented contract ("unsaved changes since the last save boundary") disagree, and a maintainer needs to decide which one is right. This is related to #83 but separate from it: #83 is only about position -1.
Suggested fix
- Track the most recent save boundary, for example the identity of the last boundary
CreateSaveBoundary made.
HasUnsavedChanges(position) returns false only when that boundary still exists and Position == position.- If that boundary is removed (by branch cleanup or by trimming), every position is dirty. Do not fall back to an older boundary.
- Keep the older boundaries for
UndoToSaveBoundaryAsync and the visualization.
- Restore the latest boundary in
RestoreFromState. The last one in the list works, because boundaries are appended in order.
- Update the existing test and add a regression test for save, edit, save, undo.
What's wrong
SaveBoundaryManager.HasUnsavedChanges(UndoRedo/Services/SaveBoundaryManager.cs, ~line 23) returns false whenever the current position matches any save boundary:MarkAsSavedadds a new boundary each time and never retires the earlier ones. After a second save, the first save point still counts as clean, even though the file on disk now holds the second save.Failure scenario
An app that asks "Save changes?" only when
HasUnsavedChangesis true, which is the pattern indocs/api-reference.md, closes without asking. The undo is lost, and the next time the file is opened it contains the change the user reverted.UndoToSaveBoundaryAsync(olderBoundary)fails the same way. Both cases were confirmed with a temporary test againstmain(71a1527).The class's own reasoning says otherwise. The comment on
_initialStateIsCleansays the initial state stops being clean "once anything is saved, since the saved state replaces it". The same logic applies to earlier save points, but the code never applies it.Note for triage: the existing test
SaveBoundaries_MultipleUndoRedoOperations_MaintainsCorrectState(UndoRedo.Test/UndoRedoStackTests.cs:532) asserts the current behavior. That test and the documented contract ("unsaved changes since the last save boundary") disagree, and a maintainer needs to decide which one is right. This is related to #83 but separate from it: #83 is only about position -1.Suggested fix
CreateSaveBoundarymade.HasUnsavedChanges(position)returns false only when that boundary still exists andPosition == position.UndoToSaveBoundaryAsyncand the visualization.RestoreFromState. The last one in the list works, because boundaries are appended in order.