UndoManager, custom actor, and callbacks, oh my!

Using Swift
, ,
1

(For some context, I'm very experienced with Apple platforms, GCD etc; fairly experienced with Swift; fairly new to SwiftUI, and brand new to Swift Concurrency.)

I have just refactored my app to do all its work/business logic inside a custom actor. Since this work is mainly done in SQL queries, I've encapsulated the sqlite database inside this actor, and given it a custom serial queue on a custom executor. Then, in all of my UI code, I've restructured things to use async calls to everywhere interacting with the database/business logic (both fetching and mutating), enforced by my custom actor. This is working great!

(Note that I have not used a custom global actor, since in theory in the future I might want to open two databases at once, and I feel they should have an actor each.)

The one thing I've not solved yet is undo. Previously, I had an undo manager that my database object (now my database actor) held onto. I also have a sqlite table that, through use of sqlite triggers, stores SQL which will do the inverse of any database operation. (This is the pattern recommended in the sqlite manual.) Immediately before each database mutation I get the current max rowID of that table, and I do that again after the mutation; I then create a closure for the undo manager to fetch and execute the sql for each row between those IDs. It worked well.

Now, I have a couple of problems. We'll start by assuming that in the new concurrent world, the undomanager will be isolated within my actor. These problems are probably related to some poor understanding of Swift Concurrency on my part:

Problem 1: how to get the undo manager's closure to run synchronously on my actor, in order to register the redo operation at the correct time

As far as I understand, to achieve a redo operation with NSUndoManager, one has to register an undo operation when one is already inside an undo operation. With the closure syntax of NSUndoManager (which is the only one usable when not working with ObjC compatible objects), I take this to mean before the undo closure returns.

Inside the undo closure, I therefore need to do some database fetches to be able to pull the relevant information needed to register the redo action, as well as to perform the database mutation for the undo action. The latter could be done async, but the former couldn't, because the redo operation has to be registered before the undo closure returns. I need to base the redo operation on the state of the database now, not on the state of the database in the future.

Thus, I cannot wrap the contents of my undo closure in a Task, or my redo operation would be registered too late. It only turns to a redo operation if it's registered inside the undo closure, otherwise it's just another undo operation.

What am I missing? Is there a way to make sure the undo closure is isolated to my actor, or else to block from within it until a Task has completed? Does the fact that my actor has its own serial queue help here?

Problem 2: tying in with SwiftUI's UndoManager (aka I really wish NSUndoManager was sendable)

If the undo manager is entirely encapsulated inside my actor, then nothing outside of my actor can talk to it.

Is the recommended pattern here to register undo actions with SwiftUI's undo manager that simply call an async function on my actor to tell its internal undo manager to undo or redo?

If so, for the registration of those undo/redo actions with the outer undo manager, will I run into the same issue that redo actions have to be registered synchronously inside the undo closure?

Surely I can't be the only one running into this? If UndoManager could take an async closure, and ensure that things counted as redo if they were registered within this async closure, it would solve things. But that'd mean the undomanager needed to manage its own serial queue too I guess.

Should I just abandon NSUndoManager and hook my own system up to the menu bar etc?

What would I lose by doing so? I fear that the system created undo managers for things like text editing might not play nice in that situation…

2

Hmmm, I seem to have rubber ducked part of my problem. I think section 1 can be solved with assumeIsolated.

3

Without actually trying it—you probably have more Cocoa and async Swift experience than me—you might be able to use beginUndoGrouping and endUndoGrouping to make this register the actions the way you want, kind of, sort of. But ultimately, registering the actions really does have to be done synchronously, because as soon as the main run loop ticks around again the user can hit Cmd-Z, even if the first transaction hasn't finished yet.

One approach would be to record your undo action as a plain old "undo action #87", where the number isn't the full row-ID-based thing you're tracking in the database, but simply a local, MainActor-bound list of outstanding changes. Then if you get asked to "undo action #87", but you don't have a local entry in the list, you know you're still waiting for the action itself to complete and the list to get updated. In which case you register "redo action #87", async-ly await the contents of "action #87", and then async-ly perform the undo.

This still doesn't seem ideal to me because ultimately everything the user does is a synchronous action, and an asynchronous database doesn't line up with that. The consequence of this is either an Undo Manager that updates its state when the user hasn't entered any input (visible in the "dirty" state of a document, for example), or UI state that doesn't reflect the actual state of the database (relevant if the app crashes, for example). That's just a fundamental disconnect no matter what UI and asynchrony idioms are in use, though, and if your database really is asynchronous and yet you want things to be undoable, you get to decide how to present that to your users.

EDIT: "the user can hit Cmd-Z" well, maybe you register the action up front, but disable undo/redo until it completes

2 Likes
4

I don't like that idea because NSUndoManager throws an exception if you have an open undo grouping when certain things happen. (Can't remember the details, but it relies on the parent undo grouping being tied to run loop ticks to make it usable in normal situations.)

That could work. It'll be some complicated programming to make sure I can get the redo sql out of the database at the right time… since I'll care about the state of the db at the time of the undo request, not whatever the current state is when the async task runs.

2 Likes
5

That's probably a good idea

2 Likes
6

