NSUndoManager setActionName: oddity

  • Greetings,

    I am using bindings in a (document-based) project in probably the simplest way one can imagine:

    -My model contains an NSMutableArray
    -An NSArrayController has its content bound to said NSMutableArray and is configured to prepare content (which takes the form of another custom model object)
    -Said NSMutableArray is mutated only through the safe NSArrayController methods (-add:, -remove:, etc.)

    Everything works just fine, including undo and redo, which I manage by adding appropriate -prepareWithInvocationTarget: calls in the model's -insertObject:in[PropertyName]AtIndex: and -removeObjectFrom[PropertyName]AtIndex: methods.

    Now, I want to give the undo actions nice names.  To maintain a good MVC pattern I created wrapper action methods in my controller class (not the NSArrayController, but a custom NSViewController subclass that manages a variety of behaviors) such as:

    - (void)addRecord:(id)sender
    {
        if (! [[self undoManager] isUndoing]) {
            [[self undoManager] setActionName:NSLocalizedString(@"Add Record", nil)];
        }
        [[self arrayController] add:sender];
    }

    - (void)removeRecord:(id)sender
    {
        if (! [[self undoManager] isUndoing]) {
            [[self undoManager] setActionName:NSLocalizedString(@"Remove Record", nil)];
        }
        [[self arrayController] remove:sender];
    }

    and have my buttons, menu items, etc., call these wrapper methods.

    Here's the strange part: the action name appears to only have an effect in the *removal* case, never in the *addition* case.

    To be clear: everything else works fine.  If I perform an action triggering -addRecord:, the record *is* added and a functional undo action *is* registered, just with no action name.  But this only happens in the addition case - in the removal case, everything works *including the action name*.

    (If I move the -setActionName: into the model layer (-inserObject:…) the action name also works.)

    For debugging, I have verified that the expected code is called in the controller layer, and that the undo manager is both non-nil and is the same undo manager instance as used in the model layer.  Indeed, I can even see that the action name has been purportedly set when stepping through the code:

    (lldb) po [[self undoManager] undoActionName]
    (id) $1 = 0x00000001045f1520 Add Record

    But this never appears in the UI.  I also set a global breakpoint on [NSUndoManager setActionName:] and can see that it's only getting called once, inside my -addRecord: method.  So it doesn't seem to be getting clobbered later on.

    I have even been able to reproduce this behavior in a minimal test project (mashing together the model and controller layers), which you can download from:

    https://dl.dropbox.com/u/5847625/BindingsUndoTest.zip

    So my questions are severalfold:

    1) Has anyone else seen this and can explain what's going on?  Have I missed something?

    2) Can anyone suggest a workaround, preferably short of moving -setActionName: into the model?

    3) Is there perhaps a more appropriate way for me to handle undo/redo in this context?

    This is on OS X, 10.7.4, FYI.  I am targeting Lion.

    Thanks,
    Conrad Shultz
  • On Jul 2, 2012, at 4:35 AM, Conrad Shultz wrote:

    > -My model contains an NSMutableArray
    > -An NSArrayController has its content bound to said NSMutableArray and is configured to prepare content (which takes the form of another custom model object)
    > -Said NSMutableArray is mutated only through the safe NSArrayController methods (-add:, -remove:, etc.)
    >
    > Everything works just fine, including undo and redo, which I manage by adding appropriate -prepareWithInvocationTarget: calls in the model's -insertObject:in[PropertyName]AtIndex: and -removeObjectFrom[PropertyName]AtIndex: methods.
    >
    > Now, I want to give the undo actions nice names.  To maintain a good MVC pattern I created wrapper action methods in my controller class (not the NSArrayController, but a custom NSViewController subclass that manages a variety of behaviors) such as:
    >
    > - (void)addRecord:(id)sender
    > {
    > if (! [[self undoManager] isUndoing]) {
    > [[self undoManager] setActionName:NSLocalizedString(@"Add Record", nil)];
    > }
    > [[self arrayController] add:sender];
    > }
    >
    > - (void)removeRecord:(id)sender
    > {
    > if (! [[self undoManager] isUndoing]) {
    > [[self undoManager] setActionName:NSLocalizedString(@"Remove Record", nil)];
    > }
    > [[self arrayController] remove:sender];
    > }
    >
    > and have my buttons, menu items, etc., call these wrapper methods.
    >
    > Here's the strange part: the action name appears to only have an effect in the *removal* case, never in the *addition* case.
    >
    > To be clear: everything else works fine.  If I perform an action triggering -addRecord:, the record *is* added and a functional undo action *is* registered, just with no action name.  But this only happens in the addition case - in the removal case, everything works *including the action name*.
    >
    > (If I move the -setActionName: into the model layer (-inserObject:…) the action name also works.)
    >
    > For debugging, I have verified that the expected code is called in the controller layer, and that the undo manager is both non-nil and is the same undo manager instance as used in the model layer.  Indeed, I can even see that the action name has been purportedly set when stepping through the code:
    >
    > (lldb) po [[self undoManager] undoActionName]
    > (id) $1 = 0x00000001045f1520 Add Record
    >
    > But this never appears in the UI.  I also set a global breakpoint on [NSUndoManager setActionName:] and can see that it's only getting called once, inside my -addRecord: method.  So it doesn't seem to be getting clobbered later on.
    >
    > I have even been able to reproduce this behavior in a minimal test project (mashing together the model and controller layers), which you can download from:
    >
    > https://dl.dropbox.com/u/5847625/BindingsUndoTest.zip
    >
    > So my questions are severalfold:
    >
    > 1) Has anyone else seen this and can explain what's going on?  Have I missed something?
    >
    > 2) Can anyone suggest a workaround, preferably short of moving -setActionName: into the model?
    >
    > 3) Is there perhaps a more appropriate way for me to handle undo/redo in this context?
    >
    > This is on OS X, 10.7.4, FYI.  I am targeting Lion.

    I believe you're not seeing the "Add Record" action name because you're setting the action name for the current undo group in one iteration of the runloop and the action itself (-[NSArrayController add:]) is taking place in another iteration of the runloop (with a different undo group).

    https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Und
    oArchitecture/Articles/UndoManager.html


    "NSUndoManager normally creates undo groups automatically during the run loop. The first time it is asked to record an undo operation in the run loop, it creates a new group. Then, at the end of the loop, it closes the group."

    https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Appl
    icationKit/Classes/NSArrayController_Class/Reference/Reference.html


    Under "Special Considerations" for add:

    "Beginning with Mac OS X v10.4 the result of this method is deferred until the next iteration of the runloop so that the error presentation mechanism (see Error Responders and Error Recovery) can provide feedback as a sheet."
  • On Jul 2, 2012, at 2:35 AM, Conrad Shultz <conrad...> wrote:

    > - (void)addRecord:(id)sender
    > {
    > if (! [[self undoManager] isUndoing]) {
    > [[self undoManager] setActionName:NSLocalizedString(@"Add Record", nil)];
    > }
    > [[self arrayController] add:sender];
    > }

    IIRC, you need to put the call to -setActionName: *after* the action that opens the undo group. Which I don't see you doing here, but I suppose you're relying on it happening as a side effect of whatever the array controller does?

    This is because NSUndoManager is a very old class with a very weird internal data structure that can't attach an action name without an object at the top of its stack. You might investigate using Graham Cox's GCUndoManager.

    FWIW, we put all our undo actions in the model because of AppleScript, but that makes our model single-threaded.

    --Kyle Sluder
  • Thanks Michael.

    I was aware of the run loop considerations for NSUndoManager but hadn't seen that note on the NSArrayController method. Thanks for pointing that out.

    What confuses me, though, is that, contrary to the identical note for -remove:, my approach DOES work in the removal case. Indeed, if the run loop deferral is the cause of my problem I would probably have figured it out sooner were it not for this mixed behavior.

    Thoughts on what might be occurring there?

    (Sent from my iPhone.)

    --
    Conrad Shultz

    On Jul 2, 2012, at 8:23, Michael Babin <mbabin...> wrote:

    > On Jul 2, 2012, at 4:35 AM, Conrad Shultz wrote:
    >
    >> -My model contains an NSMutableArray
    >> -An NSArrayController has its content bound to said NSMutableArray and is configured to prepare content (which takes the form of another custom model object)
    >> -Said NSMutableArray is mutated only through the safe NSArrayController methods (-add:, -remove:, etc.)
    >>
    >> Everything works just fine, including undo and redo, which I manage by adding appropriate -prepareWithInvocationTarget: calls in the model's -insertObject:in[PropertyName]AtIndex: and -removeObjectFrom[PropertyName]AtIndex: methods.
    >>
    >> Now, I want to give the undo actions nice names.  To maintain a good MVC pattern I created wrapper action methods in my controller class (not the NSArrayController, but a custom NSViewController subclass that manages a variety of behaviors) such as:
    >>
    >> - (void)addRecord:(id)sender
    >> {
    >> if (! [[self undoManager] isUndoing]) {
    >> [[self undoManager] setActionName:NSLocalizedString(@"Add Record", nil)];
    >> }
    >> [[self arrayController] add:sender];
    >> }
    >>
    >> - (void)removeRecord:(id)sender
    >> {
    >> if (! [[self undoManager] isUndoing]) {
    >> [[self undoManager] setActionName:NSLocalizedString(@"Remove Record", nil)];
    >> }
    >> [[self arrayController] remove:sender];
    >> }
    >>
    >> and have my buttons, menu items, etc., call these wrapper methods.
    >>
    >> Here's the strange part: the action name appears to only have an effect in the *removal* case, never in the *addition* case.
    >>
    >> To be clear: everything else works fine.  If I perform an action triggering -addRecord:, the record *is* added and a functional undo action *is* registered, just with no action name.  But this only happens in the addition case - in the removal case, everything works *including the action name*.
    >>
    >> (If I move the -setActionName: into the model layer (-inserObject:…) the action name also works.)
    >>
    >> For debugging, I have verified that the expected code is called in the controller layer, and that the undo manager is both non-nil and is the same undo manager instance as used in the model layer.  Indeed, I can even see that the action name has been purportedly set when stepping through the code:
    >>
    >> (lldb) po [[self undoManager] undoActionName]
    >> (id) $1 = 0x00000001045f1520 Add Record
    >>
    >> But this never appears in the UI.  I also set a global breakpoint on [NSUndoManager setActionName:] and can see that it's only getting called once, inside my -addRecord: method.  So it doesn't seem to be getting clobbered later on.
    >>
    >> I have even been able to reproduce this behavior in a minimal test project (mashing together the model and controller layers), which you can download from:
    >>
    >> https://dl.dropbox.com/u/5847625/BindingsUndoTest.zip
    >>
    >> So my questions are severalfold:
    >>
    >> 1) Has anyone else seen this and can explain what's going on?  Have I missed something?
    >>
    >> 2) Can anyone suggest a workaround, preferably short of moving -setActionName: into the model?
    >>
    >> 3) Is there perhaps a more appropriate way for me to handle undo/redo in this context?
    >>
    >> This is on OS X, 10.7.4, FYI.  I am targeting Lion.
    >
    > I believe you're not seeing the "Add Record" action name because you're setting the action name for the current undo group in one iteration of the runloop and the action itself (-[NSArrayController add:]) is taking place in another iteration of the runloop (with a different undo group).
    >
    > https://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/Und
    oArchitecture/Articles/UndoManager.html

    >
    > "NSUndoManager normally creates undo groups automatically during the run loop. The first time it is asked to record an undo operation in the run loop, it creates a new group. Then, at the end of the loop, it closes the group."
    >
    > https://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Appl
    icationKit/Classes/NSArrayController_Class/Reference/Reference.html

    >
    > Under "Special Considerations" for add:
    >
    > "Beginning with Mac OS X v10.4 the result of this method is deferred until the next iteration of the runloop so that the error presentation mechanism (see Error Responders and Error Recovery) can provide feedback as a sheet."
    >
    >
  • Thanks Kyle.

    As I mentioned the actual action opening the undo group is in the -insertObject:in: method in the model, which does work. My very first thought was an ordering issue so I tried inverting the order of method calling on the wrapper to no avail, but as Michael pointed out this might have been fruitless anyway.

    I'll take a look at GCUndoManager. I'd heard of it being used in the context of Core Data, but maybe I should consider it here too. (I hate to introduce third-party dependencies unless absolutely necessary.)

    I could set the action name in the model too, but that feels icky. (Do you set the action name or just register undo operations in the model? I don't see why action names would be needed for scripting, but then again I don't really know AppleScript.)

    (Sent from my iPhone.)

    --
    Conrad Shultz

    On Jul 2, 2012, at 8:25, Kyle Sluder <kyle...> wrote:

    > On Jul 2, 2012, at 2:35 AM, Conrad Shultz <conrad...> wrote:
    >
    >> - (void)addRecord:(id)sender
    >> {
    >> if (! [[self undoManager] isUndoing]) {
    >> [[self undoManager] setActionName:NSLocalizedString(@"Add Record", nil)];
    >> }
    >> [[self arrayController] add:sender];
    >> }
    >
    > IIRC, you need to put the call to -setActionName: *after* the action that opens the undo group. Which I don't see you doing here, but I suppose you're relying on it happening as a side effect of whatever the array controller does?
    >
    > This is because NSUndoManager is a very old class with a very weird internal data structure that can't attach an action name without an object at the top of its stack. You might investigate using Graham Cox's GCUndoManager.
    >
    > FWIW, we put all our undo actions in the model because of AppleScript, but that makes our model single-threaded.
    >
    > --Kyle Sluder
  • On Mon, Jul 2, 2012, at 10:26 AM, Conrad Shultz wrote:
    > Thanks Kyle.
    >
    > As I mentioned the actual action opening the undo group is in the
    > -insertObject:in: method in the model, which does work. My very first
    > thought was an ordering issue so I tried inverting the order of method
    > calling on the wrapper to no avail, but as Michael pointed out this might
    > have been fruitless anyway.
    >
    > I'll take a look at GCUndoManager. I'd heard of it being used in the
    > context of Core Data, but maybe I should consider it here too. (I hate to
    > introduce third-party dependencies unless absolutely necessary.)
    >
    > I could set the action name in the model too, but that feels icky. (Do
    > you set the action name or just register undo operations in the model? I
    > don't see why action names would be needed for scripting, but then again
    > I don't really know AppleScript.)

    Looking through the code, it appears that we set most of our action
    names in the controller using an extension method
    -setActionNameIfGrouped:
    <https://github.com/omnigroup/OmniGroup/blob/master/Frameworks/OmniFoundatio
    n/OpenStepExtensions.subproj/NSUndoManager-OFExtensions.m#L104
    >.
    I suppose the main reason is that we want the action names to match the
    user interface terminology: "Undo check" rather than "Undo set item
    value". But I know we've had internal debates about this before.

    But our actual action registration (-prepareWithInvocationTarget:)
    always happens in our model. This is because AppleScript talks directly
    to our model objects. (This is standard practice for implementing
    scriptability in Cocoa.) There are some interesting (historical?)
    interactions between AppleScript and NSUndoManager related to the
    runloop.

    --Kyle Sluder
  • On 03/07/2012, at 3:26 AM, Conrad Shultz wrote:

    > I'll take a look at GCUndoManager. I'd heard of it being used in the context of Core Data, but maybe I should consider it here too. (I hate to introduce third-party dependencies unless absolutely necessary.)

    It wasn't written with Core Data in mind, but some later fixes were made to ensure it did work with Core Data.

    > I could set the action name in the model too, but that feels icky. (Do you set the action name or just register undo operations in the model? I don't see why action names would be needed for scripting, but then again I don't really know AppleScript.)

    I think your original approach is close to optimal practice; I follow this pattern almost always:

    - (IBAction)        someAction:(id) sender
    {
    [self dotheActualActionWithUndo];

    [[self undoManager] setActionName:[sender title]];
    }

    There's no reason to check whether the undo manager is undoing - at this point it can't possibly be. All the interaction with the undo manager takes place in the internals of the controller or data model - the undo manager will never (or should never) invoke an action method. Undo is never invoked on a thread other than main.

    The undo manager is given the various tasks to do first, then the action name is set last. This takes care of ensuring that the undo manager has something it can attach the name to. I think this is the only reason you are having problems.

    The action name is taken from the title of the sender (e.g. button or menu item). Why do this? Because it's one less bit of text to localize and the action appearing in Undo is fully consistent with the text in the interface. There are sometimes exceptions to this approach that mean you have to explicitly set an action name, but they shouldn't be common.

    The action name should be set at the end of the highest level method that causes the undoable work to happen. Then, if the operation consists of several steps, each of which has some undoable component, the action name is attached to the entire group of operations. In the NSUndoManager, groups are marked internally using sentinel objects rather than an actual container of some sort, and the 'group open' marker is not added until there is a task ready to put in it. That means that if you try to set the action name first, there's nothing available to attach it to (NSUndoManager will attach it to the current group, but if there are no tasks, there's nothing there, and if there are, the group in question is the wrong one).

    --Graham
previous month july 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