On 13/07/2018, at 12:36 PM, Quincey Morris <quinceymorris@...> wrote:

On Jul 12, 2018, at 03:37 , Bill Pitcher <bill@...> wrote:

After returning from Revert To... restoring an old document, my document flicks back to displaying the original data when went into the version browser, BUT the backing document has been resorted correctly. Doing an UpdateMyControls in windowDidExitVersionBrowser does a bad visual, from new to old then new again.

Propagate the data reversion to your window controller and view controllers can be difficult, because any setup you did in windowDidLoad/viewDidLoad is not automatically redone. It can be especially confusing when the document (and the data model it provides/contains) is reverted, but individual references within you controller hierarchy refer to (and keep alive) various bits of the data model objects that were supposed to be discarded.

It’s also possible to be propagating the model data correctly to the view controllers, but still have the wrong information showing in the views because the propagation wasn’t fully KVO compliant.

There probably isn’t a simple way to solve this, other than the brute force approach of tracking the model data through the controllers to make sure it’s all being updated when necessary.

On 13/07/2018, at 5:08 PM, Quincey Morris <quinceymorris@...> wrote:

On Jul 12, 2018, at 18:22 , Bill Pitcher <bill@...> wrote:

Firstly I’ve uploaded a very cut down project which shows the problem.
https://www.ilike.co.nz/downloads/testDocument.zip

Well, you’re Doing It Wrong™, but it’s not entirely your fault. This is one of NSDocument’s big messes that we’ve been stuck with for the last 10 years or so.

What’s actually going wrong is that there are multiple document objects involved. Starting with your “latest edit” document (+ its controller hierarchy) A, when you call up the version browser, it displays A on the left and creates a new document B to display on the right. Assuming you choose to revert to B, what you see is B animate back to the original display position of A, while the rest of the version browser UI fades away.

That’s why you see the reversion happen correctly, seemingly. What happens next, though, is that document B is *closed*, since it was only a temporary adjunct needed for the version browser UI. When its window closes, A’s window is revealed underneath, and you see the content un-revert to the latest data that you had in A when this all started.

Why discard B and keep A, when B has the correct data? Because the reversion mechanism doesn’t want to leave you with a different instance of your NSDocument subclass (B) from the one you started with (A), since you might have code that knows and depends on its actual object pointer. It would break existing code if the pointer for the active document suddenly changed.

Instead, what actually happens is that (by default) your document’s reading method — read(from:ofType:) — is invoked anew *in A* but with the data from the reverted version store, and it causes A’s data model to be replaced with a new one that has the reverted data.

That would all be fine, except that your design doesn’t have any machinery to propagate this last data model change to your controller hierarchy. You have code to do it when the window controller’s “document” property changes, but it isn’t changing for document A here, so nothing happens.

There are a couple of things you should do differently:

— I *strongly* urge you not to override NSWindowController’s “document” property. NSWindowController is old-school, and even if the property is changed KVO-compliantly, there’s no guarantee it will actually go through the setter. (OTOH, I don’t think this has any impact on the current problem.)

— I *strongly* urge you not to make your view controller’s “representedObject” refer to the document, but rather to the root object of your data model.

— You need a way of getting notifications to the controller objects when the data model changes, rather than when the document changes. There are multiple approaches, but the one I usually use is to have the window controller observe its own “document.model” keypath (it would be “document.contents” in your sample project) to maintain its own “model” derived property KVO compliantly. Then I have the view controllers observe the window’s “model” property, and update their UI from that change notification. Normally, I don’t bother with the “representedObject” property (not least because it’s untyped and requires casting to be useful), but repeat the “model” property in the view controller if it’s useful to store it there too.

Note that there are other NSDocument override points that can intercept a revert operation earlier, if that’s useful. You might do that if recreating the entire document via read(from:ofType:) is expensive or has side effects, and you have a cheaper or safer way of doing the reversion, or if you want to be able to tell the difference between an “open” read(from:ofType:) and a “revert” one.

On 14/07/2018, at 2:25 AM, Quincey Morris <quinceymorris@...> wrote:

On Jul 13, 2018, at 03:49 , Bill Pitcher <bill@...> wrote:

Unless I’m mistaken KVO requires going back to NSObject for my model, and not plain Swift classes.

Well, things get more complicated when you use non-Obj-C types, but it’s not necessarily true that you can’t use Swift types. In the case of the model, it may be sufficient that your “model" property of NSDocument be @objc (but not a Swift-native type), to KVO-observe it.

WindowController.document is Any? so doesn’t seem observable but I didn’t spend long on checking that.

It’s still an @obj property of an Obj-C object, so it’s observable. The type is actually ‘id’, which translates into Swift ‘Any’ or ‘Any?’ these days, not into ‘AnyObject’ as it used to.

What I think you are saying is build a KVO compatible model and then link in observers through to all the Controllers.

I wasn’t really saying that, just saying you should be able to use KVO to keep track of the model’s root object. Whether you use KVO or Obj-C compatible objects or properties further down the line is up to you.

Note, however, that there is still a *lot* in built-in pressure to stay Obj-C-compatible in this part of your app design, because a lot of familiar design patterns rely on KVO and KVC.

On 24/07/2018, at 6:02 PM, Quincey Morris <quinceymorris@...> wrote:

On Jul 23, 2018, at 16:56 , Bill Pitcher <bill@...> wrote:

check that I’m still not Doing it Wrong ™

It’s more or less correct, although I think you’re still making it a bit harder than it needs to be. Here are some inline comments on your window controller:

class WindowController: NSWindowController {

@objc dynamic var kvoDocument: Document? // NSWindowController.document is AnyObject? so keep a KVO reference so we can observe the model

You don’t actually need this. The real “document” property is type ‘id’ in Obj-C because it’s following a very old Obj-C convention, and that happens to come across to Swift as “AnyObject?”. It is, however, *always* a subclass of NSDocument (unless of course you futzed with the document machinery, but in this case you know you didn’t).

KVC/KVO doesn’t care what the compile-time type of the property is, so you don’t need this auxiliary property, but can refer to the real one with a keypath. See below.

override func windowDidLoad() {
super.windowDidLoad()
// Do any additional setup after loading the view.
self.addObserver(self, forKeyPath: #keyPath(document) , options: [.old, .new], context: nil)
self.addObserver(self, forKeyPath: #keyPath(kvoDocument.model) , options: [.old, .new], context: nil)
}

— Swift has an observation overlay that’s much easier to use than the Obj-C ‘addObserver’ method. See below.

— Since you’re observing a keypath with 2 components, you don’t need to observe the first component separately. If the “document” property changes, there will always be a change notification for “document.model” too.

override func observeValue(forKeyPath keyPath: String?, of object: Any?, change: [NSKeyValueChangeKey : Any]?, context: UnsafeMutableRawPointer?) {
if keyPath == #keyPath(document) {
if let heldDocument = document as? Document {
self.kvoDocument = heldDocument
}
} else if keyPath == #keyPath(kvoDocument.model) {
if let heldViewController = self.contentViewController as? ViewController {
if let heldDocument = kvoDocument {
heldViewController.documentModel = heldDocument.model
heldViewController.documentUpdateChangeCountHandler = heldDocument.documentUpdateChangeCountHandler
}
}
}
}

deinit {
self.removeObserver(self, forKeyPath: #keyPath(document))
self.removeObserver(self, forKeyPath: #keyPath(kvoDocument.model))
}

With the Swift overlay, the awkward deinit isn’t necessary, either.

}

Revised version:

class WindowController: NSWindowController {
private var documentObservation: NSKeyValueObservation!
override func windowDidLoad() {
super.windowDidLoad()
documentObservation = self.observe(\WindowController.document.model) {
windowController,_ in
if let heldViewController = windowController.contentViewController as? ViewController,
let document = windowController.document as? Document {
heldViewController.documentModel = document.model
heldViewController.documentUpdateChangeCountHandler = document.documentUpdateChangeCountHandler
}
}
}

It’s not just shorter, but it’s clearer too, thanks to Swift’s improved ‘observe’ method. Note that the closure uses its window controller parameter rather than ‘self’. (If you capture self in this closure, you have to do it weakly, since a reference to the closure is stored and that would create a reference cycle with ’self’.)

To be pedantic, the “document” property of the window controller isn’t documented as being KVO compliant, so it’s not strictly safe to observe it. However, it always has been KVO compliant in practice, so it’s 101% unlikely that will ever change.

Note, also, that I removed the test whether the document is nil when updating the view controller. If that ever happens (and you may engineer it that way when things get more complicated, or it may happen temporarily when the window is being closed), you want to make sure it’s nil in the view controller, too. Otherwise, you may run into a reference cycle between your view controller and your document object, and that can lead to a hard-to-debug crash.

Related, in the view controller, I wouldn’t set the “documentModel” property initially to an empty document instance. When your document subclass gets more complicated, creating it (or destroying one) may have side effects, and you don’t want to cause problems via this empty instance.

A better choice is to declare it as an IUO:

var documentModel: DocumentModel!

and to be aware that it might actually be nil at certain times.