You can turn off groupsByEvent on the undo manager to handle this manually, but you can't directly call undo while you have a group open (which is what you are probably remembering). So you could make things work like Jordan suggests, you just wouldn't be able to use the built-in Undo/Redo menu items with SwiftUI provides, because those directly call undo/redo on the undoManager.

If you had your own undo button, for instance, you could disable it while the database is doing its thing and then re-enable it by calling back to the MainActor when you were done at the same time as you close the (manual) undo group.

So I think you need to start with: what is the user experience you are looking to provide since performing the changes is now temporally disconnected from the user performing the actions? E.g. what happens if the user chooses undo while the action is still 'in flight' in the actor? - Nothing? - Undoes the previously completed action? - Waits until the current transaction is done and then undoes it?

NSUndoManager isn't really built for any of those except maybe the first one if you handle undo groups manually. I think you are probably better off abandoning NSUndoManager and doing something that fits what you actually want.

3 Likes
7

I haven't used this pattern personally, but I just wanted to say that I think it is great and will continue to be widely-used.

There is a way to make closures under your control isolated to your actor, via isolated parameters. That will not work here, but I just wanted to point it out, since not everyone is aware and it is a powerful tool.

And while there certainly are ways to make the code block, I don't think there are any that are safe or supported.

I think this is the crux of the problem. @jrose and @gregtitus did a great job of outlining some options.

But, I also want to try to think about this a little less concretely since you've already gotten plenty of feedback on undo specifically.

1 - you have some non-Sendable mutable state (NSUndoManager)
2 - it is owned by another actor (MainActor)
3 - you need to interact with it synchronously

I think it's kind of amazing that the concurrency rules have surfaced such a high-level design challenge as you have here. Whenever you find yourself wishing some type not under your control was Sendable, you should immediately suspect that your isolation strategy or design may need revisiting. This is just the nature of isolation. This isn't the first time I've heard a story about a system that was 99% there, but this one little tiny bit of synchronous code messed up the whole thing.

Sometimes, you'll encounter APIs that aren't Sendable/asynchronous yet, but could become that way in the future. I don't think that NSUndoManager is such an API, unfortunately. This may be unsatisfying, but I'm pretty sure you have no choice other than to come up with a different approach.

8

Just repeating something here that I posted to Mastodon for completeness:

In my experience, and I have quite a lot with Core Data and sync in particular (https://ensembles.io), you are asking the wrong questions. Or at least, the architecture you are using is the problem. IMHO undo should never be a DB level operation. It should happen up in your business logic, where you can make high level decisions.

I have hit this many times in apps I have worked on for undo (eg Sketch, Agenda). When you take advantage of auto-undo, like what you get in Core Data, or your own registration of SQLite logs, you are taking on a technical debt, and the more complex your app gets, the more troublesome it becomes.

In a nutshell, the DB has no concept of user actions, which is what undo is about. For the DB, undo is just a collection of data changes which can be inverted. Things start to get messy when changes in the DB are not solely due to user actions. Think about some changes syncing in from a different device, or a background import. These clearly should not be undoable by the current user. So what happens if the current user does undo something, but the data was changed by the sync in the meantime? You end up in some possibly invalid, undefined state. Just mixing together two streams of concurrent data changes is too low level for this.

It also explains why you are struggling with async. Undo should never be async, because the user action is clear from the beginning. The data changes may take time, but that has nothing to do with the user action. So record the user action, rather than trying to wrangle undo into the async DB action.

My advice is to ditch DB level undo, and instead simply define an enum or similar at the business logic level for things the user can do that should be undoable. For a note taking app, for example, it might look like this

enum UndoableOperation {
    case addNote(Note.ID)
    case deleteNote(Note.ID)
    case moveNote(Note.ID, from: Project.ID, to: Project.ID)

    var inverse: UndoableOperation { ... }
}

Now you simply record these in the undo manager at an appropriate time, corresponding to what the user would expect.

When the user undoes, you apply the inverse of the original action, if possible. The nice aspect here is you can validate whether it is possible or not. If a sync has changed the DB in the meantime, it's doesn't matter. Eg. if you undo an "add note", you first check if the note is still there, and if it is, you delete it.

(I was inspired years ago by a Wil Shipley post on this with regard to Delicious Library. He basically ditched the Core Data "magic" undo, and did something like what I mention above. It is much easier than it seems. The magic undo always hurts you in the long term, as you have to disengage during imports, handle invalid data, etc It is a short term gain for long term pain.)

6 Likes
9

This is indeed sensible, since SQLite connections can be configured to be used from multiple threads as long as all accesses are serialized (Multi-thread mode).

I'm just replying here in order to say that there are other ways to deal with SQLite concurrency. In particular, the WAL mode has many benefits: not only it allows parallel reads and writes, but it is documented to be "significantly faster in most scenarios". Such parallel reads and writes don't directly fit the actor model. Databases are "more advanced" in their handling of shared mutable state.

You might be interested in having a look at GRDB. It's a Swift SQLite toolkit that allows apps to make the most of their SQLite databases. It's sharp so it will happily make you define your undo triggers. It supports WAL mode and concurrent reads and writes. It has built-in support for async/await if desired. And, cherry of the cake, it also supports synchronous database accesses, which makes it a good fit for UndoManager.

1 Like