Re: Swift with NSDocument and Revert To... browse all versions.

Quincey Morris

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() {
// 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() {
        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.

Join to automatically receive all group messages.