From 345942b443223448f1bdb0ab98e5ec7aec8fe578 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 17 Jun 2026 20:17:00 +0000 Subject: [PATCH] Refactor: generalize CanvasHistory into BoundedHistory for image undo too The image-level undo/redo was still two loose @State [NSImage] arrays with inline push/cap-at-20/pop logic in EditorView, duplicating the rules just extracted for the annotation canvas. Generalize CanvasHistory into BoundedHistory (CanvasHistory becomes a typealias, so #6's usage and tests are unchanged) and back the image stack with BoundedHistory(limit: 20). Behavior-preserving: image and canvas histories are still popped independently in undo()/redo() (image best-effort, canvas guarded). Adds BoundedHistoryTests (4 cases) pinning the generic behavior with a non-CanvasState type. --- .../Stag/Views/Editor/BoundedHistory.swift | 45 ++++++++++++++++++ Sources/Stag/Views/Editor/CanvasHistory.swift | 42 ----------------- Sources/Stag/Views/Editor/EditorView.swift | 19 +++----- Tests/StagTests/BoundedHistoryTests.swift | 46 +++++++++++++++++++ 4 files changed, 98 insertions(+), 54 deletions(-) create mode 100644 Sources/Stag/Views/Editor/BoundedHistory.swift delete mode 100644 Sources/Stag/Views/Editor/CanvasHistory.swift create mode 100644 Tests/StagTests/BoundedHistoryTests.swift diff --git a/Sources/Stag/Views/Editor/BoundedHistory.swift b/Sources/Stag/Views/Editor/BoundedHistory.swift new file mode 100644 index 0000000..d2e8c45 --- /dev/null +++ b/Sources/Stag/Views/Editor/BoundedHistory.swift @@ -0,0 +1,45 @@ +import Foundation + +/// Bounded undo/redo history for editor state. Owns two stacks of snapshots and +/// the push/cap/clear-redo and pop/swap rules that previously lived as loose +/// `@State` arrays inside `EditorView`. Recording a new snapshot clears the redo +/// stack; `undo`/`redo` swap the supplied "current" snapshot across the stacks +/// and return the snapshot the caller should apply (or `nil`). +struct BoundedHistory { + private(set) var undoStack: [Snapshot] = [] + private(set) var redoStack: [Snapshot] = [] + let limit: Int + + init(limit: Int = 100) { self.limit = limit } + + var canUndo: Bool { !undoStack.isEmpty } + var canRedo: Bool { !redoStack.isEmpty } + + /// Pushes a snapshot onto the undo stack (capped at `limit`) and clears redo. + mutating func record(_ snapshot: Snapshot) { + undoStack.append(snapshot) + if undoStack.count > limit { + undoStack.removeFirst(undoStack.count - limit) + } + redoStack.removeAll() + } + + /// Pops the previous snapshot, pushing `current` onto the redo stack. Returns + /// the snapshot to apply, or `nil` when there is nothing to undo. + mutating func undo(current: Snapshot) -> Snapshot? { + guard let previous = undoStack.popLast() else { return nil } + redoStack.append(current) + return previous + } + + /// Pops the next redo snapshot, pushing `current` onto the undo stack. Returns + /// the snapshot to apply, or `nil` when there is nothing to redo. + mutating func redo(current: Snapshot) -> Snapshot? { + guard let next = redoStack.popLast() else { return nil } + undoStack.append(current) + return next + } +} + +/// The editor's annotation-canvas undo/redo history. +typealias CanvasHistory = BoundedHistory diff --git a/Sources/Stag/Views/Editor/CanvasHistory.swift b/Sources/Stag/Views/Editor/CanvasHistory.swift deleted file mode 100644 index d402398..0000000 --- a/Sources/Stag/Views/Editor/CanvasHistory.swift +++ /dev/null @@ -1,42 +0,0 @@ -import Foundation - -/// Bounded undo/redo history for the editor's annotation canvas. Owns the two -/// `CanvasState` stacks that previously lived as loose `@State` arrays inside -/// `EditorView`, along with the push/cap/clear-redo rule. Recording a new state -/// clears the redo stack; `undo`/`redo` swap the supplied "current" state across -/// the stacks and return the state the caller should apply (or `nil`). -struct CanvasHistory { - private(set) var undoStack: [CanvasState] = [] - private(set) var redoStack: [CanvasState] = [] - let limit: Int - - init(limit: Int = 100) { self.limit = limit } - - var canUndo: Bool { !undoStack.isEmpty } - var canRedo: Bool { !redoStack.isEmpty } - - /// Pushes a snapshot onto the undo stack (capped at `limit`) and clears redo. - mutating func record(_ state: CanvasState) { - undoStack.append(state) - if undoStack.count > limit { - undoStack.removeFirst(undoStack.count - limit) - } - redoStack.removeAll() - } - - /// Pops the previous state, pushing `current` onto the redo stack. Returns the - /// state to apply, or `nil` when there is nothing to undo. - mutating func undo(current: CanvasState) -> CanvasState? { - guard let previous = undoStack.popLast() else { return nil } - redoStack.append(current) - return previous - } - - /// Pops the next redo state, pushing `current` onto the undo stack. Returns the - /// state to apply, or `nil` when there is nothing to redo. - mutating func redo(current: CanvasState) -> CanvasState? { - guard let next = redoStack.popLast() else { return nil } - undoStack.append(current) - return next - } -} diff --git a/Sources/Stag/Views/Editor/EditorView.swift b/Sources/Stag/Views/Editor/EditorView.swift index cddc76e..6c43c4d 100644 --- a/Sources/Stag/Views/Editor/EditorView.swift +++ b/Sources/Stag/Views/Editor/EditorView.swift @@ -73,8 +73,7 @@ struct EditorView: View { @State private var rotation: CGFloat = 0 // Image undo stack (for destructive edits) - @State private var imageUndoStack: [NSImage] = [] - @State private var imageRedoStack: [NSImage] = [] + @State private var imageHistory = BoundedHistory(limit: 20) @State private var cloneStampSource: CGPoint? @State private var removeBgRunning = false @State private var eraserSwipePoints: [CGPoint] = [] @@ -1230,16 +1229,13 @@ struct EditorView: View { } private func pushUndoImage() { - imageUndoStack.append(workingImage) - if imageUndoStack.count > 20 { imageUndoStack.removeFirst() } - imageRedoStack.removeAll() + imageHistory.record(workingImage) pushUndo() } private func undo() { - if !imageUndoStack.isEmpty { - imageRedoStack.append(workingImage) - workingImage = imageUndoStack.removeLast() + if let image = imageHistory.undo(current: workingImage) { + workingImage = image } let current = CanvasState(annotations: annotations, currentTool: currentTool, selectedAnnotationId: selectedAnnotationId, rotation: rotation) guard let state = canvasHistory.undo(current: current) else { return } @@ -1250,9 +1246,8 @@ struct EditorView: View { } private func redo() { - if !imageRedoStack.isEmpty { - imageUndoStack.append(workingImage) - workingImage = imageRedoStack.removeLast() + if let image = imageHistory.redo(current: workingImage) { + workingImage = image } let current = CanvasState(annotations: annotations, currentTool: currentTool, selectedAnnotationId: selectedAnnotationId, rotation: rotation) guard let state = canvasHistory.redo(current: current) else { return } @@ -1853,7 +1848,7 @@ struct EditorView: View { /// True when the user has made any change worth persisting. private var hasEdits: Bool { - !annotations.isEmpty || rotation != 0 || backdrop.isActive || !imageUndoStack.isEmpty + !annotations.isEmpty || rotation != 0 || backdrop.isActive || imageHistory.canUndo } /// Writes the edited image back to its source file synchronously (for ⌘S and safety-net onDisappear). diff --git a/Tests/StagTests/BoundedHistoryTests.swift b/Tests/StagTests/BoundedHistoryTests.swift new file mode 100644 index 0000000..8a4469f --- /dev/null +++ b/Tests/StagTests/BoundedHistoryTests.swift @@ -0,0 +1,46 @@ +import XCTest +@testable import Stag + +/// CanvasHistoryTests already exercises the history rules via the +/// `CanvasHistory = BoundedHistory` typealias. These tests pin the +/// generic behavior with a value type other than CanvasState (mirroring the +/// image-undo use, BoundedHistory) so the generalization can't regress. +final class BoundedHistoryTests: XCTestCase { + + func testGenericRecordUndoRedoRoundTrip() { + var h = BoundedHistory() + h.record(1) + XCTAssertTrue(h.canUndo) + XCTAssertEqual(h.undo(current: 2), 1) // returns previous, 2 -> redo + XCTAssertFalse(h.canUndo) + XCTAssertTrue(h.canRedo) + XCTAssertEqual(h.redo(current: 1), 2) // returns swapped, 1 -> undo + XCTAssertTrue(h.canUndo) + } + + func testRecordingClearsRedo() { + var h = BoundedHistory() + h.record(1) + _ = h.undo(current: 2) + XCTAssertTrue(h.canRedo) + h.record(3) + XCTAssertFalse(h.canRedo) + } + + func testEmptyReturnsNil() { + var h = BoundedHistory() + XCTAssertNil(h.undo(current: 0)) + XCTAssertNil(h.redo(current: 0)) + } + + func testCapDropsOldest() { + var h = BoundedHistory(limit: 20) + for i in 1...25 { h.record(i) } // retains 6...25 + var popped: [Int] = [] + var current = 999 + while let v = h.undo(current: current) { popped.append(v); current = v } + XCTAssertEqual(popped.count, 20) + XCTAssertEqual(popped.first, 25) // most recent first + XCTAssertEqual(popped.last, 6) // 1...5 were dropped + } +}