Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Where did my UndoManager gone? #50

Open
Motti-Shneor opened this issue Nov 12, 2017 · 9 comments
Open

Where did my UndoManager gone? #50

Motti-Shneor opened this issue Nov 12, 2017 · 9 comments

Comments

@Motti-Shneor
Copy link

I just re-compiled my BSManagedDocument base app again in Xcode 9, and updated my BSManagedDocument to branch v0.4 - and my Undo functionality disappeared...

Short debugging revealed that the managedObjectContext I'm getting from BSManagedDocument has a nil UndoManager.

I did not remove any code that created this undoManager. Furthermore, I always assumed the default NSManagedObjectContext creates its own NSUndoManager.

First question: is this the expected behavior of BSManagedDocument? Did something change recently on this matter? What will happen to previous versions of my app running on latest OS-X ? Is this CoreDate that changed?

Second question - is there a "right" place to bring back undo-support to my BSManagedDocument pased app? Where is the managedObjectContext created (earliest stage, so I can add the UndoManager there). Maybe I should just override one of the

-(void)setUndoManager:(NSUndoManager *)undoManager;
-(void)setHasUndoManager:(BOOL)hasUndoManager;

no-op methods? what to do there? simply return a "YES" ?

Any help will be greatly appreciated.

@theMikeSwan
Copy link

If you take a look at my fork, https://github.com/theMikeSwan/BSManagedDocument, you will find I have merged most of the work others have done into a single place (branch v0.5.x). It includes a fix for the undo manager however, and this is very important, it currently is having issues on 10.14.

I don't know what happened but if you even try to look at the undo manager of the context right after the context was created there is a bad access crash. I'm currently working on a workaround.

This of course assumes you are still interested in BSManagedDocument since it has been almost a year since you posted this bug.

@Motti-Shneor
Copy link
Author

Motti-Shneor commented Oct 15, 2018 via email

@theMikeSwan
Copy link

The original version had a line that set the document's undo manager by calling super in -setManagedObjectContext:. That crashes on 10.14 as does any access that I have tried so far in -setManagedObjectContext: or -managedObjectContext.

Basically if you have something like context.undoManager in one of those methods or any other method that runs in the same run loop as -managedObjectContext you will likely get a crash.

I have found that if you delay by a tiny amount before messing with the context's undo manager all is fine. Unfortunately, I have also found that timers and perform selector after delay don't seem to work on 10.14. As a result I have had to stick the undo setup into -showWindows (I haven't totally tested this yet and it seems to mess with state restoration on 10.14).

@Motti-Shneor
Copy link
Author

Motti-Shneor commented Oct 15, 2018 via email

@theMikeSwan
Copy link

I haven't tried building on 10.13 and running on 10.14 as of yet and I haven't messed with NSPersistentDocument in forever so I'm not sure what happens in those cases.

I'm guessing this is all the result of a bug introduced in 10.14 as checking a value for nil really shouldn't ever cause a crash. When I have time I'll make a sample app and stick it into bug reporter.

I would try out your app on 10.14 for sure even if it means installing it onto an external drive of some sort.

Also, feel free to take any patches you've come up with and push them to your public repo. You can create a pull request or I can look through them at some point and pull in any useful bits that haven't been solved by others already (most likely Jerry Krinock, he seems to have done the most work).

@evanmiller
Copy link

@theMikeSwan I've been using the solution described here without any crashes:

#47 (comment)

Note that this works with vanilla BSManagedDocument, not with the Jerry Kronick changes.

@evanmiller
Copy link

Also @theMikeSwan have you tried accessing undoManager inside performBlock: or performBlockAndWait:? I think this will execute the message on the correct queue.

@theMikeSwan
Copy link

@evanmiller I have not tried either of the perform block methods, I may have to give it a go when I have some time again.

While unlikely it is always possible that the issue is some how limited to Swift/Obj-C hybrid apps…

@jerrykrinock
Copy link

@theMikeSwan, I saw that bad access crash earlier today too. But upon closer inspection, in my case, I see it is one of those +[NSManagedObjectContext __Multithreading_Violation_AllThatIsLeftToUsIsHonor__] assertions. (Core Data multi-threading assertions enabled is your friend :)

This does seem pretty weird that simply accessing the pointer value of the context's undo manager to see if it is nil must be done on the proper thread, but after I saw my friend AllThatIsLeftToUsIsHonor, I wrapped the said code of @evanmiller in performBlockAndWait:, and this wrap fixed the crashes. So, if you've not given it a go yet, the answer is YES.

I am finishing this up in such a way that supports a custom undo manager class, or nil undo manager, both of which I need for my projects. I want to create a partition running macOS 10.11 to test it there too, before I commit and push.

jerrykrinock added a commit to jerrykrinock/BSManagedDocument that referenced this issue Jun 3, 2019
…ustom or nil undo manager.

The issue would happen whenever was called from -[BSManagedDocument managedObjectContext] before undo manager had been created.   Nowadays, macOS typically initializes documents on a secondary thread.  Apparenly, simply *accessing* the undo manager of a main-queue managed object context to see if that undo manager is nil when on a secondary thread, either crashes inexplicably, or with Core Data multi-threading assertions enabled, raises one of those +[NSManagedObjectContext __Multithreading_Violation_AllThatIsLeftToUsIsHonor__]: assertions.
The fix for this issue, as usual, is to wrap the code in -performBlockAndWait:, as predicted in karelia#50.

A custom undo manager is required to use, for example, Graham Cox’ GCUndoManager.  A nil undo manager may be desired when documents are processed automatically by a background-running agent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants