karelia / BSManagedDocument

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

Closing the main window of a "dirty" BSManagedDocument, and choosing "Revert" erroneously re-opens the document. #32

Open Motti-Shneor opened 9 years ago

Motti-Shneor commented 9 years ago

Hi. Haven't debugged BSManagedDocument yet, but behavior is wrong (in the sense that it is unlike any other Mac application).

I open an existing document, and introduce a few changes using my App. I have the dot sign inside my window's "Close-box" (top left red button).

I press the close box, and receive the standard dialog warning me against closing without saving. I press the "Revert" button, an the document closes, but…

It immediately re-opens!!!!

My window controllers don't override or otherwise mess with the window closing behavior.

mikeabdullah commented 9 years ago

Hi, sorry it's taken so long to look at this. Been under a lot of strain lately.

First question: are you using the modern or legacy document saving approach?

Motti-Shneor commented 9 years ago

Thanks anyway. The issue is still there, and your help is much appreciated.

On 9 בדצמ 2014, at 19:30, Mike Abdullah notifications@github.com wrote:

Hi, sorry it's taken so long to look at this. Been under a lot of strain lately.

First question: are you using the modern or legacy document saving approach?

The simple answer - I don't know. Meaning --- my subclass of BSManagedDocument does not override or implement any "saving" methods, and even in the UI, I solely rely on the default action in standard template of the "File" menu "Save" item to do its job directly against BSManagedDocument implementation. I don't even know who gets the "save" action, and what action is it exactly...

The reason for this is - In the past, I tried to programmatically "save" my document at important times. My application is like a live-database interface, where users don't expect to need to save, but rather have the system save every change to disk. But trying to explicitly save my NSManagedDocument subclass caused so many crashes (internal autosaves collided with my saves etc.) that I finally completely gave up understanding how one should save - and reverted to the default behavior of your beautiful BSManagedDocument.

That said --- I still call

[self.document scheduleAutosaving];

Occasionally from my document window controllers, to preserve user actions, until user's next manual "Save" action.

and In my BSManagedDocument subclass, I have

