karelia / BSManagedDocument

Brings UIManagedDocument's design to OS X
Other
55 stars 25 forks source link

Where did my UndoManager gone? #50

Open Motti-Shneor opened 6 years ago

Motti-Shneor commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

Thank you very much, and yes - I am very interested in the fix

I still need a little clarification -

My software is deployed already to customers, compiled and linked against a relatively old point in BSManagedDocument repo. Will it crash/otherwise misbehave when run on MacOS 10.14? on 10.13 it behaves nicely.

Of course I had to work around deficiencies - and create+apply an undo manager myself after creating the BSManagedDocument instance (when run on later versions of OS-X) - plus several other patches I needed to apply, but I still need to know what to expect.

If the software is bound to misbehave - then I’m more than willing to update my git submodule of BSManagedDocument to a newer branch/commit, rebuild and re-deploy the application.

On 15 Oct 2018, at 6:50, theMikeSwan notifications@github.com wrote:

If you take a look at my fork, https://github.com/theMikeSwan/BSManagedDocument 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.

Actually, since I heavily rely on the undo manager - I create one myself and apply it to the context PRIOR to doing most anything else with it. I do it as a patch on BSManagedDocument code that was never submitted as a commit on the repository. only on my local branch. This of course assumes you are still interested in BSManagedDocument since it has been almost a year since you posted this bug.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/karelia/BSManagedDocument/issues/50#issuecomment-429700432, or mute the thread https://github.com/notifications/unsubscribe-auth/AChna8TBEQjWwqHekV36OROhT8W5Wo18ks5ulAX-gaJpZM4QbASS.

Motti Shneor e-mail: motti.shneor@gmail.com phone: +972-8-9267730 mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

theMikeSwan commented 5 years ago

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 commented 5 years ago

On 15 Oct 2018, at 16:46, theMikeSwan notifications@github.com wrote:

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.

I haven’t yet upgraded my Mac to 10.14, and now I know why. It may be the first OS-X version I dislike even before installing…

I guess many CoreData+Document based apps set up their undo manager sometime while setting up the document’s managed object context — how do they cope? and what happens with NSManagedDocument in this respect?

I see this line of code (the last line of - (void)setManagedObjectContext:(NSManagedObjectContext *)context;) In the version I’m using

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’m doing quite a lot with the undo-manager, since my user actions are very frequently composed of several sub-actions that I group and name together - so to be able to revert them as a group. I don’t think I do this right while setting up - but I’ll inspect my code again.

BTW - to experience the crash, do you build again against the new (10.14) SDK? or simply run the old app on 10.14?

PI 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).

I will inspect my code again at home, and I’ll try to find a machine with 10.14 where I can exercise the code again. In the mean time - I need to warn my customers NOT to upgrade to 10.14 — as the app may crash on them. this is an important tool used by several dozen scientists on a daily basis - but they don’t regularly upgrade their OS.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/karelia/BSManagedDocument/issues/50#issuecomment-429858451, or mute the thread https://github.com/notifications/unsubscribe-auth/AChna6Lu4qCOrCjPzYvARXo9AirybvDnks5ulJGigaJpZM4QbASS.

Thank you!

Motti Shneor e-mail: motti.shneor@gmail.com phone: +972-8-9267730 mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

theMikeSwan commented 5 years ago

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 commented 5 years ago

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

https://github.com/karelia/BSManagedDocument/issues/47#issuecomment-493429031

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

evanmiller commented 5 years ago

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

theMikeSwan commented 5 years ago

@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 commented 5 years ago

@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.