-
Notifications
You must be signed in to change notification settings - Fork 25
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
Closing the main window of a "dirty" BSManagedDocument, and choosing "Revert" erroneously re-opens the document. #32
Comments
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? |
Thanks anyway. The issue is still there, and your help is much appreciated. On 9 בדצמ 2014, at 19:30, Mike Abdullah [email protected] wrote:
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
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.
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
OK, simple test: do you override
What made you think this is a good idea? I'm pretty sure Apple do not intend for this method to be overridden. |
On 9 בדצמ 2014, at 23:00, Mike Abdullah [email protected] wrote:
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).
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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 |
On 10 בדצמ 2014, at 14:33, Mike Abdullah [email protected] wrote:
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.
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?
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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.
Step 1: Measure. Why is it slow/sluggish? |
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. |
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 [email protected] wrote:
Motti Shneor, Mac OS X Software Architect & Team Leader |
Ah, think I've got enough to take a look myself now then. |
I think normally, 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:
Unfortunately there appear to be no APIs for detecting the middle of closing. Two ideas so far:
I'm favouring option 2; 1 seems pretty risky. |
OK, what do you make of #34 ? |
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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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.
Two reasons:
|
Hi, I don't know if that's the
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.
Thread 1, Queue : com.apple.main-thread
I wish I knew more about the document architecture to help --- maybe I'll dig into this myself.
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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. |
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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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 #33 0x00007fff8ccba6a7 in -NSAlert didEndAlert:returnCode:contextInfo: On 17 בדצמ 2014, at 22:28, Mike Abdullah [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
My guess is the process goes something like this:
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! |
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:
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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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 |
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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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
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 { 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 [email protected] wrote:
Motti Shneor mobile: +972-54-3136621Ceterum censeo Microsoftinem delendam esse |
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.
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. |
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.
The text was updated successfully, but these errors were encountered: