`redo` broken for `UndoManager`?

Using Swift
1

Whereas undo works without issues for the following code, a redo (triggered via user interaction) does not again call the closure passed to the UndoManager such that value stays unchanged. Is this a bug in the UndoManager or am I missing something on how to set up redo for the UndoManager correctly?

import Combine
import SwiftUI

class DataModel: ObservableObject {
    var value: Float = 120.0
}

struct ContentView: View {
    @Environment(\.undoManager) var undoManager
    @StateObject var dataModel = DataModel()

    var body: some View {
        VStack {
            Button("+1") {
                let previousValue = dataModel.value
                undoManager?.registerUndo(withTarget: dataModel, handler: { dataModel in
                    print(dataModel.value)
                    dataModel.value = previousValue
                })
                dataModel.value += 1
            }
        }.frame(width: 100, height: 100)
    }
}
2

The same happens in a plain CLI application which owns the UndoManager and undo / redo being called programmatically:

import Foundation

let undoManager = UndoManager()

class SwiftClass {
  var property: String = ""
}

var target = SwiftClass()
undoManager.registerUndo(withTarget: target) { target in
  target.property = "expected"
}
// empty string before `undo`
assert(target.property == "")

// set to "expected" by calling `undo`
undoManager.undo()
assert(target.property == "expected")

// set to empty string again by calling `redo`
undoManager.redo()
assert(target.property == "") // assertion fails: `.property` is still equal to `"expected"`
3

Submitted a bug report: Calling `redo` on `UndoManager` doesn't redo but results in a no-op · Issue #61949 · apple/swift · GitHub

4

This is normal. The redo action is recorded from calls to registerUndo while performing the undo action, but you're not calling registerUndo in your undo closure. Here's one way to do it:

class SwiftClass {
  var property: String = ""

  // use this method when you want to register an undo action.
  func setProperty(_ newValue: String, undoManager: UndoManager?) {
    let oldValue = property
    undoManager?.registerUndo(withTarget: self) { [weak undoManager] target in
      target.setProperty(oldValue, undoManager: undoManager) // this will call registerUndo
    }
    property = newValue
  }
}

var target = SwiftClass()
target.setProperty("expected", undoManager: undoManager)

Alternatively, you can store the undo manager within the class and record changes every time you set property:

class SwiftClass {
  var undoManager: UndoManager

  init(undoManager: UndoManager) {
    self.undoManager = undoManager
  }

  var property: String = "" {
    didSet {
      undoManager.registerUndo(withTarget: self) { target in
        target.property = oldValue // this will call registerUndo
      }
    }
  }
}

var target = SwiftClass(undoManager: UndoManager)
target.property = "expected"

This has the drawback that you can't bypass the undo manager if you need to when setting property, although it could also be an advantage in some situations.

2 Likes
5

Adding to @michelf’s answer, this makes sense if you think about how UndoManager works: how else would it know how to reset the original value? All you’ve given it is an object and a closure, and that closure could do pretty much anything. There’s no way for the UndoManager to know how to reverse the effects of the closure unless you tell it.

That said, personally, I’ve always avoided UndoManager. It’s from another time and another programming paradigm. If your document is all value types rolling your own state-based undo in Swift is pretty easy:

struct UndoManaged<T> {
    private(set) var value: T
    private var undoStack: [T] = []
    private var redoStack: [T] = []

    init(initialValue: T) {
        self.value = initialValue
    }

    mutating func changeValue(_ closure: (inout T) -> ()) {
        let oldValue = value
        closure(&value)
        undoStack.append(oldValue)
        redoStack.removeAll()
    }

    mutating func undo() {
        if let newValue = undoStack.popLast() {
            let oldValue = value
            value = newValue
            redoStack.append(oldValue)
        }
    }

    mutating func redo() {
        if let newValue = redoStack.popLast() {
            let oldValue = value
            value = newValue
            undoStack.append(oldValue)
        }
    }

    var canUndo: Bool {
        !undoStack.isEmpty
    }

    var canRedo: Bool {
        !redoStack.isEmpty
    }
}
4 Likes
6

@michelf Ah right, thanks for pointing out that registerUndo needs to be called again in the undo closure!

@hisekaldma Makes perfect sense that the UndoManager has no way of knowing the initial value like that. Though in my code snippet above redo is also a no-op after several previous undo such that there could be a previous closure to refer to. But as @michelf pointed out, the issue was somewhere else. Thanks for outlining how a custom undo manager could like!

Thanks again both of you!

2 Likes