Implementing undo in custom model object for lots of properties

  • Hi all,

    I have a custom data model object with a number of properties of various basic types.  An array of these is managed by an NSArrayController and bound to an NSTableView (as the reader might recall from my earlier thread).  I am not using Core Data.

    I am now wiring in undo support.  Were the data backed by an NSManagedObjectContext, I would get undo behaviour for free, but since there isn't, I need to write my own setters to handle it.

    It seems onerous, verbose and error-prone to have to implement a custom stub setter method for every property like so:

    - (void)setBlah:(Blah *)newBlah
    {
    if (newBlah != blah)
      {
      [undoManager registerUndoWithTarget:self selector:@selector(setBlah:) object:blah];
      [blah release];
      blah = [newBlah copy];
      }
    }

    Thus, I have tried to be clever by doing the following override:

    - (void)setValue:(id)value forKey:(NSString *)key
    {
    NSArray *undoableKeys = [NSArray arrayWithObjects: @"blah", @"foo", @"anotherProperty", nil];

    if ([undoableKeys containsObject:key])
      {
      [[undoManager prepareWithInvocationTarget:self] setValue:[self valueForKey:key] forKey:key];
      }

    [super setValue:value forKey:key];

    return;
    }

    However, the undo event does not seem to be recorded -- the Undo menu command remains ghosted.  It is not clear to me why.

    It wouldn't surprise me to be admonished for such an abuse of setValue:forKey:, but is there a better way?  I imagine this idiom must be extremely commonplace, but I could not find a clear directive.

    I looked to Apple's "DemoMonkey" sample for its undo practices, and it just implements the verbose boilerplate for each property.  Is that really the recommended solution?

    thanks,

    -ben

    --
    Ben Kennedy, chief magician
    Zygoat Creative Technical Services
    http://www.zygoat.ca
  • Hi Ben,

    I think this undo approach should work fine.

    Another way to 'centralise' undo is to have an object listen for the KVO notifications of changes for properties it's interested in, and use the observation method to record to the undo manager. You'd still use setValue:forKey: at undo time, so it amounts to a very similar idea. I've done it that way and it works.

    So your problem must be something else. Are you sure undoManager returns something?

    Also, more obvious now I come to think about it, is that setValue:forKey: is not called for every property when it is set! That method can be used to call down to the setter for a named property, but if the setter is invoked directly (the more usual case), then setValue:forKey: isn't invoked. You might want to check whether that is what's happening. In which case making your undo handler a KVO observer will get around that problem, or you could override -willChangeValueForKey: instead, which is the method that causes some of the KVO "magic" to happen.

    --Graham

    On 08/05/2012, at 9:11 AM, Ben Kennedy wrote:

    > I have a custom data model object with a number of properties of various basic types.  An array of these is managed by an NSArrayController and bound to an NSTableView (as the reader might recall from my earlier thread).  I am not using Core Data.
    >
    > I am now wiring in undo support.  Were the data backed by an NSManagedObjectContext, I would get undo behaviour for free, but since there isn't, I need to write my own setters to handle it.
    >
    > It seems onerous, verbose and error-prone to have to implement a custom stub setter method for every property like so:
    >
    > - (void)setBlah:(Blah *)newBlah
    > {
    > if (newBlah != blah)
    > {
    > [undoManager registerUndoWithTarget:self selector:@selector(setBlah:) object:blah];
    > [blah release];
    > blah = [newBlah copy];
    > }
    > }
    >
    > Thus, I have tried to be clever by doing the following override:
    >
    > - (void)setValue:(id)value forKey:(NSString *)key
    > {
    > NSArray *undoableKeys = [NSArray arrayWithObjects: @"blah", @"foo", @"anotherProperty", nil];
    >
    > if ([undoableKeys containsObject:key])
    > {
    > [[undoManager prepareWithInvocationTarget:self] setValue:[self valueForKey:key] forKey:key];
    > }
    >
    > [super setValue:value forKey:key];
    >
    > return;
    > }
    >
    > However, the undo event does not seem to be recorded -- the Undo menu command remains ghosted.  It is not clear to me why.
    >
    > It wouldn't surprise me to be admonished for such an abuse of setValue:forKey:, but is there a better way?  I imagine this idiom must be extremely commonplace, but I could not find a clear directive.
    >
    > I looked to Apple's "DemoMonkey" sample for its undo practices, and it just implements the verbose boilerplate for each property.  Is that really the recommended solution?
  • On 07 May 2012, at 4:45 pm, Graham Cox wrote:

    > Another way to 'centralise' undo is to have an object listen for the KVO notifications of changes for properties it's interested in, and use the observation method to record to the undo manager. You'd still use setValue:forKey: at undo time, so it amounts to a very similar idea. I've done it that way and it works.

    Well, that would require my controller to -addObserver:... on each of the collected objects, which I had earlier decided to avoid for various reasons.  But that's a good technique for me to keep in mind, and is beginning to sound like possibly a better approach.

    > So your problem must be something else. Are you sure undoManager returns something?

    You're right, I deked myself out; I was trying to examine changes to a property that was changed via the setter of a different property, but setValue:forKey: was not being called for the former.  I wasn't looking closely enough at the call stack or the forKey: parameter when I saw the seemingly do-nothing undoManager line dutifully execute.

    But, that's sort of moot because as you point out...:

    > Also, more obvious now I come to think about it, is that setValue:forKey: is not called for every property when it is set! That method can be used to call down to the setter for a named property, but if the setter is invoked directly (the more usual case), then setValue:forKey: isn't invoked. You might want to check whether that is what's happening. In which case making your undo handler a KVO observer will get around that problem, or you could override -willChangeValueForKey: instead, which is the method that causes some of the KVO "magic" to happen.

    Much better idea, thanks.  Moving my undo registration shim into -willChangeValueForKey: solves the problem nicely:

    - (void)willChangeValueForKey:(NSString *)key
    {
    // Register the inverse action if we are about to change an undoable property.
    if ([[FAEditorNote undoableKeys] containsObject:key])
      [[undoManager prepareWithInvocationTarget:self] setValue:[self valueForKey:key] forKey:key];

    [super willChangeValueForKey:key];

    return;
    }

    Which allows me to turf this gnarly macro I briefly purpose-built in the interim:

    #define DEFINE_UNDOABLE_COPYING_SETTER(SETTER,PROPERTY) \
    - (void)SETTER(id)newValue \
    { \
    if (newValue != PROPERTY) \
      { \
      [undoManager registerUndoWithTarget:self selector:@selector(SETTER) object:PROPERTY]; \
      [PROPERTY release]; \
      PROPERTY = [newValue copy]; \
      } \
    }

    DEFINE_UNDOABLE_COPYING_SETTER(setPgmInTC:, pgmInTC);
    DEFINE_UNDOABLE_COPYING_SETTER(setPgmOutTC:, pgmOutTC);
    //etc.

    thanks!

    b

    --
    Ben Kennedy, chief magician
    Zygoat Creative Technical Services
    http://www.zygoat.ca
  • On 08/05/2012, at 1:17 PM, Ben Kennedy wrote:

    > Well, that would require my controller to -addObserver:... on each of the collected objects, which I had earlier decided to avoid for various reasons.  But that's a good technique for me to keep in mind, and is beginning to sound like possibly a better approach.

    Indeed, but if you are willing to have your object "publish" a list of keys it wants to be undoable (as you already are), then you can iterate over them and do -addObserver: in a loop, so it might not be all that bad.

    However, I think that the override to -willChangeValueForkey: is just as good.

    --Graham
previous month may 2012 next month
MTWTFSS
  1 2 3 4 5 6
7 8 9 10 11 12 13
14 15 16 17 18 19 20
21 22 23 24 25 26 27
28 29 30 31      
Go to today