but the program never enters this method (I verified this). Although I did not know how to better implement it (meaning, how to distinguish between my own scheduled autosaves, when I want to return YES, to the time we're closing the document, when I should return NO. I did not manage to understand BSManagedDocument implementation regarding to this, especially against the confusing documentation of NSDocument.

Anyhow - no one ever calls this, so it is not the cause.

Last point that might be of interest --- I'm building using Xcode 5.1.1 and 10.9 SDK, but my deployment OS is 10.8.

I hope you can help.

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

The simple answer - I don't know.

OK, simple test: do you override + autosavesInPlace to return YES?

  • (BOOL)autosavingIsImplicitlyCancellable { return YES; }

What made you think this is a good idea? I'm pretty sure Apple do not intend for this method to be overridden.

Motti-Shneor commented 9 years ago

On 9 בדצמ 2014, at 23:00, Mike Abdullah notifications@github.com wrote:

The simple answer - I don't know.

OK, simple test: do you override + autosavesInPlace to return YES?

I don't. You do. I see in BSManagedDocument.m that you override it like thus:

Which in my case will always return YES. (my SDK is 10.9, and minimum deployment is 10.8).

(BOOL)autosavingIsImplicitlyCancellable { return YES; } What made you think this is a good idea? I'm pretty sure Apple do not intend for this method to be overridden.

Reading NSDocument documentation, I came to think that it is the NSDocument subclass's responsibility to "report" when I'm explicitely autosaving. Then I replaced my implementation of explicit autosaving with the [document scheduleAutosave]; and this override can of course be removed. I now removed it, but it makes no difference regarding the issue of closing a dirty document.

  • (BOOL)autosavingIsImplicitlyCancellable Description Returns whether autosaving is happening now but could be safely cancelled. For example, when periodic autosaving is being done only for crash protection, which doesn’t need to be done all of the time, this method returns YES. When autosaving is being done because the document is being closed, this method returns NO. When this method returns YES your document-writing code can invokeunblockUserInteraction after recording the fact that changes to the document model made by the user should first cancel the rest of the writing. Your code that makes changes to the document model then must always do that cancellation first. If your writing code is implicitly cancelled in this way, it should set the NSError object passed by reference to the writing method to NSUserCancelledError in NSCocoaErrorDomain. Returns YES if autosaving is being done now but nothing bad would happen if it were cancelled; NO otherwise. Availability Available in OS X v10.7 and later.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

OK, good, so you're opted in to modern saving. In which case I have no idea offhand why you're seeing this behaviour and I'm not in Sandvox. Please can you post a sample project which demonstrates the issue.

I'm 99% sure autosavingIsImplicitlyCancellable is Apple's method to communicate to you. Not for you to override.

Motti-Shneor commented 9 years ago

On 10 בדצמ 2014, at 14:33, Mike Abdullah notifications@github.com wrote:

OK, good, so you're opted in to modern saving. In which case I have no idea offhand why you're seeing this behaviour and I'm not in Sandvox. Please can you post a sample project which demonstrates the issue.

I'll try to come up with such sample project today or tomorrow, but since my BSMangedDocument subclass is nearly empty (only minimal setup) and since all the rest of the application never actually calls any "save" or "autosave" directly - only using [document scheculeAutosave] occasionally, I can't see how my project is different from any other regarding this.

Anyway - I'll try.

I'm 99% sure autosavingIsImplicitlyCancellable is Apple's method to communicate to you. Not for you to override.—

OK - I removed it. But there is an ill effect to it - my application became sluggish for certain user actions - when I try to synchronize rapid user actions to my model. Every few clicks and double-click, I create an entity. Think of it as a drawing program where the user draws a polygon (series of clicks, concluded by a double-click). upon a double-click the program does some calculation, and creates an entity for the last polygon. The program then calls [document scheculeAutosave], and play some sound.

The documentation says this is fast, and does nothing if another autosave is already scheduled. However, since I removed autosavingIsImplicitlyCancellable my program takes few seconds from the double-click to the "beep" when user can start a new polygon.

This is a measurement tool over live microscope image - so responsiveness is important. Yet - persistence is very important too. I really want my document to persist every new measurement entity in a matter of seconds.

Since I failed to "save' and "autosave" programmatically (I got so many weird crashes, I simply gave up) I simply schedule autosaves, which seems the most lightweight requirement from the document architecture to protect my changes against crashes, power-failure etc.

Have you any idea?

Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

Motti-Shneor commented 9 years ago

Another reply for the same message - sorry. I had a tiny debug session on the issue and saw it straightforward:

Your implementation ALWAYS re-opens and re-shows the document window controllers: I've put a breakpoint in the @finally block (look below) and I get there every time I close my document's main window, and ask to "Revert changes".

I believe this behavior will be in EVERY application using BSManagedDocument. I'll make a project, but i think there's no reason to - the code is very clear. Any call to revertToContentsOfURL:(NSURL )absoluteURL ofType:(NSString )typeName error:(NSError )outError

will result in re-opening the windows of the document. I don't know where and how you can ask the document architecture "Am I closing the document now" to refrain from doing so --- but obviously you don't.

Here is the relevant BSManagedDocument.m method:

On 10 בדצמ 2014, at 14:33, Mike Abdullah notifications@github.com wrote:

OK, good, so you're opted in to modern saving. In which case I have no idea offhand why you're seeing this behaviour and I'm not in Sandvox. Please can you post a sample project which demonstrates the issue.

I'm 99% sure autosavingIsImplicitlyCancellable is Apple's method to communicate to you. Not for you to override.

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

This is a measurement tool over live microscope image - so responsiveness is important. Yet - persistence is very important too. I really want my document to persist every new measurement entity in a matter of seconds.

Why? Is the power likely to fail at any moment? Are you expecting your app to crash very frequently?

Bear in mind that in addition to the work of saving the document, you're also throwing at the OS the work to create and maintain backup copies of each document in the Versions history.

Have you any idea?

Step 1: Measure. Why is it slow/sluggish?

mikeabdullah commented 9 years ago

Your implementation ALWAYS re-opens and re-shows the document window controllers: I've put a breakpoint in the @finally block (look below) and I get there every time I close my document's main window, and ask to "Revert changes".

Ah, what do you have in System Prefs > General > Ask to keep changes when closing documents? I suspect you've got that box ticked, and I haven't.

Motti-Shneor commented 9 years ago

Indeed, I have it checked. And this is the default behavior.

You can now very easily recreate the issue, and compare it with, let's say, TextEdit behavior.

Question is --- how do you know there in the context of that method, that you should not re-open the document? or should have you been going a different route?

I simply don't know enough about the Document architecture to fix this myself.

On 11 בדצמ 2014, at 12:04, Mike Abdullah notifications@github.com wrote:

Your implementation ALWAYS re-opens and re-shows the document window controllers: I've put a breakpoint in the @finally block (look below) and I get there every time I close my document's main window, and ask to "Revert changes".

Ah, what do you have in System Prefs > General > Ask to keep changes when closing documents? I suspect you've got that box ticked, and I haven't.

— Reply to this email directly or view it on GitHub.

Motti Shneor, Mac OS X Software Architect & Team Leader Spectrum Reflections Ltd. +972-54-3136621

mikeabdullah commented 9 years ago

Ah, think I've got enough to take a look myself now then.

mikeabdullah commented 9 years ago

I think normally, NSDocuments are expected to do the restore — i.e. re-read in the reverted data — and then go on to close as normal. There's probably an expectation the user won't see anything too weird as most windows will close before they actually perform any drawing due to the revert.

But Core Data forces us to take a different route, of closing down the window controller stack and rebuilding it. That rebuild is really not what the document system is expecting or equipped to handle during document closing!

I think a little bit of problem here as well as that the document doesn't actually get fully closed down. It's windows are closed, but it looks to me like Apple actually keep the document instance around ready to re-use if the document is re-opened (this smells fishy to me, and perhaps it's something specific to Sandvox). But as a result I think us opening a fresh window is a problem because the document system doesn't then close it; not that the system can't handle extra windows.

So ideally I think we want to:

  1. Tear down window controllers
  2. Revert the Core Data stack
  3. Rebuild window controllers — so document can be re-opened later if desired
  4. Not show the windows if in the middle of closing

Unfortunately there appear to be no APIs for detecting the middle of closing. Two ideas so far:

  1. Does the window have a sheet on it? If so, assume that's the sheet asking if doc should be reverted etc.
  2. Track if document is closing ourselves

I'm favouring option 2; 1 seems pretty risky.

mikeabdullah commented 9 years ago

OK, what do you make of https://github.com/karelia/BSManagedDocument/pull/34 ?

Motti-Shneor commented 9 years ago

I think NSDocument shouldn't do any "restore" when user chooses the "Revert" option when the Closing scenario.

The purpose of reverting-while-closing, is to NOT re-read the document from disk, but the opposite --- Suppress the saving of changes. It should be, in principle, the most lightweight route. Get rid of anything the user has done, forget about "dirtiness", and close everything, leaving the the original document on disk intact.

Isn't "closing" the time where you free all your document resources, and get rid of all its objects?

In the complex world of autosave-in-place, and other stuff you were doing, it might not be so simple to "revert while closing" especially if you created temporary stuff, or renamed, or else-changed the original, but on the whole, that's the approach.

I don't know how NSDocument (or actually NSManagedDocument) manages this. User-wise, as simple sample program behaves as expected, and I guess it does not restore the in-memory document structures (the core-data contexts etc.) upon closing.

In any way, I saw your pushed commit to BSManagedDocument, and could not but wonder why you re-create the document controllers when _closing is true. What use will they have?

They are NSWindowControllers, and we're just closing the whole thing...

On 12 בדצמ 2014, at 20:14, Mike Abdullah notifications@github.com wrote:

I think normally, NSDocuments are expected to do the restore — i.e. re-read in the reverted data — and then go on to close as normal. There's probably an expectation the user won't see anything too weird as most windows will close before they actually perform any drawing due to the revert.

But Core Data forces us to take a different route, of closing down the window controller stack and rebuilding it. That rebuild is really not what the document system is expecting or equipped to handle during document closing!

I think a little bit of problem here as well as that the document doesn't actually get fully closed down. It's windows are closed, but it looks to me like Apple actually keep the document instance around ready to re-use if the document is re-opened (this smells fishy to me, and perhaps it's something specific to Sandvox). But as a result I think us opening a fresh window is a problem because the document system doesn't then close it; not that the system can't handle extra windows.

So ideally I think we want to:

Tear down window controllers Revert the Core Data stack Rebuild window controllers — so document can be re-opened later if desired Not show the windows if in the middle of closing Unfortunately there appear to be no APIs for detecting the middle of closing. Two ideas so far:

Does the window have a sheet on it? If so, assume that's the sheet asking if doc should be reverted etc. Track if document is closing ourselves I'm favouring option 2; 1 seems pretty risky.

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

I think NSDocument shouldn't do any "restore" when user chooses the "Revert" option when the Closing scenario. The purpose of reverting-while-closing, is to NOT re-read the document from disk, but the opposite --- Suppress the saving of changes. It should be, in principle, the most lightweight route. Get rid of anything the user has done, forget about "dirtiness", and close everything, leaving the the original document on disk intact.

Well, like it or not, Apple have chosen to implement it by asking documents to restore. I'm not keen to try and go against that unless I have to.

In any way, I saw your pushed commit to BSManagedDocument, and could not but wonder why you re-create the document controllers when _closing is true. What use will they have?

Two reasons:

Motti-Shneor commented 9 years ago

Hi, I don't know if that's the On 13 בדצמ 2014, at 21:32, Mike Abdullah notifications@github.com wrote:

I think NSDocument shouldn't do any "restore" when user chooses the "Revert" option when the Closing scenario. The purpose of reverting-while-closing, is to NOT re-read the document from disk, but the opposite --- Suppress the saving of changes. It should be, in principle, the most lightweight route. Get rid of anything the user has done, forget about "dirtiness", and close everything, leaving the the original document on disk intact.

Well, like it or not, Apple have chosen to implement it by asking documents to restore. I'm not keen to try and go against that unless I have to.

I'm not sure you're correct. I just built a venerable NSPersistentDocument based sample from Apple, I overridden 2 document methods

Just to break in the debugger, and tried the behavior with my application (based on BSManagedDocument) and the sample based on "MyDocument" subclass of NSPersistentDocument.

Here are my findings.

  1. When closing a document window, and choosing "Dont Save" (the standard dialog doesn't say "Revert" in the apple sample code, just "Don't save") Only the "close" method is being called (from the NSWindowController's _windowDidClose). The "revert" is never being called. However, If I choose "revert" from the file menu - the revertToContentsOfURL:ofType:error: DOES get called.
  2. When closing a non-dirty document, there's also a difference. In My (BSManagedDocument) app, I see the "close" gets called TWICE recursively. First call has more-or-less the same stack as the Apple sample code, (app calls NSWindowController's _windowDidClose, which calls MyDocument close). But when I continue --- I break there again, this time in a double stack (see below).

Thread 1, Queue : com.apple.main-thread

0 0x0000000100005a54 in -[BSManagedDocument close] at /Users/motti/PlanktoMetrix II Repository/External/BSManagedDocument/BSManagedDocument.m:209

1 0x00007fff8b48ba73 in -[NSWindowController _windowDidClose]()

2 0x00007fff8b1d9125 in -[NSDocument close]()

3 0x0000000100005a76 in -[BSManagedDocument close] at /Users/motti/PlanktoMetrix II Repository/External/BSManagedDocument/BSManagedDocument.m:209

4 0x00007fff8b48ba73 in -[NSWindowController _windowDidClose]()

5 0x00007fff8b44487c in -[NSWindow _close]()

6 0x00007fff8b1d8f53 in -[NSDocument _something:didSomething:soContinue:]()

7 0x00007fff8b5d819b in **block_global_155 ()

8 0x00007fff8b1d8f53 in -[NSDocument _something:didSomething:soContinue:]()

9 0x00007fff8b1ae59e in __block_global_151 ()

10 0x00007fff8b1837df in __67-[NSDocument autosaveWithImplicitCancellability:completionHandler:]_block_invoke_0 ()

11 0x00007fff8b17e8a9 in -[NSDocument continueFileAccessUsingBlock:]()

12 0x00007fff8b1831b4 in __block_global_18 ()

13 0x00007fff8b5f8662 in __block_global_44 ()

14 0x00007fff91036cd2 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK** ()

15 0x00007fff90ff747f in __CFRunLoopDoBlocks ()

16 0x00007fff9101b7e4 in __CFRunLoopRun ()

17 0x00007fff9101b0e2 in CFRunLoopRunSpecific ()

18 0x00007fff8c9bceb4 in RunCurrentEventLoopInMode ()

19 0x00007fff8c9bcb94 in ReceiveNextEventCommon ()

20 0x00007fff8c9bcae3 in BlockUntilNextEventMatchingListInMode ()

21 0x00007fff8b2ae533 in _DPSNextEvent ()

22 0x00007fff8b2addf2 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]()

23 0x00007fff8b2a51a3 in -[NSApplication run]()

24 0x00007fff8b249bd6 in NSApplicationMain ()

25 0x0000000100014c02 in main at /Users/motti/IOLR/PlanktoMetrix II Repository/PlanktoMetrix-II/main.m:13

In any way, I saw your pushed commit to BSManagedDocument, and could not but wonder why you re-create the document controllers when _closing is true. What use will they have?

Two reasons:

The fewer things I have to special-case, the better I found in practice the system seems to hang on to the document, expecting the window controllers to be there too, ready in case the document is re-opened later. If you tear down the window controllers, they don't get created again as part of that, and so the document never appears on-screen again. I'll comment the code up a bit. Who is "The System". I hardly believe someone caches documents, which can be very big in memory (think "Pages", or Xcode, or iMovie documents.)

I wish I knew more about the document architecture to help --- maybe I'll dig into this myself.

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

Motti-Shneor commented 9 years ago

Hello again, and sorry for hammering on this point ---

I must say I suffer more and more ill effects of the current implementation, regarding "Revert". Now I have another bad scenario. I added some CoreData validation on some of my entities. (few KVC validations and overrides on validateForUpdate:(NSError _)error and validateForInsert:(NSError _)error.

Now If my document is dirty and contains invalid attributes (say, a zero where nonzero number is needed) and at that time the user tries to close the document's main window, and chooses to "revert" -- he's stuck, because you try to save! Saving drives the validation mechanism, which fails and emits errors to the user, who is forced to cancel the "Close and Revert" action. But the user asked to revert, and expects this to simply get rid of all changes since last save.

The stack shows that the "saveOperationKind" is 3 (autosave), and Apple documentation clearly differentiates an "autosave" from a "save" by saying "autosave is not validated, and can hold intermediate invalid states of the model". Clearly this is not the case with BSManagedDocument, Or I'm missing something very big.

I verified that this behavior is not happening in NSPersistentDocument implementation. Do you have any idea?

On 13 בדצמ 2014, at 21:32, Mike Abdullah notifications@github.com wrote:

I think NSDocument shouldn't do any "restore" when user chooses the "Revert" option when the Closing scenario. The purpose of reverting-while-closing, is to NOT re-read the document from disk, but the opposite --- Suppress the saving of changes. It should be, in principle, the most lightweight route. Get rid of anything the user has done, forget about "dirtiness", and close everything, leaving the the original document on disk intact.

Well, like it or not, Apple have chosen to implement it by asking documents to restore. I'm not keen to try and go against that unless I have to.

In any way, I saw your pushed commit to BSManagedDocument, and could not but wonder why you re-create the document controllers when _closing is true. What use will they have?

Two reasons:

The fewer things I have to special-case, the better I found in practice the system seems to hang on to the document, expecting the window controllers to be there too, ready in case the document is re-opened later. If you tear down the window controllers, they don't get created again as part of that, and so the document never appears on-screen again. I'll comment the code up a bit. — Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

NSPersistentDocument doesn't opt into the modern saving model by default, and I don't believe Apple support it being opted in. You're comparing two very different setups.

If you can give me a sample app of the problems, I can take a closer look.

Motti-Shneor commented 9 years ago

Making up a sample app for this is a little too much work because I need to implement quite a lot to have this scenario. However I don't mind sending you a zip of my real program - only please delete it after inspecting, since it actually belongs to a client. (Nothing confidential there, but they still have the rights).

Can you give me a less public e-mail to which I can end it?

On 17 בדצמ 2014, at 22:28, Mike Abdullah notifications@github.com wrote:

NSPersistentDocument doesn't opt into the modern saving model by default, and I don't believe support it being opted in. You're comparing two very different setups.

If you can give me a sample app of the problems, I can take a closer look.

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

Motti-Shneor commented 9 years ago

In the mean time - here's scenario and the stack:

User clicks the document's main window's close-box. A dialog opens with "Revert Changes", "Cancel" and "Save" buttons. User clicks Revert Changes.

I have a breakpoint in my validation methods, and here is the stack when I break there. #0 is my NSManagedObject subclass PMWaterSample's KVC validation method for an attribute named "initialVolume".

Thread 1, Queue : com.apple.main-thread

0 0x000000010008fc23 in -[PMWaterSample(PMExtensions) validateInitialVolume:error:] at /Users/motti/planktometrix-ii - GitLab Repository/PlanktoMetrix-II/Model Classes/PMExtensions/PMWaterSample+PMExtensions.m:72

1 0x00007fff83db74ef in -[NSManagedObject(_NSInternalMethods) _validateValue:forProperty:andKey:withIndex:error:]()

2 0x00007fff83db735a in -[NSManagedObject(_NSInternalMethods) _validatePropertiesWithError:]()

3 0x00007fff83db720c in -[NSManagedObject(_NSInternalMethods) _validateForSave:]()

4 0x00007fff83dcd450 in -[NSManagedObject validateForInsert:]()

5 0x00007fff83db6e19 in -[NSManagedObjectContext(_NSInternalAdditions) _validateObjects:forOperation:error:exhaustive:forSave:]()

6 0x00007fff83db6b73 in -[NSManagedObjectContext(_NSInternalAdditions) _validateChangesForSave:]()

7 0x00007fff83db6936 in -[NSManagedObjectContext(_NSInternalChangeProcessing) _prepareForPushChanges:]()

8 0x00007fff83db49d6 in -[NSManagedObjectContext save:]()

9 0x00000001000064d0 in -[BSManagedDocument contentsForURL:ofType:saveOperation:error:] at /Users/motti/planktometrix-ii - GitLab Repository/External/BSManagedDocument/BSManagedDocument.m:326

10 0x0000000100008de0 in -[BSManagedDocument writeToURL:ofType:forSaveOperation:originalContentsURL:error:] at /Users/motti/planktometrix-ii - GitLab Repository/External/BSManagedDocument/BSManagedDocument.m:756

11 0x00007fff8c9542ca in -[NSDocument _writeSafelyToURL:ofType:forSaveOperation:forceTemporaryDirectory:error:]()

12 0x00007fff8c954054 in -[NSDocument _writeSafelyToURL:ofType:forSaveOperation:error:]()

13 0x00007fff8c953bd9 in -[NSDocument writeSafelyToURL:ofType:forSaveOperation:error:]()

14 0x0000000100008bf0 in -[BSManagedDocument writeSafelyToURL:ofType:forSaveOperation:error:] at /Users/motti/planktometrix-ii - GitLab Repository/External/BSManagedDocument/BSManagedDocument.m:733

15 0x00007fff8cd94e26 in -[NSDocument _preserveCurrentVersionForReason:error:]()

16 0x00007fff8cd927e4 in __block_global_21 ()

17 0x00007fff8fd0cd74 in -[NSFileCoordinator _invokeAccessor:thenCompletionHandler:]()

18 0x00007fff8fcf00a4 in __73-[NSFileCoordinator coordinateWritingItemAtURL:options:error:byAccessor:]_block_invoke_0 ()

19 0x00007fff8fd0cbce in -[NSFileCoordinator(NSPrivate) _invokeAccessor:orDont:thenRelinquishAccessClaimForID:]()

20 0x00007fff8fcef91a in -[NSFileCoordinator(NSPrivate) _coordinateWritingItemAtURL:options:error:byAccessor:]()

21 0x00007fff8fcef63f in -[NSFileCoordinator coordinateWritingItemAtURL:options:error:byAccessor:]()

22 0x00007fff8cd9265a in __53-[NSDocument _revertToVersion:preservingFirst:error:]_block_invoke_0 ()

23 0x00007fff8c94d156 in -[NSDocument performSynchronousFileAccessUsingBlock:]()

24 0x00007fff8cd924bd in -[NSDocument _revertToVersion:preservingFirst:error:]()

25 0x00007fff8cd934d2 in -[NSDocument _revertToDiscardRecentChangesPreservingFirst:error:]()

26 0x00007fff8cda777d in __block_global_160 ()

27 0x00007fff8c94d996 in __53-[NSDocument performSynchronousFileAccessUsingBlock:]_block_invoke_0 ()

28 0x00007fff8c94d8a9 in -[NSDocument continueFileAccessUsingBlock:]()

29 0x00007fff8c94d472 in -[NSDocument _performFileAccessOnMainThread:usingBlock:]()

30 0x00007fff8c94d1b8 in -[NSDocument performSynchronousFileAccessUsingBlock:]()

31 0x00007fff8cda761e in __block_global_159 ()

32 0x00007fff8cd8f0c3 in -[NSDocument _something:wasPresentedWithResult:soContinue:]()

33 0x00007fff8ccba6a7 in -[NSAlert didEndAlert:returnCode:contextInfo:]()

34 0x00007fff8c97aac6 in -[NSApplication endSheet:returnCode:]()

35 0x00007fff8ccba9a6 in -[NSAlert buttonPressed:]()

36 0x00007fff8cb6d959 in -[NSApplication sendAction:to:from:]()

37 0x00007fff8cb6d7b7 in -[NSControl sendAction:to:]()

38 0x00007fff8cb6d6eb in -[NSCell _sendActionFrom:]()

39 0x00007fff8cb6bbd3 in -[NSCell trackMouse:inRect:ofView:untilMouseUp:]()

40 0x00007fff8cb6b421 in -[NSButtonCell trackMouse:inRect:ofView:untilMouseUp:]()

41 0x00007fff8cb6ab9c in -[NSControl mouseDown:]()

42 0x00007fff8cb6250e in -[NSWindow sendEvent:]()

43 0x00007fff8cb5e644 in -[NSApplication sendEvent:]()

44 0x00007fff8ca7421a in -[NSApplication run]()

45 0x00007fff8ca18bd6 in NSApplicationMain ()

46 0x0000000100014a12 in main at /Users/motti/planktometrix-ii - GitLab Repository/PlanktoMetrix-II/main.m:13

On 17 בדצמ 2014, at 22:28, Mike Abdullah notifications@github.com wrote:

NSPersistentDocument doesn't opt into the modern saving model by default, and I don't believe support it being opted in. You're comparing two very different setups.

If you can give me a sample app of the problems, I can take a closer look.

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

My guess is the process goes something like this:

  1. Get document into invalid state; persistence of that not attempted yet
  2. Close > Revert
  3. The document system wants to first save the document in its current state for the benefit of Versions
  4. That save internally asks Core Data to save the store, which fails because it fails validation rules
  5. That failure propagates back out to the document system which doesn't really know how to handle it

You mentioned the idea that invalid document states should be tolerated for autosave. Sadly Core Data doesn't really fit into that plan. But I guess if you wanted, you could override saving to ignore certain classes of error in certain situations. That would probably cover the problem described above too.

Can we hold off on sending the program for the moment, please? I'd quite like to avoid digging through someone else's full-fledged app!

Motti-Shneor commented 9 years ago

My "Full-fledge" app is pretty small and concise. I wouldn't suggest sending it if i though you need to actually "dig" in it. I could send you just the main classes involved (my BSManagedDocument subclass, my main document window controller, and a few demonstrative NSManagedObject subclasses from my model. I just thought it would be much simpler and straight-forward for you to open-and-run something build-able out of the box, and stop in YOUR code where you need.

The scenario you describe below is reasonable, but contradicts "document system" logic. When user closes and reverts -- what need is there is for a save, and a version at all? User just explicitly dismissed all changes since last save. Could this be an Apple logic issue?

Now for my case - I don't need, and don't want versions at all!

My document is not a "Word" style document. My document is long-lived, regularly updating, over YEARS of use. A bit like an iTunes library, or iPhoto library. I'm more like a full-fledge SQL database with millions of entities, and an expected store size of few hundred megabytes. I build on CoreData's "Database" abilities to update this store quickly, and the whole "versions" theme simply does not fit my needs. Could this be "configured out" of the system? versioning could kill user-experience here, because of huge duplications, even if they're done in the background.

I do need autosaving, though, exactly for the reasons mentioned in the documentation:

  1. Prevent loss of data due to crash/power-failure etc.
  2. persist intermediate, pre-validated data.

Again --- this kind-of-worked for me when I used NSPersistentDocument, but I experienced very weird crashes then, due to which I turned to BSManagedDocument.

On 19 בדצמ 2014, at 16:46, Mike Abdullah notifications@github.com wrote:

My guess is the process goes something like this:

Get document into invalid state; persistence of that not attempted yet Close > Revert The document system wants to first save the document in its current state for the benefit of Versions That save internally asks Core Data to save the store, which fails because it fails validation rules That failure propagates back out to the document system which doesn't really know how to handle it You mentioned the idea that invalid document states should be tolerated for autosave. Sadly Core Data doesn't really fit into that plan. But I guess if you wanted, you could override saving to ignore certain classes of error in certain situations. That would probably cover the problem described above too.

Can we hold off on sending the program for the moment, please? I'd quite like to avoid digging through someone else's full-fledged app!

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

I expect that Apple have chosen to save the document at that point because even though the user is reverting it, they may wish to return to that present state later. Thus it is saved to be filed away in Versions.

It sounds like you want to disable Versions, so try doing exactly that. Override +preservesVersions to return NO. Any better?

Motti-Shneor commented 9 years ago

Thanks a lot.

I wasn't even aware of "versions" in my app... I'll override, and report any change of behavior.

On 23 בדצמ 2014, at 16:08, Mike Abdullah notifications@github.com wrote:

I expect that Apple have chosen to save the document at that point because even though the user is reverting it, they may wish to return to that present state later. Thus it is saved to be filed away in Versions.

It sounds like you want to disable Versions, so try doing exactly that. Override +preservesVersions to return NO. Any better?

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

Motti-Shneor commented 9 years ago

Hi Mike. Please excuse this lengthy e-mail but I really get puzzled the more I dig into this, and my application behaves more and more bizarre as I try to resolve the problems.

First - you said in a former e-mail

You mentioned the idea that invalid document states should be tolerated for autosave. Sadly Core Data doesn't really fit into that plan. But I guess if you wanted, you could override saving to ignore certain classes of error in certain situations. That would probably cover the problem described above too.

But my app, based on your BSManagedDocument and CoreData did just that! It surely autosaves INVALID states of my model every 10 seconds, without a hitch.

I open a document. I do some work (that can't be saved - I have validation errors displayed if I try to save). The red close button on the window also indicates dirtiness (black dot in the middle). I wait a few seconds, and then force-quit or crash my app, and relaunch it.

-- Voila, all my work is there, the context contains all the invalid changes, together with the dirty indication on the window, and the inability to save. If core-data didn't save it --- where was it preserved? I never did anything but apply changes to my ManagedObjectContext and schedule for autosave.

So --- Maybe CoreData is fit for this after all?

Next. Overriding preservesVersions like thus

+(BOOL) preservesVersions { return NO; }

doesn't seem to help me much. First, NSDocument header says:

/* Return YES if the receiving subclass of NSDocument supports Mac OS 10.7 version preservation, NO otherwise. The default implementation of this method returns [self autosavesInPlace]. You can override it and return NO to declare that NSDocument should not preserve old document versions.

Returning NO from this method will disable version browsing and -revertDocumentToSaved:, which rely on version preservation when autosaving in place. Returning YES from this method when +autosavesInPlace returns NO will result in undefined behavior. */

and Indeed I now have the "Revert to Saved" disabled in the File menu. Sad and undesirable for the user.

If I go on and try to close the dirty document window, I still receive the normal Revert/Save/Cancel closing-dialog, but choosing to "Revert" still re-opens the document as before. (This is without your latest fix on BSManagedDocument). Stopping in the debugger I see the same behavior (stopping twice in the same place along the closing scenario etc.).

So your theory about NSDocument trying to preserve a version when I'm closing the document may be inaccurate. Something else may be going on there.

B.T.W. I'm using it as a git submodule, and you haven't delivered the fix to your main branch - how should I go about this?.

Now reading the preservesVersions documentation I saw this method is strongly intertwined with +(BOOL) autoSavesInPlace; which you overridden in BSManagedDocument to return YES (on MacOS 10.7 or later).

Now I'm really puzzled. If until now we "autosaved in place" (meaning inside our original persistent store?) HOW COULD IT BE that I have invalid changes autosaved ?

Maybe turning off versions is not enough, and I must autosave "elsewhere" (not "InPlace") ??

If I override to not autosavesInPlace, where do I autosave? how does it work?

The documentation is quite frustrating, again.

/* Return YES if the receiving subclass of NSDocument supports Mac OS 10.7 autosaving in place, NO otherwise. The default implementation of this method returns NO. You can override it and return YES to declare your NSDocument subclass' ability to do Mac OS 10.7 autosaving in place. You should not invoke this method to find out whether autosaving in place is actually being done at any particular moment. You should instead use the NSSaveOperationType parameter passed to your overrides of -save... and -write... methods.

AppKit invokes this method at a variety of times, and not always on the main thread. For example, -autosaveWithImplicitCancellability:completionHandler: invokes this as part of determining whether the autosaving will be an NSAutosaveInPlaceOperation instead of an NSAutosaveElsewhereOperation. For another example, -canCloseDocumentWithDelegate:shouldCloseSelector:contextInfo: and NSDocumentController's machinery for handling unsaved changes at application termination time both invoke this as part of determining whether alerts about unsaved changes should be presented to the user. */

Please look at the red section. No matter what I return in + (BOOL)autosavesInPlace, User still receives the alert about unsaved changes when closing the document. Strangely enough - no such alert is displayed when user just Quits the application without saving. (Changes are preserved though, with or without versioning turned on).

Have you any idea where should I go from here?

Motti.

On 23 בדצמ 2014, at 16:08, Mike Abdullah notifications@github.com wrote:

I expect that Apple have chosen to save the document at that point because even though the user is reverting it, they may wish to return to that present state later. Thus it is saved to be filed away in Versions.

It sounds like you want to disable Versions, so try doing exactly that. Override +preservesVersions to return NO. Any better?

— Reply to this email directly or view it on GitHub.

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

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

mikeabdullah commented 9 years ago

I've seen similar behaviour by turning Versions off. Seems your choices are:

Perhaps the legacy approach suits your app better then, I don't know.

I open a document. I do some work (that can't be saved - I have validation errors displayed if I try to save). The red close button on the window also indicates dirtiness (black dot in the middle). I wait a few seconds, and then force-quit or crash my app, and relaunch it.

Voila, all my work is there, the context contains all the invalid changes, together with the dirty indication on the window, and the inability to save. If core-data didn't save it --- where was it preserved? I never did anything but apply changes to my ManagedObjectContext and schedule for autosave.

If it got persisted in some fashion, then Core Data agreed to save it. I can only deduce your validation logic takes place up in the document or similar instead, rather than down in the model.

I don't mean to be overly harsh here, but this discussion is taking up quite a bit of my valuable time. It sounds like you have quite a confusing setup. If you can boil it down to a sample app, that would likely be a lot easier to debug, and educational to yourself.