diff --git a/Examples/UndoForMacOS/UndoForMacOSApp.swift b/Examples/UndoForMacOS/UndoForMacOSApp.swift index 8546116..12efaa5 100644 --- a/Examples/UndoForMacOS/UndoForMacOSApp.swift +++ b/Examples/UndoForMacOS/UndoForMacOSApp.swift @@ -5,11 +5,6 @@ import SwiftUI @main struct UndoForMacOSApp: App { - static let store = Store( - initialState: DemoFeature.State() - ) { - DemoFeature() - } init() { prepareDependencies { let database = try! makeDemoDatabase() @@ -22,17 +17,44 @@ struct UndoForMacOSApp: App { } var body: some Scene { WindowGroup { - DemoView(store: Self.store) + DemoWindow() + } + } +} + +/// One window's worth of state. +/// +/// The database and engine are app-wide, but each window gets its own `UndoStack`, +/// which binds to that window's UndoManager. Barriers opened by this window's store +/// register with that manager, and only this window sees the resulting undo events. +/// Open a second window with ⌘N to see the two undo stacks operate independently. +struct DemoWindow: View { + @State private var store = withDependencies { + $0.installDefaultUndoStack() + } operation: { + Store(initialState: DemoFeature.State()) { + DemoFeature() } } + var body: some View { + DemoView(store: store) + } } @Reducer struct DemoFeature { @ObservableState struct State { - @FetchAll(DemoItem.all) var items: [DemoItem] + let windowID: UUID var eventLog: [UndoEvent] = [] + @FetchAll(DemoItem.none) var items: [DemoItem] + init(windowID: UUID = UUID()) { + self._items = FetchAll( + wrappedValue: [], + DemoItem.all.where { $0.windowID.eq(windowID) } + ) + self.windowID = windowID + } } enum Action: UndoManageableAction { @@ -56,6 +78,7 @@ struct DemoFeature { case .undoManager(.event(let event)): if let ids = event.ids(for: DemoItem.self) { print( + "window: \(state.windowID) received undo:", event.kind, event.name.debugDescription, ids.map { $0.formatted() } @@ -71,18 +94,22 @@ struct DemoFeature { try undoable("Add Item") { try database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { + DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") + }.execute(db) } } } return .none case .addItemInBackground: - return .run { _ in + return .run { [windowID = state.windowID] _ in try await undoable("Add Item (Background)") { try await database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { + DemoItem(id: nextID, windowID: windowID, name: "Item \(nextID)") + }.execute(db) } } } @@ -92,7 +119,9 @@ struct DemoFeature { try withUndoDisabled { try database.write { db in let nextID = (try DemoItem.all.fetchAll(db).map(\.id).max() ?? 0) + 1 - try DemoItem.insert { DemoItem(id: nextID, name: "Item \(nextID)") }.execute(db) + try DemoItem.insert { + DemoItem(id: nextID, windowID: state.windowID, name: "Item \(nextID)") + }.execute(db) } } } @@ -322,6 +351,7 @@ final class ObservableUndoManager { @Table struct DemoItem: Identifiable { var id: Int + var windowID: UUID var name: String = "" var count: Int = 0 } @@ -339,6 +369,7 @@ func makeDemoDatabase() throws -> any DatabaseWriter { """ CREATE TABLE "demoItems" ( "id" INTEGER PRIMARY KEY, + "windowID" TEXT NOT NULL, "name" TEXT NOT NULL DEFAULT '', "count" INTEGER NOT NULL DEFAULT 0 ) diff --git a/README.md b/README.md index b646926..2ebe28e 100644 --- a/README.md +++ b/README.md @@ -103,24 +103,39 @@ BEGIN END ``` -### With explicit barrier management +### What a barrier captures -```swift -@Dependency(\.defaultUndoEngine) var undoEngine +A barrier claims exactly the writes made inside its `undoable` block. Barriers may +overlap freely — concurrent barriers, or one opened inside another, each keep their +own changes, and undoing one never disturbs another. + +Only writes made inside a barrier are tracked. A write outside one is applied +normally but is not undoable: -let barrierId = try undoEngine.beginBarrier("Set Rating") -try database.write { db in +```swift +try database.write { db in // not undoable try Article.find(id).update { $0.rating = 5 }.execute(db) } -try undoEngine.endBarrier(barrierId) + +try undoable("Set Rating") { // undoable + try database.write { db in + try Article.find(id).update { $0.rating = 5 }.execute(db) + } +} ``` +Tracking follows Swift's structured concurrency, so it reaches through `async` +writes and child tasks. It does not reach into a `Task.detached`, whose writes are +outside the barrier and therefore untracked. + ### Undo events -After each undo/redo, `UndoEngine` emits an `UndoEvent` with the affected table rows. Use this to drive UI responses like scrolling to a restored item or switching views. +After each undo/redo, the `UndoStack` that performed it emits an `UndoEvent` with the affected table rows. Use this to drive UI responses like scrolling to a restored item or switching views. ```swift -for await event in undoEngine.events() { +@Dependency(\.defaultUndoStack) var undoStack + +for await event in undoStack.events() { if let articleIds = event.ids(for: Article.self) { // scroll to restored articles } @@ -132,6 +147,8 @@ for await event in undoEngine.events() { `ids(for:)` returns `nil` when no rows of that table were affected, so `if let` naturally gates your response logic. +Events are scoped to the stack that performed the undo, so an undo in one window does not notify another. See [Multiple windows](#multiple-windows). + ## ComposableArchitecture/SwiftUI Integration ```swift @@ -183,6 +200,37 @@ struct MyView: View { } ``` +## Multiple windows + +An undo scope is one `UndoStack` bound to one `UndoManager`. The database, engine, and +undo log are app-wide; the stack is not. + +Give each window its own stack by scoping the dependency where its store is created: + +```swift +struct MyWindow: View { + @State private var store = withDependencies { + $0.installDefaultUndoStack() + } operation: { + Store(initialState: MyFeature.State()) { MyFeature() } + } + + var body: some View { + MyView(store: store) + } +} +``` + +The default stack is already app-wide, so a single-window app needs no setup at all. `installDefaultUndoStack()` exists to create an *additional* scope — call it once per window. + +Each window then has its own undo/redo stack, its own Edit menu state, and its own +event stream. Barriers register with whichever stack is current when they close, and +undoing in one window never touches another's changes. + +Because AppKit resolves `UndoManager` up the responder chain, this also gives you the +document case for free: several windows onto one `NSDocument` resolve to the *same* +`UndoManager`, so give them the same stack and they correctly share one undo history. + ## License This library is released under the MIT license. See [LICENSE](LICENSE) for details. diff --git a/Sources/SQLiteUndo/UndoBarrier.swift b/Sources/SQLiteUndo/UndoBarrier.swift index fe012e4..862a07b 100644 --- a/Sources/SQLiteUndo/UndoBarrier.swift +++ b/Sources/SQLiteUndo/UndoBarrier.swift @@ -1,37 +1,71 @@ import Foundation /// A barrier represents a single undoable user action, grouping all database -/// changes that occurred between `beginBarrier` and `endBarrier`. +/// changes made while it was open. /// /// When undo is performed, all changes within the barrier are reversed in /// reverse chronological order. /// -/// ## Sequence Numbers +/// ## Entry Ownership /// -/// The `startSeq` and `endSeq` store the ORIGINAL sequence range when the -/// barrier was created. However, after undo/redo operations, the actual -/// entries in the undolog move to new sequence positions (seq numbers grow, -/// they are not reused). `UndoEngine` tracks the current seq range separately -/// in `barrierSeqRanges` - see that documentation for details. +/// Undolog rows are stamped with the barrier's `id` as they are captured, so a +/// barrier owns its entries no matter what else writes concurrently. Replaying +/// a barrier re-stamps the newly captured reverse entries with the same `id`, +/// so ownership survives any number of undo/redo cycles. public struct UndoBarrier: Hashable, Sendable, Codable { - /// Unique identifier for this barrier. + /// Unique identifier for this barrier, and the key its undolog entries carry. public let id: UUID /// Display name for the action (shown in Edit > Undo menu). public let name: String - /// Original first sequence number when barrier was created (may not reflect current position). - let startSeq: Int - /// Original last sequence number when barrier was created (may not reflect current position). - let endSeq: Int + /// The number of undolog entries captured when this barrier closed. + public let count: Int - public init(id: UUID, name: String, startSeq: Int, endSeq: Int) { + public init(id: UUID, name: String, count: Int) { self.id = id self.name = name - self.startSeq = startSeq - self.endSeq = endSeq + self.count = count } +} + +/// Run an operation inside a barrier: open one, claim the writes it makes, close it. +/// +/// This is the only place a barrier is paired with its `_undoBarrierID` scope. A +/// barrier claims writes solely while that task local carries its ID, so opening one +/// any other way captures nothing at all — keeping the pairing here means callers +/// can't get it wrong. +func withBarrierScope( + _ name: String, + begin: (String) throws -> UUID, + end: (UUID) throws -> Void, + cancel: (UUID) throws -> Void, + operation: () throws -> T +) throws -> T { + let id = try begin(name) + do { + let result = try $_undoBarrierID.withValue(id.uuidString) { try operation() } + try end(id) + return result + } catch { + try cancel(id) + throw error + } +} - /// The number of undolog entries in this barrier. - public var count: Int { - endSeq - startSeq + 1 +/// Run an async operation inside a barrier. See ``withBarrierScope(_:begin:end:cancel:operation:)``. +func withBarrierScope( + _ name: String, + begin: (String) throws -> UUID, + end: (UUID) throws -> Void, + cancel: (UUID) throws -> Void, + operation: @Sendable () async throws -> T +) async throws -> T { + let id = try begin(name) + do { + let result = try await $_undoBarrierID.withValue(id.uuidString) { try await operation() } + try end(id) + return result + } catch { + try cancel(id) + throw error } } diff --git a/Sources/SQLiteUndo/UndoCoordinator.swift b/Sources/SQLiteUndo/UndoCoordinator.swift index 8d9e41e..07450ce 100644 --- a/Sources/SQLiteUndo/UndoCoordinator.swift +++ b/Sources/SQLiteUndo/UndoCoordinator.swift @@ -18,38 +18,7 @@ final class UndoCoordinator: Sendable { private let state = LockIsolated(State()) private struct State { - var openBarriers: [UUID: OpenBarrier] = [:] - var subscribers: [UUID: AsyncStream.Continuation] = [:] - - /// Tracks current seq range for each barrier. - /// - /// ## Why this is needed - /// - /// Following the sqlite.org/undoredo pattern, sequence numbers are NOT reused. - /// When you undo a barrier: - /// 1. Original entries (e.g., seq 1-2) are deleted - /// 2. Reverse SQL executes, triggers capture NEW entries (e.g., seq 3-4) - /// 3. The barrier's "current" range is now 3-4, not 1-2 - /// - /// The sqlite.org pattern stores `[begin, end]` pairs on undo/redo stacks, - /// pushing the NEW range after each operation. We can't do that because - /// NSUndoManager owns the stack and the barrier is captured in closures - /// with fixed `startSeq`/`endSeq` values. - /// - /// Instead, we track the current seq range per barrier here. When undo/redo - /// is performed, we look up the current range (not the original), execute - /// the SQL, and update the range to wherever the new entries landed. - var barrierSeqRanges: [UUID: SeqRange] = [:] - } - - private struct OpenBarrier { - let name: String - let startSeq: Int - } - - struct SeqRange { - var startSeq: Int - var endSeq: Int + var openBarriers: [UUID: String] = [:] } init( @@ -63,66 +32,42 @@ final class UndoCoordinator: Sendable { self.untrackedTables = untrackedTables } - /// Create a new stream of undo/redo events. - /// - /// Each call creates an independent subscription that receives events emitted from - /// this point on; earlier events are not replayed. - func events() -> AsyncStream { - let id = UUID() - let (stream, continuation) = AsyncStream.makeStream() - state.withValue { $0.subscribers[id] = continuation } - continuation.onTermination = { [state] _ in - state.withValue { _ = $0.subscribers.removeValue(forKey: id) } - } - return stream - } - - /// Broadcast an event to all active subscribers. - private func emit(_ event: UndoEvent) { - // Copy out before yielding so `onTermination` can't re-enter the lock. - for continuation in state.withValue({ Array($0.subscribers.values) }) { - continuation.yield(event) - } - } - /// Begin recording changes for a new undoable action. /// - /// All database changes after this call will be captured in the undolog - /// until `endBarrier` or `cancelBarrier` is called. + /// Changes are claimed by this barrier only while `_undoBarrierID` is set to its + /// ID — see ``withBarrier(_:_:)-(_,()throws->Void)``, which scopes that for you. /// /// - Parameter name: The action name (shown in Edit > Undo menu) /// - Returns: A unique ID for this barrier func beginBarrier(_ name: String) throws -> UUID { let id = UUID() - try database.read { db in - let currentSeq = try db.undoLogMaxSeq() ?? 0 - let startSeq = currentSeq + 1 - state.withValue { - $0.openBarriers[id] = OpenBarrier(name: name, startSeq: startSeq) - } - } + state.withValue { $0.openBarriers[id] = name } logger.debug("Begin barrier: \(name) (id: \(id))") return id } - /// End a barrier and capture all changes made since it began. + /// End a barrier and capture all changes it claimed. /// /// If no changes were made within the barrier, returns nil. /// /// - Parameter id: The barrier ID returned from `beginBarrier` /// - Returns: The completed barrier, or nil if no changes were captured func endBarrier(_ id: UUID) throws -> UndoBarrier? { - guard let openBarrier = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { + guard let name = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { logger.warning("Attempted to end unknown barrier: \(id)") return nil } return try database.write { db in - guard let endSeq = try db.undoLogMaxSeq(), endSeq >= openBarrier.startSeq else { + // Reconcile duplicate entries from cascading BEFORE triggers + try db.reconcileUndoLogEntries(barrierID: id) + + let count = try db.undoLogCount(barrierID: id) + guard count > 0 else { let tables = registeredTables.sorted() logger.warning( """ - End barrier (empty): \(openBarrier.name) — no database changes were captured. + End barrier (empty): \(name) — no database changes were captured. Did you forget to register a table with the UndoEngine? @@ -133,33 +78,17 @@ final class UndoCoordinator: Sendable { return nil } - // Reconcile duplicate entries from cascading BEFORE triggers - try db.reconcileUndoLogEntries(from: openBarrier.startSeq, to: endSeq) - - // Re-read endSeq since reconciliation may have removed entries - guard let endSeq = try db.undoLogMaxSeq(), endSeq >= openBarrier.startSeq else { - return nil - } - - let barrier = UndoBarrier( - id: id, - name: openBarrier.name, - startSeq: openBarrier.startSeq, - endSeq: endSeq - ) + let barrier = UndoBarrier(id: id, name: name, count: count) // Check for unregistered tables if !registeredTables.isEmpty { - let modifiedTables = try db.tablesModifiedInRange( - from: openBarrier.startSeq, - to: endSeq - ) + let modifiedTables = try db.tablesModified(barrierID: id) let allowedTables = registeredTables.union(untrackedTables) let unknownTables = modifiedTables.subtracting(allowedTables) if !unknownTables.isEmpty { reportIssue( """ - Barrier '\(openBarrier.name)' modified tables not registered with UndoEngine: \ + Barrier '\(name)' modified tables not registered with UndoEngine: \ \(unknownTables.sorted().joined(separator: ", ")). \ These changes won't be undone. Register the tables with UndoEngine, \ or add them to 'untracked:' if this is intentional. @@ -168,84 +97,113 @@ final class UndoCoordinator: Sendable { } } - // Track the seq range for this barrier - state.withValue { - $0.barrierSeqRanges[id] = SeqRange(startSeq: barrier.startSeq, endSeq: barrier.endSeq) - } - logger.debug("End barrier: \(barrier.name) (\(barrier.count) entries)") return barrier } } + /// Run an operation inside a barrier, returning the completed barrier. + /// + /// The barrier is cancelled if the operation throws. Changes must be made + /// within the operation to be captured. + /// + /// - Returns: The completed barrier, or nil if no changes were captured. + @discardableResult + func withBarrier(_ name: String, _ operation: () throws -> Void) throws -> UndoBarrier? { + var barrier: UndoBarrier? + try withBarrierScope( + name, + begin: beginBarrier, + end: { barrier = try endBarrier($0) }, + cancel: cancelBarrier, + operation: operation + ) + return barrier + } + + /// Run an async operation inside a barrier, returning the completed barrier. + /// + /// The barrier is cancelled if the operation throws. Changes must be made + /// within the operation to be captured. + /// + /// - Returns: The completed barrier, or nil if no changes were captured. + @discardableResult + func withBarrier( + _ name: String, + _ operation: @Sendable () async throws -> Void + ) async throws -> UndoBarrier? { + var barrier: UndoBarrier? + try await withBarrierScope( + name, + begin: beginBarrier, + end: { barrier = try endBarrier($0) }, + cancel: cancelBarrier, + operation: operation + ) + return barrier + } + /// Cancel a barrier without registering it for undo. /// /// Any changes made within the barrier remain in the database but won't /// be undoable as a group. Use this for aborted operations. /// + /// The entries are deleted whether or not the barrier is still open. A barrier that + /// threw on its way out of `endBarrier` has already been forgotten here, and its + /// rows would otherwise be orphaned in the log with nothing left to replay them. + /// /// - Parameter id: The barrier ID returned from `beginBarrier` func cancelBarrier(_ id: UUID) throws { - guard let openBarrier = state.withValue({ $0.openBarriers.removeValue(forKey: id) }) else { - logger.warning("Attempted to cancel unknown barrier: \(id)") - return - } + let name = state.withValue { $0.openBarriers.removeValue(forKey: id) } try database.write { db in - if let endSeq = try db.undoLogMaxSeq(), endSeq >= openBarrier.startSeq { - try db.deleteUndoLogEntries(from: openBarrier.startSeq, to: endSeq) - } + try db.deleteUndoLogEntries(barrierID: id) } - logger.debug("Cancel barrier: \(openBarrier.name)") + logger.debug("Cancel barrier: \(name ?? id.uuidString)") } - /// Perform undo for a barrier. + /// Discard a closed barrier's undolog entries. /// - /// Executes all reverse SQL in the barrier in reverse order. - /// The executed SQL is captured by triggers, becoming the redo SQL. - /// - /// The seq range used is looked up from `barrierSeqRanges` (not the barrier's - /// original values) because entries move to new seq positions after each - /// undo/redo. After execution, the tracked range is updated to the new positions. - func performUndo(barrier: UndoBarrier) throws { - let seqRange = - state.withValue { $0.barrierSeqRanges[barrier.id] } - ?? SeqRange(startSeq: barrier.startSeq, endSeq: barrier.endSeq) - - let result = try database.write { db in - try db.performUndoRedo(startSeq: seqRange.startSeq, endSeq: seqRange.endSeq) + /// Used when a barrier could not be registered for undo: nothing holds it, so its + /// entries can never be replayed and would otherwise sit in the log forever. The + /// database changes themselves stand — only the ability to undo them is gone. + func discardBarrier(_ id: UUID) throws { + try database.write { db in + try db.deleteUndoLogEntries(barrierID: id) } + logger.warning("Discarded unregistered barrier \(id) — its changes are not undoable") + } - if let result { - state.withValue { - $0.barrierSeqRanges[barrier.id] = result.seqRange - } - emit(UndoEvent(kind: .undo, name: barrier.name, affectedItems: result.affectedItems)) - } + /// Perform undo for a barrier. + /// + /// Executes all reverse SQL in the barrier in reverse order. + /// The executed SQL is captured by triggers, becoming the redo SQL, and is + /// re-stamped with this barrier's ID so it stays owned across cycles. + /// + /// - Returns: The event describing what changed, or nil if nothing was replayed. + /// The caller delivers it, since only it knows which undo scope this belongs to. + func performUndo(barrier: UndoBarrier) throws -> UndoEvent? { + guard let affectedItems = try replay(barrier: barrier) else { return nil } + return UndoEvent(kind: .undo, name: barrier.name, affectedItems: affectedItems) } /// Perform redo for a barrier. /// - /// Re-applies the original changes that were undone. - /// The executed SQL is captured by triggers, becoming the undo SQL again. + /// Re-applies the original changes that were undone. The executed SQL is + /// captured by triggers, becoming the undo SQL again. /// - /// The seq range used is looked up from `barrierSeqRanges` (not the barrier's - /// original values) because entries move to new seq positions after each - /// undo/redo. After execution, the tracked range is updated to the new positions. - func performRedo(barrier: UndoBarrier) throws { - let seqRange = - state.withValue { $0.barrierSeqRanges[barrier.id] } - ?? SeqRange(startSeq: barrier.startSeq, endSeq: barrier.endSeq) - - let result = try database.write { db in - try db.performUndoRedo(startSeq: seqRange.startSeq, endSeq: seqRange.endSeq) - } + /// - Returns: The event describing what changed, or nil if nothing was replayed. + func performRedo(barrier: UndoBarrier) throws -> UndoEvent? { + guard let affectedItems = try replay(barrier: barrier) else { return nil } + return UndoEvent(kind: .redo, name: barrier.name, affectedItems: affectedItems) + } - if let result { - state.withValue { - $0.barrierSeqRanges[barrier.id] = result.seqRange - } - emit(UndoEvent(kind: .redo, name: barrier.name, affectedItems: result.affectedItems)) + /// Replay a barrier's entries. Undo and redo are the same operation — each + /// captures the reverse of what it executes. + private func replay(barrier: UndoBarrier) throws -> Set? { + try database.write { db in + try db.performUndoRedo(barrierID: barrier.id) } } } diff --git a/Sources/SQLiteUndo/UndoEngine.swift b/Sources/SQLiteUndo/UndoEngine.swift index c35c022..8a4fe48 100644 --- a/Sources/SQLiteUndo/UndoEngine.swift +++ b/Sources/SQLiteUndo/UndoEngine.swift @@ -16,7 +16,6 @@ private let logger = Logger(subsystem: "SQLiteUndo", category: "UndoEngine") /// ```swift /// prepareDependencies { /// $0.defaultDatabase = try! appDatabase() -/// $0.defaultUndoStack = .live(windowUndoManager) /// $0.defaultUndoEngine = try! UndoEngine( /// for: $0.defaultDatabase, /// tables: Item.self, Edit.self @@ -49,30 +48,27 @@ private let logger = Logger(subsystem: "SQLiteUndo", category: "UndoEngine") public struct UndoEngine: Sendable { /// Begin recording changes for a new undoable action. /// + /// Internal: a barrier only claims writes made while `_undoBarrierID` is set to + /// its ID. Never call this directly — ``withBarrierScope(_:begin:end:cancel:operation:)`` + /// is what pairs the two, and ``undoable(_:operation:)-3cgh0`` is the public door to it. + /// /// - Parameter name: The action name (shown in Edit > Undo menu) /// - Returns: A unique ID for this barrier - public var beginBarrier: @Sendable (_ name: String) throws -> UUID = { _ in UUID() } + var beginBarrier: @Sendable (_ name: String) throws -> UUID = { _ in UUID() } /// End a barrier and register with UndoManager. /// /// If no changes were made within the barrier, nothing is registered. /// /// - Parameter id: The barrier ID from `beginBarrier` - public var endBarrier: @Sendable (_ id: UUID) throws -> Void + var endBarrier: @Sendable (_ id: UUID) throws -> Void /// Cancel a barrier without registering it. /// /// Use this for aborted operations or error handling. /// /// - Parameter id: The barrier ID from `beginBarrier` - public var cancelBarrier: @Sendable (_ id: UUID) throws -> Void - - /// Stream of events emitted after each undo/redo operation. - /// - /// Each call returns an independent subscription delivering events from that point - /// on; earlier events are not replayed. Cancelling one subscription leaves the others - /// unaffected, so callers may freely resubscribe. - public var events: @Sendable () -> AsyncStream = { .finished } + var cancelBarrier: @Sendable (_ id: UUID) throws -> Void } /// Whether undo tracking is active. Default true; set false inside `withUndoDisabled`. @@ -81,11 +77,22 @@ public struct UndoEngine: Sendable { /// Whether the undo system is replaying entries (undo/redo in progress). @TaskLocal var _undoIsReplaying = false +/// The barrier that owns writes made in the current scope, or nil when no barrier is open. +/// +/// Triggers stamp this onto each undolog row, which is how a barrier claims its +/// entries. Writes made outside a barrier are not tracked. +@TaskLocal var _undoBarrierID: String? + @DatabaseFunction("sqliteundo_isActive") func undoIsActiveFunction() -> Bool { _undoIsActive } +@DatabaseFunction("sqliteundo_barrierID") +func undoBarrierIDFunction() -> String? { + _undoBarrierID +} + @DatabaseFunction("sqliteundo_isReplaying") func undoIsReplayingFunction() -> Bool { _undoIsReplaying @@ -189,7 +196,6 @@ extension UndoEngine: DependencyKey { prepareDependencies { $0.defaultDatabase = try! appDatabase() - $0.defaultUndoStack = .live(windowUndoManager) $0.defaultUndoEngine = try! UndoEngine( for: $0.defaultDatabase, tables: MyTable1.self, MyTable2.self @@ -223,17 +229,28 @@ extension UndoEngine: DependencyKey { guard let barrier = try coordinator.endBarrier(id) else { return } - undoStack.registerBarrier( + // Capturing `undoStack` binds this barrier — and the events its undo/redo + // produce — to the scope that registered it. + let registered = undoStack.registerBarrier( barrier, - { try coordinator.performUndo(barrier: barrier) }, - { try coordinator.performRedo(barrier: barrier) } + { + if let event = try coordinator.performUndo(barrier: barrier) { + undoStack.emit(event) + } + }, + { + if let event = try coordinator.performRedo(barrier: barrier) { + undoStack.emit(event) + } + } ) + if !registered { + // Nothing holds this barrier, so its entries can never be replayed. + try coordinator.discardBarrier(barrier.id) + } }, cancelBarrier: { id in try coordinator.cancelBarrier(id) - }, - events: { - coordinator.events() } ) } diff --git a/Sources/SQLiteUndo/UndoOperations.swift b/Sources/SQLiteUndo/UndoOperations.swift index dc8219b..bd0e2f5 100644 --- a/Sources/SQLiteUndo/UndoOperations.swift +++ b/Sources/SQLiteUndo/UndoOperations.swift @@ -31,31 +31,24 @@ extension Database { /// ## Sequence Numbers Grow, Not Reused /// /// The sqlite.org pattern does NOT try to reuse sequence numbers. After each - /// undo/redo, entries move to new (higher) seq positions. This avoids conflicts - /// when multiple barriers exist - each barrier's entries can move independently - /// without colliding with other barriers' seq ranges. + /// undo/redo, entries move to new (higher) seq positions. `seq` therefore only + /// orders entries; ownership is carried by `barrierID`, which replay re-stamps + /// onto the newly captured entries. Barriers may interleave freely. /// - /// The caller (UndoEngine) tracks the current seq range for each barrier and - /// updates it after this method returns. - /// - struct UndoRedoResult { - var seqRange: UndoCoordinator.SeqRange - var affectedItems: Set - } - - /// - Returns: The new seq range and affected items, or nil if no entries were executed. - func performUndoRedo(startSeq: Int, endSeq: Int) throws -> UndoRedoResult? { - logger.debug("Performing undo/redo: seq \(startSeq)...\(endSeq)") + /// - Returns: The affected items, or nil if no entries were executed. + func performUndoRedo(barrierID: UUID) throws -> Set? { + let id = barrierID.uuidString + logger.debug("Performing undo/redo for barrier \(id)") // Fetch entries to execute (in reverse order) let entries = try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(id) } .order { $0.seq.desc() } .fetchAll(self) guard !entries.isEmpty else { - logger.debug("No entries found for seq range \(startSeq)...\(endSeq)") + logger.debug("No entries found for barrier \(id)") return nil } @@ -67,80 +60,74 @@ extension Database { ) // Delete the entries - try deleteUndoLogEntries(from: startSeq, to: endSeq) - - // Get current max seq before executing (new entries will be added after this) - let seqBefore = try undoLogMaxSeq() ?? 0 + try deleteUndoLogEntries(barrierID: barrierID) // Execute with triggers ENABLED - this captures the reverse SQL. // Set isReplaying so app-level triggers suppress cascading writes. // The undo log already contains all effects (including cascades), // so replaying them individually is sufficient. + // Re-stamp the captured entries with this barrier so it keeps owning them. // Batch consecutive same-table, same-type entries for efficiency. - try $_undoIsReplaying.withValue(true) { - try #sql("PRAGMA defer_foreign_keys = ON").execute(self) - for sql in batchedSQL(from: entries) { - logger.trace("Executing SQL: \(sql)") - try #sql("\(raw: sql)").execute(self) - } - #if DEBUG - // Check for FK violations that will cause the commit to fail. - let violations = try #sql( - """ - SELECT "table" || ' rowid=' || rowid || ' parent=' || "parent" || ' fkid=' || fkid - FROM pragma_foreign_key_check - """, - as: String.self - ).fetchAll(self) - if !violations.isEmpty { - logger.error( - """ - Undo replay will fail due to foreign key violations - - Ensure all tables involved in foreign key relationships are undo-tracked, - and that undo-tracked tables do not have foreign keys to non-tracked tables. + try $_undoBarrierID.withValue(id) { + try $_undoIsReplaying.withValue(true) { + try #sql("PRAGMA defer_foreign_keys = ON").execute(self) + for sql in batchedSQL(from: entries) { + logger.trace("Executing SQL: \(sql)") + try #sql("\(raw: sql)").execute(self) + } + #if DEBUG + // Check for FK violations that will cause the commit to fail. + let violations = try #sql( """ - ) - for v in violations { - logger.error(" FK violation after undo replay: \(v)") + SELECT "table" || ' rowid=' || rowid || ' parent=' || "parent" || ' fkid=' || fkid + FROM pragma_foreign_key_check + """, + as: String.self + ).fetchAll(self) + if !violations.isEmpty { + logger.error( + """ + Undo replay will fail due to foreign key violations + + Ensure all tables involved in foreign key relationships are undo-tracked, + and that undo-tracked tables do not have foreign keys to non-tracked tables. + """ + ) + for v in violations { + logger.error(" FK violation after undo replay: \(v)") + } } - } - #endif - } - - // Get new seq range for captured entries - let seqAfter = try undoLogMaxSeq() ?? seqBefore - if seqAfter > seqBefore { - let newRange = UndoCoordinator.SeqRange(startSeq: seqBefore + 1, endSeq: seqAfter) - // No reconciliation needed during replay: _undoIsReplaying suppresses - // app-level cascade triggers, so each row produces exactly one reverse entry. - logger.debug("New seq range: \(newRange.startSeq)...\(newRange.endSeq)") - return UndoRedoResult(seqRange: newRange, affectedItems: affectedItems) + #endif + } } - return nil + // No reconciliation needed during replay: _undoIsReplaying suppresses + // app-level cascade triggers, so each row produces exactly one reverse entry. + return affectedItems } } extension Database { - /// Get the current maximum sequence number in the undolog. - func undoLogMaxSeq() throws -> Int? { - try #sql("SELECT MAX(seq) FROM undolog", as: Int?.self).fetchOne(self) ?? nil + /// Count the undolog entries owned by a barrier. + func undoLogCount(barrierID: UUID) throws -> Int { + try UndoLogEntry + .where { $0.barrierID.eq(barrierID.uuidString) } + .fetchCount(self) } - /// Delete undolog entries in a sequence range. - func deleteUndoLogEntries(from startSeq: Int, to endSeq: Int) throws { + /// Delete the undolog entries owned by a barrier. + func deleteUndoLogEntries(barrierID: UUID) throws { try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(barrierID.uuidString) } .delete() .execute(self) } - /// Get the set of table names modified in a sequence range. - func tablesModifiedInRange(from startSeq: Int, to endSeq: Int) throws -> Set { + /// Get the set of table names a barrier modified. + func tablesModified(barrierID: UUID) throws -> Set { let tableNames = try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(barrierID.uuidString) } .select { $0.tableName } .fetchAll(self) return Set(tableNames) @@ -154,12 +141,14 @@ extension Database { /// - INSERT (DELETE-reverse) + DELETE (INSERT-reverse) of same row → remove both (no-op) /// - INSERT (DELETE-reverse) + UPDATE → keep just the DELETE-reverse (undo = delete) /// - Multiple UPDATEs → keep first (true original values) - func reconcileUndoLogEntries(from startSeq: Int, to endSeq: Int) throws { + func reconcileUndoLogEntries(barrierID: UUID) throws { + let id = barrierID.uuidString + // Fast path: check if any duplicates exist before fetching all entries let hasDuplicates = try #sql( """ SELECT 1 FROM undolog - WHERE seq >= \(startSeq) AND seq <= \(endSeq) AND trackedRowid != 0 + WHERE barrierID = \(id) AND trackedRowid != 0 GROUP BY tableName, trackedRowid HAVING COUNT(*) > 1 LIMIT 1 @@ -171,7 +160,7 @@ extension Database { let entries = try UndoLogEntry - .where { $0.seq >= startSeq && $0.seq <= endSeq } + .where { $0.barrierID.eq(id) } .order { $0.seq.asc() } .fetchAll(self) @@ -208,13 +197,13 @@ extension Database { // adding any columns not already present (first entry's values win). var mergedAssignments: [UndoSQL.UpdateSQL.Assignment]? var existingColumns: Set? - if case let .update(upd) = first.sql { + if case .update(let upd) = first.sql { mergedAssignments = upd.assignments existingColumns = Set(upd.assignments.map(\.column)) } for entry in group.dropFirst() { - if case let .update(upd) = entry.sql { + if case .update(let upd) = entry.sql { if var assignments = mergedAssignments, var columns = existingColumns { let additions = upd.assignments.filter { !columns.contains($0.column) } if !additions.isEmpty { @@ -228,7 +217,7 @@ extension Database { } } - if case let .update(upd) = first.sql, + if case .update(let upd) = first.sql, let assignments = mergedAssignments, assignments.count > upd.assignments.count { seqsToUpdate.append( diff --git a/Sources/SQLiteUndo/UndoSchema.swift b/Sources/SQLiteUndo/UndoSchema.swift index c58c72f..2e79c51 100644 --- a/Sources/SQLiteUndo/UndoSchema.swift +++ b/Sources/SQLiteUndo/UndoSchema.swift @@ -9,6 +9,8 @@ import SQLiteData struct UndoLogEntry: Sendable { /// Auto-incrementing sequence number for ordering. var seq: Int + /// The barrier that owns this entry. + var barrierID: String /// The name of the table that was modified. var tableName: String /// The rowid of the tracked row, for deduplication during reconciliation. @@ -78,15 +80,18 @@ extension DatabaseWriter { """ CREATE TABLE undolog ( seq INTEGER PRIMARY KEY AUTOINCREMENT, + barrierID TEXT NOT NULL, tableName TEXT NOT NULL, trackedRowid INTEGER NOT NULL DEFAULT 0, sql TEXT NOT NULL ) """ ).execute(db) + try #sql("CREATE INDEX undolog_barrierID ON undolog(barrierID)").execute(db) db.add(function: $undoIsActiveFunction) db.add(function: $undoIsReplayingFunction) + db.add(function: $undoBarrierIDFunction) } } } diff --git a/Sources/SQLiteUndo/UndoStack.swift b/Sources/SQLiteUndo/UndoStack.swift index 91def7d..8d02270 100644 --- a/Sources/SQLiteUndo/UndoStack.swift +++ b/Sources/SQLiteUndo/UndoStack.swift @@ -10,14 +10,15 @@ private let logger = Logger(subsystem: "SQLiteUndo", category: "UndoStack") /// This type handles registration of undo/redo actions with NSUndoManager /// and tracks the undo/redo stack state for testing. /// +/// A stack is one undo scope: one undo/redo history, one Edit menu, one event +/// stream. The default stack is app-wide, which is all a single-window app needs. +/// For separate per-window histories, see ``Dependencies/DependencyValues/installDefaultUndoStack(_:)``. +/// /// ## Setup /// -/// In production, wrap the window's UndoManager: -/// ```swift -/// prepareDependencies { -/// $0.defaultUndoStack = .live(windowUndoManager) -/// } -/// ``` +/// The UndoManager usually arrives from the view's environment rather than being +/// known up front — `setUndoManager(_:)` connects it, which +/// `UndoManagingReducer` does for you in the TCA integration. /// /// In tests, use the automatic test implementation which tracks stack state /// without requiring a real UndoManager. @@ -26,12 +27,15 @@ public struct UndoStack: Sendable { /// Register a barrier for undo/redo with the UndoManager. /// /// Called by UndoEngine when a barrier completes with changes. + /// - Returns: Whether the barrier was registered. False means there was no + /// UndoManager to register with, so the barrier is unreachable and its + /// undolog entries should be discarded. public var registerBarrier: @Sendable ( _ barrier: UndoBarrier, _ onUndo: @escaping @Sendable () throws -> Void, _ onRedo: @escaping @Sendable () throws -> Void - ) -> Void = { _, _, _ in } + ) -> Bool = { _, _, _ in true } /// Returns the current undo/redo stack state. /// @@ -49,6 +53,45 @@ public struct UndoStack: Sendable { /// For the `.live()` stack, this updates which UndoManager receives registrations. /// For the test stack, this is a no-op. public var setUndoManager: @Sendable (_ undoManager: UndoManager?) -> Void = { _ in } + + /// Stream of events emitted after each undo/redo performed on this stack. + /// + /// Events are scoped to the stack, so a window observes only the undos performed + /// against its own UndoManager. Windows sharing an UndoManager (as multiple + /// windows on one document do) share a stack, and so see the same events. + /// + /// Each call returns an independent subscription delivering events from that point + /// on; earlier events are not replayed. Cancelling one subscription leaves the + /// others unaffected, so callers may freely resubscribe. + public var events: @Sendable () -> AsyncStream = { .finished } + + /// Deliver an event to this stack's subscribers. + /// + /// Internal: called by `UndoEngine` from the undo/redo closures it registers, which + /// is what binds an event to the scope that performed it. + var emit: @Sendable (_ event: UndoEvent) -> Void = { _ in } +} + +/// Fan-out of undo events to any number of independent subscribers. +private final class UndoEventBroadcaster: Sendable { + private let subscribers = LockIsolated([UUID: AsyncStream.Continuation]()) + + func events() -> AsyncStream { + let id = UUID() + let (stream, continuation) = AsyncStream.makeStream() + subscribers.withValue { $0[id] = continuation } + continuation.onTermination = { [subscribers] _ in + subscribers.withValue { _ = $0.removeValue(forKey: id) } + } + return stream + } + + func emit(_ event: UndoEvent) { + // Copy out before yielding so `onTermination` can't re-enter the lock. + for continuation in subscribers.withValue({ Array($0.values) }) { + continuation.yield(event) + } + } } extension DependencyValues { @@ -56,6 +99,32 @@ extension DependencyValues { get { self[UndoStack.self] } set { self[UndoStack.self] = newValue } } + + /// Give the surrounding dependency scope its own undo stack. + /// + /// A stack is one undo scope: one undo/redo history, one Edit menu, one event + /// stream. The default stack is already app-wide, so a single-window app needs + /// none of this. + /// + /// Call this to create an *additional* scope — once per window, inside the + /// `withDependencies` that builds that window's store: + /// + /// ```swift + /// @State private var store = withDependencies { + /// $0.installDefaultUndoStack() + /// } operation: { + /// Store(initialState: MyFeature.State()) { MyFeature() } + /// } + /// ``` + /// + /// Windows meant to share one undo history should share one stack, so install it + /// once and hand the same value to each. + /// + /// - Parameter undoManager: The UndoManager to register with. Omit it when the + /// manager arrives later from the view's environment, as it does in SwiftUI. + public mutating func installDefaultUndoStack(_ undoManager: UndoManager? = nil) { + defaultUndoStack = .live(undoManager) + } } extension UndoStack: DependencyKey { @@ -69,6 +138,7 @@ extension UndoStack: DependencyKey { public static var testValue: UndoStack { let state = LockIsolated(UndoStackState(undo: [])) + let broadcaster = UndoEventBroadcaster() return UndoStack( registerBarrier: { barrier, onUndo, onRedo in @@ -76,6 +146,7 @@ extension UndoStack: DependencyKey { $0.undo.append(barrier.name) $0.redo = [] } + return true }, currentState: { UndoStackState( @@ -83,7 +154,9 @@ extension UndoStack: DependencyKey { redo: state.value.redo.reversed() ) }, - setUndoManager: { _ in } + setUndoManager: { _ in }, + events: { broadcaster.events() }, + emit: { broadcaster.emit($0) } ) } @@ -95,15 +168,48 @@ extension UndoStack: DependencyKey { /// - Parameter undoManager: Optional initial UndoManager public static func live(_ undoManager: UndoManager? = nil) -> UndoStack { let state = LockIsolated(UndoStackState(undo: [])) + let broadcaster = UndoEventBroadcaster() - // Target object for NSUndoManager registration - holds mutable UndoManager reference + // Target object for NSUndoManager registration - holds mutable UndoManager reference. + // + // `undoManager` is weak because the window owns its UndoManager, not us. That + // also keeps the registration closures below (which capture this target + // strongly, since NSUndoManager does not retain its target) from cycling. + // + // It sits behind a lock because `setUndoManager` is called from wherever the view + // layer runs while registration reads it on the main actor. Racing loads and + // stores of a *weak* reference are not merely torn values — they corrupt the + // runtime's side table. final class UndoTarget: @unchecked Sendable { + private struct WeakManager { + weak var value: UndoManager? + } + let state: LockIsolated - var undoManager: UndoManager? + private let manager: LockIsolated init(state: LockIsolated, undoManager: UndoManager?) { self.state = state - self.undoManager = undoManager + self.manager = LockIsolated(WeakManager(value: undoManager)) + } + + var undoManager: UndoManager? { + manager.withValue { $0.value } + } + + /// Point this target at a new UndoManager. + /// + /// - Returns: Whether a different, still-live manager was displaced — the + /// signature of two windows sharing one stack. Read and write happen under + /// one lock so the answer can't be stale by the time it's reported. + func setUndoManager(_ newManager: UndoManager?) -> Bool { + manager.withValue { box in + defer { box.value = newManager } + // A manager that was legitimately torn down has already gone nil, and + // clearing the manager is never a conflict. + guard let existing = box.value, newManager != nil else { return false } + return existing !== newManager + } } var currentState: UndoStackState { @@ -114,47 +220,46 @@ extension UndoStack: DependencyKey { } @MainActor + @discardableResult func registerUndo( barrier: UndoBarrier, onUndo: @escaping @Sendable () throws -> Void, onRedo: @escaping @Sendable () throws -> Void - ) { + ) -> Bool { guard let undoManager else { reportIssue( - "No UndoManager set. Call setUndoManager() or configure defaultUndoStack = .live(undoManager)" + "No UndoManager set. Call setUndoManager(), or install the stack with installDefaultUndoStack(undoManager)" ) logger.warning( "\(self.currentState.logDescription(after: "\"\(barrier.name)\" — undoManager is nil, registration dropped"))" ) - return + return false } logger.debug("Registering undo: \(barrier.name)") undoManager.beginUndoGrouping() undoManager.setActionName(barrier.name) - undoManager.registerUndo(withTarget: self) { [weak self] target in + undoManager.registerUndo(withTarget: self) { [self] _ in MainActor.assumeIsolated { logger.debug("Performing undo: \(barrier.name)") do { try onUndo() - self?.state.withValue { + state.withValue { if let index = $0.undo.lastIndex(of: barrier.name) { $0.undo.remove(at: index) } $0.redo.append(barrier.name) } - if let self { - logger.info( - "\(self.currentState.logDescription(after: "undo \"\(barrier.name)\""))" - ) - } - target.registerRedo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) + logger.info( + "\(self.currentState.logDescription(after: "undo \"\(barrier.name)\""))" + ) + registerRedo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } catch { logger.error("Undo failed for \"\(barrier.name)\": \(error)") } } } undoManager.endUndoGrouping() - logger.info("\(self.currentState.logDescription(after: "register \"\(barrier.name)\""))") + return true } @MainActor @@ -165,7 +270,7 @@ extension UndoStack: DependencyKey { ) { guard let undoManager else { reportIssue( - "No UndoManager set. Call setUndoManager() or configure defaultUndoStack = .live(undoManager)" + "No UndoManager set. Call setUndoManager(), or install the stack with installDefaultUndoStack(undoManager)" ) logger.warning( "\(self.currentState.logDescription(after: "redo \"\(barrier.name)\" — undoManager is nil"))" @@ -173,23 +278,21 @@ extension UndoStack: DependencyKey { return } logger.debug("Registering redo: \(barrier.name)") - undoManager.registerUndo(withTarget: self) { [weak self] target in + undoManager.registerUndo(withTarget: self) { [self] _ in MainActor.assumeIsolated { logger.debug("Performing redo: \(barrier.name)") do { try onRedo() - self?.state.withValue { + state.withValue { if let index = $0.redo.lastIndex(of: barrier.name) { $0.redo.remove(at: index) } $0.undo.append(barrier.name) } - if let self { - logger.info( - "\(self.currentState.logDescription(after: "redo \"\(barrier.name)\""))" - ) - } - target.registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) + logger.info( + "\(self.currentState.logDescription(after: "redo \"\(barrier.name)\""))" + ) + registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } catch { logger.error("Redo failed for \"\(barrier.name)\": \(error)") } @@ -202,22 +305,29 @@ extension UndoStack: DependencyKey { return UndoStack( registerBarrier: { barrier, onUndo, onRedo in - state.withValue { - $0.undo.append(barrier.name) - $0.redo = [] - } // NSUndoManager requires main thread + let registered: Bool if Thread.isMainThread { - MainActor.assumeIsolated { + registered = MainActor.assumeIsolated { target.registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } } else { - DispatchQueue.main.sync { + registered = DispatchQueue.main.sync { MainActor.assumeIsolated { target.registerUndo(barrier: barrier, onUndo: onUndo, onRedo: onRedo) } } } + // Only a barrier that reached the UndoManager belongs on the stack. Pushing + // first and rolling back would clear a redo stack that is still perfectly + // valid, since nothing new was actually performed. + guard registered else { return false } + state.withValue { + $0.undo.append(barrier.name) + $0.redo = [] + } + logger.info("\(target.currentState.logDescription(after: "register \"\(barrier.name)\""))") + return true }, currentState: { UndoStackState( @@ -225,14 +335,28 @@ extension UndoStack: DependencyKey { redo: state.value.redo.reversed() ) }, - setUndoManager: { - target.undoManager = $0 - if $0 != nil { + setUndoManager: { newManager in + // Two windows sharing one stack shows up here: the second window's mount + // replaces the first's manager, and from then on every window's undo goes + // to whichever mounted last. + if target.setUndoManager(newManager) { + reportIssue( + """ + This UndoStack is being handed a second UndoManager while the first is + still in use, so undo will only ever reach whichever window registered + last. Give each window its own stack by calling installDefaultUndoStack() + in the withDependencies that builds that window's store. + """ + ) + } + if newManager != nil { logger.info("setUndoManager: set") } else { logger.warning("setUndoManager: nil") } - } + }, + events: { broadcaster.events() }, + emit: { broadcaster.emit($0) } ) } } diff --git a/Sources/SQLiteUndo/UndoTracked.swift b/Sources/SQLiteUndo/UndoTracked.swift index 12c4014..a669c70 100644 --- a/Sources/SQLiteUndo/UndoTracked.swift +++ b/Sources/SQLiteUndo/UndoTracked.swift @@ -31,10 +31,10 @@ extension StructuredQueries.Table { """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_\(table)_insert AFTER INSERT ON "\(table)" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('\(table)', NEW.rowid, 'D'||char(9)||'\(table)'||char(9)||NEW.rowid); + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), '\(table)', NEW.rowid, 'D'||char(9)||'\(table)'||char(9)||NEW.rowid); END """ } @@ -54,11 +54,11 @@ extension StructuredQueries.Table { return """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_\(table)_update BEFORE UPDATE ON "\(table)" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL AND (\(changeChecks)) BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('\(table)', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), '\(table)', OLD.rowid, 'U'||char(9)||'\(table)'||char(9)||OLD.rowid || \(caseClauses) ); @@ -76,10 +76,10 @@ extension StructuredQueries.Table { return """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_\(table)_delete BEFORE DELETE ON "\(table)" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('\(table)', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), '\(table)', OLD.rowid, 'I'||char(9)||'\(table)'||char(9)||OLD.rowid || \(colValuePairs) ); diff --git a/Sources/SQLiteUndo/Undoable.swift b/Sources/SQLiteUndo/Undoable.swift index 4e3766b..0961504 100644 --- a/Sources/SQLiteUndo/Undoable.swift +++ b/Sources/SQLiteUndo/Undoable.swift @@ -18,15 +18,13 @@ public func undoable( ) throws -> T { @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier(actionName) - do { - let result = try operation() - try undoEngine.endBarrier(barrierId) - return result - } catch { - try undoEngine.cancelBarrier(barrierId) - throw error - } + return try withBarrierScope( + actionName, + begin: { try undoEngine.beginBarrier($0) }, + end: { try undoEngine.endBarrier($0) }, + cancel: { try undoEngine.cancelBarrier($0) }, + operation: operation + ) } /// Execute an async operation within an undoable barrier. @@ -46,15 +44,13 @@ public func undoable( ) async throws -> T { @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier(actionName) - do { - let result = try await operation() - try undoEngine.endBarrier(barrierId) - return result - } catch { - try undoEngine.cancelBarrier(barrierId) - throw error - } + return try await withBarrierScope( + actionName, + begin: { try undoEngine.beginBarrier($0) }, + end: { try undoEngine.endBarrier($0) }, + cancel: { try undoEngine.cancelBarrier($0) }, + operation: operation + ) } /// Execute an operation with undo tracking disabled. diff --git a/Sources/SQLiteUndoTCA/UndoManaging.swift b/Sources/SQLiteUndoTCA/UndoManaging.swift index 18d32aa..6156f27 100644 --- a/Sources/SQLiteUndoTCA/UndoManaging.swift +++ b/Sources/SQLiteUndoTCA/UndoManaging.swift @@ -55,7 +55,6 @@ public enum UndoManagingAction: Sendable { /// ``` public struct UndoManagingReducer: Reducer { @Dependency(\.defaultUndoStack) var undoStack - @Dependency(\.defaultUndoEngine) var undoEngine private enum CancelID { case eventSubscription } @@ -68,7 +67,7 @@ public struct UndoManagingReducer Double { let (database, engine) = try makeUndoBenchmarkDatabase() // Insert rows in one barrier - let barrierId = try engine.beginBarrier("Insert") - try database.write { db in - for i in 1...rows { - try BenchRecord.insert { BenchRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + let barrier = try engine.withBarrier("Insert") { + try database.write { db in + for i in 1...rows { + try BenchRecord.insert { BenchRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! _undoBatchingDisabled = !batched @@ -81,11 +81,11 @@ private func measureUpdate(rows: Int, batched: Bool) throws -> Double { } // Update all rows in one barrier - let barrierId = try engine.beginBarrier("Update") - try database.write { db in - try BenchRecord.all.update { $0.value = 42 }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update") { + try database.write { db in + try BenchRecord.all.update { $0.value = 42 }.execute(db) + } + }! _undoBatchingDisabled = !batched diff --git a/Tests/SQLiteUndoTests/UndoEngineTests.swift b/Tests/SQLiteUndoTests/UndoEngineTests.swift index 58170a2..04314c5 100644 --- a/Tests/SQLiteUndoTests/UndoEngineTests.swift +++ b/Tests/SQLiteUndoTests/UndoEngineTests.swift @@ -24,19 +24,19 @@ enum UndoEngineTests { """ CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_testRecords_insert AFTER INSERT ON "testRecords" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('testRecords', NEW.rowid, 'D'||char(9)||'testRecords'||char(9)||NEW.rowid); + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), 'testRecords', NEW.rowid, 'D'||char(9)||'testRecords'||char(9)||NEW.rowid); END CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_testRecords_update BEFORE UPDATE ON "testRecords" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL AND (OLD."id" IS NOT NEW."id" OR OLD."name" IS NOT NEW."name" OR OLD."value" IS NOT NEW."value") BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('testRecords', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), 'testRecords', OLD.rowid, 'U'||char(9)||'testRecords'||char(9)||OLD.rowid || CASE WHEN OLD."id" IS NOT NEW."id" THEN char(9)||'id'||char(9)||quote(OLD."id") ELSE '' END || CASE WHEN OLD."name" IS NOT NEW."name" THEN char(9)||'name'||char(9)||quote(OLD."name") ELSE '' END @@ -46,10 +46,10 @@ enum UndoEngineTests { CREATE TEMPORARY TRIGGER IF NOT EXISTS _undo_testRecords_delete BEFORE DELETE ON "testRecords" - WHEN "sqliteundo_isActive"() + WHEN "sqliteundo_isActive"() AND "sqliteundo_barrierID"() IS NOT NULL BEGIN - INSERT INTO undolog(tableName, trackedRowid, sql) - VALUES('testRecords', OLD.rowid, + INSERT INTO undolog(barrierID, tableName, trackedRowid, sql) + VALUES("sqliteundo_barrierID"(), 'testRecords', OLD.rowid, 'I'||char(9)||'testRecords'||char(9)||OLD.rowid || char(9)||'id'||char(9)||quote(OLD."id") || char(9)||'name'||char(9)||quote(OLD."name") @@ -68,14 +68,12 @@ enum UndoEngineTests { func beginAndEndBarrier() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Test Action") - #expect(barrierId != UUID()) - - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + let barrier = try engine.withBarrier("Test Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } } - let barrier = try engine.endBarrier(barrierId) #expect(barrier != nil) #expect(barrier?.name == "Test Action") #expect(barrier?.count ?? 0 > 0) @@ -85,8 +83,7 @@ enum UndoEngineTests { func endBarrierWithNoChanges() throws { let (_, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Empty Action") - let barrier = try engine.endBarrier(barrierId) + let barrier = try engine.withBarrier("Empty Action") {} #expect(barrier == nil) } @@ -95,15 +92,37 @@ enum UndoEngineTests { func cancelBarrier() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Cancelled Action") + struct CancelError: Error {} + #expect(throws: CancelError.self) { + try engine.withBarrier("Cancelled Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + throw CancelError() + } + } - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + // Verify the undolog entries were removed + let undoLogCount = try database.read { db in + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") } + #expect(undoLogCount == 0) + } - try engine.cancelBarrier(barrierId) + /// A barrier that threw on its way out of `endBarrier` is already forgotten by + /// the time the cancel runs, so cancelling must not depend on it still being open. + @Test + func cancellingAClosedBarrierStillRemovesItsEntries() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try engine.withBarrier("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! + + try engine.cancelBarrier(barrier.id) - // Verify the undolog entries were removed let undoLogCount = try database.read { db in try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") } @@ -112,17 +131,466 @@ enum UndoEngineTests { } @Suite - struct UndoRedoTests { + @MainActor + struct ScopeRoutingTests { + /// Two "windows" share one database and engine but each has its own + /// UndoStack/UndoManager, supplied by its own dependency scope. @Test - func undoInsert() throws { + func barriersRegisterWithTheScopesUndoManager() throws { + let managerA = UndoManager() + let managerB = UndoManager() + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + } operation: { + @Dependency(\.defaultDatabase) var database + + try withDependencies { + $0.defaultUndoStack = .live(managerA) + } operation: { + try undoable("From A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + } + + try withDependencies { + $0.defaultUndoStack = .live(managerB) + } operation: { + try undoable("From B") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + } + + #expect(managerA.undoActionName == "From A") + #expect(managerB.undoActionName == "From B") + + // Undoing in A reverts only A's row. + managerA.undo() + + try database.read { db in + let rowA = try TestRecord.find(1).fetchOne(db) + let rowB = try TestRecord.find(2).fetchOne(db) + #expect(rowA == nil) + #expect(rowB?.name == "b") + } + + #expect(managerA.canUndo == false) + #expect(managerB.canUndo == true) + } + } + + /// An undo performed in one window must not notify the other. + @Test + func undoEventsReachOnlyTheScopeThatPerformedThem() async throws { + let managerA = UndoManager() + let managerB = UndoManager() + let stackA = UndoStack.live(managerA) + let stackB = UndoStack.live(managerB) + + var eventsA = stackA.events().makeAsyncIterator() + var eventsB = stackB.events().makeAsyncIterator() + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + } operation: { + @Dependency(\.defaultDatabase) var database + + try withDependencies { + $0.defaultUndoStack = stackA + } operation: { + try undoable("From A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + } + try withDependencies { + $0.defaultUndoStack = stackB + } operation: { + try undoable("From B") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + } + + managerA.undo() + } + + let a = await eventsA.next() + expectNoDifference( + a, + UndoEvent( + kind: .undo, + name: "From A", + affectedItems: [AffectedItem(table: TestRecord.self, rowid: 1)] + )) + + // B performed no undo, so its stream has nothing pending. + stackB.emit(UndoEvent(kind: .redo, name: "sentinel", affectedItems: [])) + let b = await eventsB.next() + #expect(b?.name == "sentinel") + } + + /// Sharing one stack between windows silently sends every window's undo to + /// whichever mounted last, so it must be reported rather than left to discover. + /// This fires on the second window's mount, before any action is taken. + @Test + func warnsWhenOneStackIsHandedASecondUndoManager() { + let managerA = UndoManager() + let managerB = UndoManager() + let shared = UndoStack.live() + + shared.setUndoManager(managerA) // window A mounts + + withKnownIssue { + shared.setUndoManager(managerB) // window B mounts against the same stack + } matching: { issue in + issue.description.contains("installDefaultUndoStack") + } + } + + @Test + func noWarningWhenTheSameUndoManagerIsSetAgain() { + let manager = UndoManager() + let stack = UndoStack.live() + + // `.task(id: undoManager)` re-fires with the same manager; not a misconfiguration. + stack.setUndoManager(manager) + stack.setUndoManager(manager) + } + + @Test + func noWarningWhenThePreviousUndoManagerIsGone() { + let stack = UndoStack.live() + + // A window that closed: its manager deallocated, so the weak reference is + // already nil and the next window is not a conflict. + do { + let closing = UndoManager() + stack.setUndoManager(closing) + } + stack.setUndoManager(UndoManager()) + + // Clearing is likewise not a conflict. + stack.setUndoManager(nil) + } + + /// A barrier that could not be registered is unreachable, so its undolog + /// entries must not accumulate. + @Test + func unregisterableBarrierDiscardsItsEntries() throws { + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + $0.defaultUndoStack = .live() // never given an UndoManager + } operation: { + @Dependency(\.defaultDatabase) var database + + try withKnownIssue { + try undoable("Nowhere to register") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + } matching: { issue in + issue.description.contains("No UndoManager set") + } + + let undoLogCount = try database.read { db in + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") + } + #expect(undoLogCount == 0, "entries for an unregisterable barrier should be discarded") + + // The write itself still stands — it just isn't undoable. + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 1) + } + } + } + + /// A dropped registration performed nothing, so it must not clear a redo stack + /// whose entries are still perfectly valid. + @Test + func droppedRegistrationLeavesTheRedoStackIntact() throws { + let manager = UndoManager() + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + $0.defaultUndoStack = .live(manager) + } operation: { + @Dependency(\.defaultDatabase) var database + @Dependency(\.defaultUndoStack) var undoStack + + try undoable("A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + manager.undo() + #expect(undoStack.currentState() == UndoStackState(undo: [], redo: ["A"])) + + // The window goes away, taking its UndoManager with it. + undoStack.setUndoManager(nil) + + try withKnownIssue { + try undoable("B") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + } matching: { issue in + issue.description.contains("No UndoManager set") + } + + #expect(undoStack.currentState() == UndoStackState(undo: [], redo: ["A"])) + } + } + + /// NSUndoManager does not retain its registration target, so the stack that + /// registered a barrier may be released long before the undo is performed. + @Test + func undoWorksAfterTheRegisteringStackIsReleased() throws { + let manager = UndoManager() + + try withDependencies { + let database = try! makeTestDatabase() + $0.defaultDatabase = database + $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) + } operation: { + @Dependency(\.defaultDatabase) var database + + // The stack is owned by this scope alone and released when it exits. + try withDependencies { + $0.defaultUndoStack = .live(manager) + } operation: { + try undoable("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "test") }.execute(db) + } + } + } + + manager.undo() + + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 0) + } + + manager.redo() + + try database.read { db in + let row = try TestRecord.find(1).fetchOne(db) + #expect(row?.name == "test") + } + } + } + } + + @Suite + struct BarrierOwnershipTests { + + @Test + func openBarrierDoesNotClaimAnotherBarriersChanges() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + // "Inner" opens, writes, and closes while "Outer" is still open. + var inner: UndoBarrier? + let outer = try engine.withBarrier("Outer") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "outer") }.execute(db) + } + inner = try engine.withBarrier("Inner") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "inner") }.execute(db) + } + } + }! + + #expect(outer.count == 1) + #expect(inner?.count == 1) + + // Undoing "Outer" must not revert "Inner"'s row. + try engine.performUndo(barrier: outer) + + try database.read { db in + let outerRow = try TestRecord.find(1).fetchOne(db) + let innerRow = try TestRecord.find(2).fetchOne(db) + #expect(outerRow == nil) + #expect(innerRow?.name == "inner") + } + + // "Inner" is still independently undoable. + try engine.performUndo(barrier: inner!) + + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 0) + } + } + + @Test + func concurrentBarriersOwnOnlyTheirOwnChanges() async throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Insert Item") + // Two barriers racing, as two windows would. Whatever the interleaving, + // each must end up owning exactly its own row. + async let a = engine.withBarrier("A") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "a") }.execute(db) + } + } + async let b = engine.withBarrier("B") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "b") }.execute(db) + } + } + let (barrierA, barrierB) = try await (a!, b!) + + #expect(barrierA.count == 1) + #expect(barrierB.count == 1) + + try engine.performUndo(barrier: barrierA) + + let (rowA, rowB) = try await database.read { db in + (try TestRecord.find(1).fetchOne(db), try TestRecord.find(2).fetchOne(db)) + } + #expect(rowA == nil) + #expect(rowB?.name == "b") + } + + @Test + func writesOutsideAnyBarrierAreNotTracked() throws { + let (database, _) = try makeTestDatabaseWithUndo() + try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + try TestRecord.insert { TestRecord(id: 1, name: "untracked") }.execute(db) } - let barrier = try engine.endBarrier(barrierId)! + + let undoLogCount = try database.read { db in + try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM undolog") + } + #expect(undoLogCount == 0) + + try database.read { db in + let count = try TestRecord.all.fetchCount(db) + #expect(count == 1) + } + } + + @Test + func asyncBarrierCapturesChanges() async throws { + // Guards the mechanism the whole design rests on: the barrier TaskLocal + // must survive GRDB's async write, which hops to its own executor. + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try await engine.withBarrier("Async Insert") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + } + + #expect(barrier?.count == 1) + + try engine.performUndo(barrier: barrier!) + + let count = try await database.read { db in + try TestRecord.all.fetchCount(db) + } + #expect(count == 0) + } + + @Test + func detachedTaskWritesAreNotTracked() async throws { + // Documented limitation: a detached task starts a fresh task context, so it + // is outside the barrier. Its writes apply but are not undoable. + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try await engine.withBarrier("Detached") { + try await Task.detached { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "detached") }.execute(db) + } + }.value + } + + #expect(barrier == nil) + + let count = try await database.read { db in + try TestRecord.all.fetchCount(db) + } + #expect(count == 1) + } + + @Test + func ownershipSurvivesRepeatedUndoRedoCycles() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + let first = try engine.withBarrier("First") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "first") }.execute(db) + } + }! + let second = try engine.withBarrier("Second") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "second") }.execute(db) + } + }! + + // Cycle the first barrier repeatedly; entries move to new seq positions + // each time but must stay owned by their barrier. + for _ in 1...3 { + try engine.performUndo(barrier: first) + try database.read { db in + let firstRow = try TestRecord.find(1).fetchOne(db) + let secondRow = try TestRecord.find(2).fetchOne(db) + #expect(firstRow == nil) + #expect(secondRow != nil) + } + try engine.performRedo(barrier: first) + try database.read { db in + let firstRow = try TestRecord.find(1).fetchOne(db) + #expect(firstRow?.name == "first") + } + } + + // The second barrier is unaffected by all that churn. + try engine.performUndo(barrier: second) + try database.read { db in + let firstRow = try TestRecord.find(1).fetchOne(db) + let secondRow = try TestRecord.find(2).fetchOne(db) + #expect(firstRow?.name == "first") + #expect(secondRow == nil) + } + } + } + + @Suite + struct UndoRedoTests { + + @Test + func undoInsert() throws { + let (database, engine) = try makeTestDatabaseWithUndo() + + let barrier = try engine.withBarrier("Insert Item") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -147,14 +615,14 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Update Item") - try database.write { db in - try TestRecord.find(1).update { - $0.name = "Updated" - $0.value = 20 - }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Update Item") { + try database.write { db in + try TestRecord.find(1).update { + $0.name = "Updated" + $0.value = 20 + }.execute(db) + } + }! try database.read { db in let record = try TestRecord.find(1).fetchOne(db)! @@ -181,11 +649,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Delete Item") - try database.write { db in - try TestRecord.find(1).delete().execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Delete Item") { + try database.write { db in + try TestRecord.find(1).delete().execute(db) + } + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -211,11 +679,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Set Value") - try database.write { db in - try TestRecord.find(1).update { $0.value = 100 }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Set Value") { + try database.write { db in + try TestRecord.find(1).update { $0.value = 100 }.execute(db) + } + }! try engine.performUndo(barrier: barrier) @@ -236,13 +704,13 @@ enum UndoEngineTests { func multipleChangesInOneBarrier() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Batch Insert") - try database.write { db in - for i in 1...5 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + let barrier = try engine.withBarrier("Batch Insert") { + try database.write { db in + for i in 1...5 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -292,11 +760,11 @@ enum UndoEngineTests { } // Normal insert — trigger should fire - let barrierId = try engine.beginBarrier("Insert") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Alice") }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Alice") }.execute(db) + } + }! try database.read { db in let actions = try String.fetchAll(db, sql: "SELECT action FROM auditLog ORDER BY id") @@ -357,13 +825,12 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier("Set Name") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + try undoable("Set Name") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } } - try undoEngine.endBarrier(barrierId) #expect(testUndoManager.canUndo == true) #expect(testUndoManager.undoActionName == "Set Name") @@ -380,13 +847,12 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - let barrierId = try undoEngine.beginBarrier("Insert") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + try undoable("Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } } - try undoEngine.endBarrier(barrierId) let countBefore = try database.read { db in try TestRecord.all.fetchCount(db) } #expect(countBefore == 1) @@ -408,7 +874,6 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine try withUndoDisabled { try database.write { db in @@ -416,11 +881,11 @@ enum UndoEngineTests { } } - let barrierId = try undoEngine.beginBarrier("Update") - try database.write { db in - try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + try undoable("Update") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + } } - try undoEngine.endBarrier(barrierId) testUndoManager.undo() @@ -448,21 +913,18 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - // Create item 1 - let barrierId1 = try undoEngine.beginBarrier("Create Item 1") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + try undoable("Create Item 1") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) - // Create item 2 - let barrierId2 = try undoEngine.beginBarrier("Create Item 2") - try database.write { db in - try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + try undoable("Create Item 2") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) // Verify both items exist #expect(try database.read { db in try TestRecord.all.fetchCount(db) } == 2) @@ -508,16 +970,14 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine - - let barrierId = try undoEngine.beginBarrier("Background Insert") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - // End the barrier from a background thread + // Run the whole barrier from a background thread DispatchQueue.global().sync { - try! undoEngine.endBarrier(barrierId) + try! undoable("Background Insert") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + } } #expect(testUndoManager.canUndo == true) @@ -542,27 +1002,26 @@ enum UndoEngineTests { $0.defaultUndoEngine = try! UndoEngine(for: database, tables: TestRecord.self) } operation: { @Dependency(\.defaultDatabase) var database - @Dependency(\.defaultUndoEngine) var undoEngine @Dependency(\.defaultUndoStack) var undoStack // Initial state #expect(undoStack.currentState() == UndoStackState(undo: [], redo: [])) // Do "A" - let barrierId1 = try undoEngine.beginBarrier("A") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "A") }.execute(db) + try undoable("A") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "A") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) #expect(undoStack.currentState() == UndoStackState(undo: ["A"], redo: [])) // Do "B" - let barrierId2 = try undoEngine.beginBarrier("B") - try database.write { db in - try TestRecord.find(1).update { $0.name = "B" }.execute(db) + try undoable("B") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "B" }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) #expect(undoStack.currentState() == UndoStackState(undo: ["B", "A"], redo: [])) @@ -583,11 +1042,11 @@ enum UndoEngineTests { #expect(undoStack.currentState() == UndoStackState(undo: ["B", "A"], redo: [])) // Do "C" - should clear redo stack - let barrierId3 = try undoEngine.beginBarrier("C") - try database.write { db in - try TestRecord.find(1).update { $0.name = "C" }.execute(db) + try undoable("C") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "C" }.execute(db) + } } - try undoEngine.endBarrier(barrierId3) #expect(undoStack.currentState() == UndoStackState(undo: ["C", "B", "A"], redo: [])) @@ -595,11 +1054,11 @@ enum UndoEngineTests { testUndoManager.undo() #expect(undoStack.currentState() == UndoStackState(undo: ["B", "A"], redo: ["C"])) - let barrierId4 = try undoEngine.beginBarrier("D") - try database.write { db in - try TestRecord.find(1).update { $0.name = "D" }.execute(db) + try undoable("D") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "D" }.execute(db) + } } - try undoEngine.endBarrier(barrierId4) #expect(undoStack.currentState() == UndoStackState(undo: ["D", "B", "A"], redo: [])) } @@ -629,19 +1088,19 @@ enum UndoEngineTests { func tracksUndoableActions() throws { #expect(undoStack.currentState() == []) - let barrierId1 = try undoEngine.beginBarrier("Add Item") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + try undoable("Add Item") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) #expect(undoStack.currentState() == ["Add Item"]) - let barrierId2 = try undoEngine.beginBarrier("Update Item") - try database.write { db in - try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + try undoable("Update Item") { + try database.write { db in + try TestRecord.find(1).update { $0.name = "Updated" }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) // Most recent first #expect(undoStack.currentState() == ["Update Item", "Add Item"]) @@ -649,20 +1108,20 @@ enum UndoEngineTests { @Test func newActionClearsRedoStack() throws { - let barrierId1 = try undoEngine.beginBarrier("First Action") - try database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + try undoable("First Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Item 1") }.execute(db) + } } - try undoEngine.endBarrier(barrierId1) #expect(undoStack.currentState() == ["First Action"]) // New action should clear redo stack (even though we can't undo in test mode) - let barrierId2 = try undoEngine.beginBarrier("Second Action") - try database.write { db in - try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + try undoable("Second Action") { + try database.write { db in + try TestRecord.insert { TestRecord(id: 2, name: "Item 2") }.execute(db) + } } - try undoEngine.endBarrier(barrierId2) // Most recent first #expect(undoStack.currentState() == ["Second Action", "First Action"]) @@ -670,9 +1129,8 @@ enum UndoEngineTests { @Test func emptyBarrierNotTracked() throws { - let barrierId = try undoEngine.beginBarrier("Empty Action") // No database changes - try undoEngine.endBarrier(barrierId) + try undoable("Empty Action") {} #expect(undoStack.currentState() == []) } @@ -684,13 +1142,13 @@ enum UndoEngineTests { func bulkInsertUndoRedo() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Bulk Insert") - try database.write { db in - for i in 1...1000 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + let barrier = try engine.withBarrier("Bulk Insert") { + try database.write { db in + for i in 1...1000 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)", value: i) }.execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -730,11 +1188,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Bulk Delete") - try database.write { db in - try TestRecord.all.delete().execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Bulk Delete") { + try database.write { db in + try TestRecord.all.delete().execute(db) + } + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -771,11 +1229,11 @@ enum UndoEngineTests { } } - let barrierId = try engine.beginBarrier("Bulk Update") - try database.write { db in - try TestRecord.all.update { $0.value = 42 }.execute(db) - } - let barrier = try engine.endBarrier(barrierId)! + let barrier = try engine.withBarrier("Bulk Update") { + try database.write { db in + try TestRecord.all.update { $0.value = 42 }.execute(db) + } + }! try engine.performUndo(barrier: barrier) @@ -798,19 +1256,19 @@ enum UndoEngineTests { func bulkMixedOperations() throws { let (database, engine) = try makeTestDatabaseWithUndo() - let barrierId = try engine.beginBarrier("Mixed Ops") - try database.write { db in - for i in 1...500 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) - } - for i in 1...250 { - try TestRecord.find(i).update { $0.value = 99 }.execute(db) - } - for i in 251...500 { - try TestRecord.find(i).delete().execute(db) + let barrier = try engine.withBarrier("Mixed Ops") { + try database.write { db in + for i in 1...500 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + } + for i in 1...250 { + try TestRecord.find(i).update { $0.value = 99 }.execute(db) + } + for i in 251...500 { + try TestRecord.find(i).delete().execute(db) + } } - } - let barrier = try engine.endBarrier(barrierId)! + }! try database.read { db in let count = try TestRecord.all.fetchCount(db) @@ -839,19 +1297,16 @@ enum UndoEngineTests { struct UndoEventTests { @Test - func eventEmittedOnUndo() async throws { + func eventReturnedFromUndo() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! - - var iterator = coordinator.events().makeAsyncIterator() - try coordinator.performUndo(barrier: barrier) + let barrier = try await coordinator.withBarrier("Insert Item") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! - let event = await iterator.next() + let event = try coordinator.performUndo(barrier: barrier) expectNoDifference( event, UndoEvent( @@ -862,22 +1317,19 @@ enum UndoEngineTests { } @Test - func eventEmittedOnRedo() async throws { + func eventReturnedFromRedo() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! - - var iterator = coordinator.events().makeAsyncIterator() - try coordinator.performUndo(barrier: barrier) - try coordinator.performRedo(barrier: barrier) + let barrier = try await coordinator.withBarrier("Insert Item") { + try await database.write { db in + try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) + } + }! - let undoEvent = await iterator.next() + let undoEvent = try coordinator.performUndo(barrier: barrier) #expect(undoEvent?.kind == .undo) - let redoEvent = await iterator.next() + + let redoEvent = try coordinator.performRedo(barrier: barrier) expectNoDifference( redoEvent, UndoEvent( @@ -891,18 +1343,15 @@ enum UndoEngineTests { func affectedItemsForMultiRowBarrier() async throws { let (database, coordinator) = try makeTestDatabaseWithUndo() - let barrierId = try coordinator.beginBarrier("Batch Insert") - try await database.write { db in - for i in 1...3 { - try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + let barrier = try await coordinator.withBarrier("Batch Insert") { + try await database.write { db in + for i in 1...3 { + try TestRecord.insert { TestRecord(id: i, name: "Item \(i)") }.execute(db) + } } - } - let barrier = try coordinator.endBarrier(barrierId)! - - var iterator = coordinator.events().makeAsyncIterator() - try coordinator.performUndo(barrier: barrier) + }! - let event = await iterator.next() + let event = try coordinator.performUndo(barrier: barrier) expectNoDifference( event, UndoEvent( @@ -917,18 +1366,18 @@ enum UndoEngineTests { } @Test - func eventsBroadcastToAllSubscribers() async throws { - let (database, coordinator) = try makeTestDatabaseWithUndo() + func affectedItemIdAs() { + let item = AffectedItem(table: TestRecord.self, rowid: 42) + #expect(item.id(as: TestRecord.self) == 42) + } - var first = coordinator.events().makeAsyncIterator() - var second = coordinator.events().makeAsyncIterator() + @Test + func eventsBroadcastToAllSubscribersOfAStack() async { + let stack = UndoStack.live() + var first = stack.events().makeAsyncIterator() + var second = stack.events().makeAsyncIterator() - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! - try coordinator.performUndo(barrier: barrier) + stack.emit(UndoEvent(kind: .undo, name: "Insert", affectedItems: [])) let firstEvent = await first.next() let secondEvent = await second.next() @@ -937,55 +1386,49 @@ enum UndoEngineTests { } @Test - func resubscribingAfterCancellationStillReceivesEvents() async throws { - let (database, coordinator) = try makeTestDatabaseWithUndo() + func resubscribingAfterCancellationStillReceivesEvents() async { + let stack = UndoStack.live() - let abandoned = coordinator.events() + let abandoned = stack.events() let task = Task { for await _ in abandoned {} } task.cancel() await task.value - var iterator = coordinator.events().makeAsyncIterator() - - let barrierId = try coordinator.beginBarrier("Insert Item") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let barrier = try coordinator.endBarrier(barrierId)! - try coordinator.performUndo(barrier: barrier) + var iterator = stack.events().makeAsyncIterator() + stack.emit(UndoEvent(kind: .undo, name: "Insert", affectedItems: [])) let event = await iterator.next() #expect(event?.kind == .undo) } @Test - func eventsAreNotReplayedToLaterSubscribers() async throws { - let (database, coordinator) = try makeTestDatabaseWithUndo() - - let firstId = try coordinator.beginBarrier("First") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 1, name: "Test") }.execute(db) - } - let first = try coordinator.endBarrier(firstId)! - try coordinator.performUndo(barrier: first) + func eventsAreNotReplayedToLaterSubscribers() async { + let stack = UndoStack.live() - var iterator = coordinator.events().makeAsyncIterator() + stack.emit(UndoEvent(kind: .undo, name: "First", affectedItems: [])) - let secondId = try coordinator.beginBarrier("Second") - try await database.write { db in - try TestRecord.insert { TestRecord(id: 2, name: "Test") }.execute(db) - } - let second = try coordinator.endBarrier(secondId)! - try coordinator.performUndo(barrier: second) + var iterator = stack.events().makeAsyncIterator() + stack.emit(UndoEvent(kind: .undo, name: "Second", affectedItems: [])) let event = await iterator.next() #expect(event?.name == "Second") } @Test - func affectedItemIdAs() { - let item = AffectedItem(table: TestRecord.self, rowid: 42) - #expect(item.id(as: TestRecord.self) == 42) + func eventsDoNotCrossScopes() async { + let stackA = UndoStack.live() + let stackB = UndoStack.live() + + var fromA = stackA.events().makeAsyncIterator() + var fromB = stackB.events().makeAsyncIterator() + + stackA.emit(UndoEvent(kind: .undo, name: "From A", affectedItems: [])) + stackB.emit(UndoEvent(kind: .undo, name: "From B", affectedItems: [])) + + let a = await fromA.next() + let b = await fromB.next() + #expect(a?.name == "From A") + #expect(b?.name == "From B") } @Test diff --git a/Tests/SQLiteUndoTests/UnregisteredTableTests.swift b/Tests/SQLiteUndoTests/UnregisteredTableTests.swift index 0f91a98..8813d56 100644 --- a/Tests/SQLiteUndoTests/UnregisteredTableTests.swift +++ b/Tests/SQLiteUndoTests/UnregisteredTableTests.swift @@ -28,14 +28,13 @@ struct UnregisteredTableTests { registeredTables: [ArticleRecord.tableName] ) - let barrierId = try coordinator.beginBarrier("Mixed Changes") - try database.write { db in - try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) - try AuditRecord.insert { AuditRecord(id: 1, data: "Created article") }.execute(db) - } - try withKnownIssue { - _ = try coordinator.endBarrier(barrierId) + try coordinator.withBarrier("Mixed Changes") { + try database.write { db in + try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) + try AuditRecord.insert { AuditRecord(id: 1, data: "Created article") }.execute(db) + } + } } matching: { issue in issue.description.contains("auditRecords") } @@ -61,13 +60,12 @@ struct UnregisteredTableTests { registeredTables: [ArticleRecord.tableName, AuditRecord.tableName] ) - let barrierId = try coordinator.beginBarrier("Both Registered") - try database.write { db in - try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) - try AuditRecord.insert { AuditRecord(id: 1, data: "Audit") }.execute(db) + let barrier = try coordinator.withBarrier("Both Registered") { + try database.write { db in + try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) + try AuditRecord.insert { AuditRecord(id: 1, data: "Audit") }.execute(db) + } } - - let barrier = try coordinator.endBarrier(barrierId) #expect(barrier != nil) } @@ -92,13 +90,12 @@ struct UnregisteredTableTests { untrackedTables: [AuditRecord.tableName] ) - let barrierId = try coordinator.beginBarrier("With Untracked") - try database.write { db in - try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) - try AuditRecord.insert { AuditRecord(id: 1, data: "Audit log entry") }.execute(db) + let barrier = try coordinator.withBarrier("With Untracked") { + try database.write { db in + try ArticleRecord.insert { ArticleRecord(id: 1, name: "Article") }.execute(db) + try AuditRecord.insert { AuditRecord(id: 1, data: "Audit log entry") }.execute(db) + } } - - let barrier = try coordinator.endBarrier(barrierId) #expect(barrier != nil) } }