[RFA/base] NSUndoManager cleanup

[David Ayers]

Hello,

This patch cleans up the NSUndoManager and is the first step to get this 
bug fixed:

http://savannah.gnu.org/bugs/?func=detailbug&bug_id=3797&group_id=99

The main changes are
- implicitly beginUndoGrouping when groupsByEvent
- implicitly register the _loop: invocation when groupsByEvent
- actionNames are associated with the undo groups instead of the undo 
manager
- remove special case handling in undoNestedGroup

I've tested this against WO 4.5 which has identical documentation.  The 
documentation of undoNestedGroup concerning the raising of the 
NSInternalInconsistencyException is rather misleading (to say the 
least).  But the semantics are of the patch reflect the behavior of WO 
4.5 and also logically fit into the undo concept.

OK to commit?

   * Headers/gnustep/base/NSUndoManager.h: Add
   NSUndoCloseGroupingRunLoopOrdering enum and
   _runLoopGroupingPending instance variable.  Remove _actionName and
   _registeredUndo instance variable.
   * Source/NSUndoManager.m: Added documentation.
   Added actionName instance variable to PrivateUndoGroup.
   (-[PrivateUndoGroup actionName]): New method.
   (-[PrivateUndoGroup setActionName]): Ditto.
   (-[PrivateUndoGroup dealloc]): Release new instance variable.
   (-[PrivateUndoGroup initWithParent:]): Initialize actionName.
   (-[NSUndoManager forwardInvocation]): Implicilty call
   beginUndoGrouping if group has not been setup and we are grouping
   by events.  Register _loop: invocation if none is already pending
   and if we are grouping by event.
   (-[NSUndoManager registerUndoWithTarget:selector:object]): Ditto.
   (-[NSUndoManager _loop:]): Set flag to determine pending _loop:
   processing.
   (-[NSUndoManager init]): Removed access to _actionName.
   (-[NSUndoManager dealloc]): Ditto.
   (-[NSUndoManager enableUndoRegistration]): Remove access to
   _registeredUndo.
   (-[NSUndoManager redo]): Simplified implementation.
   (-[NSUndoManager redoActionName:]: Retrieve action name from first
   grouping of the redo stack.
   (-[NSUndoManager redoMenuTitleForUndoActionName:]): Add comment
   about localization.
   (-[NSUndoManager undoMenuTitleForUndoActionName:]): Ditto.
   (-[NSUndoManager setActionName:]): Forward call to current
   grouping.
   (-[NSUndoManager undoActionName:]: Retrieve action name from first
   grouping of the undo stack.
   (-[NSUndoManager setRunLoopModes:]): Use correct run loop ordering
   and set flag for pending _loop: invocation.
   (-[NSUndoManager undoNestedGroup]): Removed special case handling
   of _registeredUndo.  Raise NSInternalInconsistencyException if
   grouping is still in progress.  Simplified implementation.


Index: Headers/gnustep/base/NSUndoManager.h
===================================================================
RCS file: 
/cvsroot/gnustep/gnustep/core/base/Headers/gnustep/base/NSUndoManager.h,v
retrieving revision 1.5
diff -u -r1.5 NSUndoManager.h
--- Headers/gnustep/base/NSUndoManager.h        17 Dec 2001 14:31:42 -0000      
1.5
+++ Headers/gnustep/base/NSUndoManager.h        14 Jul 2003 08:42:18 -0000
@@ -30,6 +30,11 @@
 @class NSMutableArray;
 @class NSInvocation;
 
+/** Defines run loop ordering for closing undo groupings. */
+enum {
+  NSUndoCloseGroupingRunLoopOrdering = 350000
+};
+
 /* Public notification */
 GS_EXPORT NSString *NSUndoManagerCheckpointNotification;
 GS_EXPORT NSString *NSUndoManagerDidOpenUndoGroupNotification;
@@ -44,14 +49,13 @@
 @private
     NSMutableArray     *_redoStack;
     NSMutableArray     *_undoStack;
-    NSString           *_actionName;
     id                 _group;
     id                 _nextTarget;
     NSArray            *_modes;
     BOOL               _isRedoing;
     BOOL               _isUndoing;
     BOOL               _groupsByEvent;
-    BOOL               _registeredUndo;
+    BOOL               _runLoopGroupingPending;
     unsigned           _disableCount;
     unsigned           _levelsOfUndo;
 }
@@ -73,7 +77,7 @@
 - (void) redo;
 - (NSString*) redoActionName;
 - (NSString*) redoMenuItemTitle;
-- (NSString*) redoMenuTitleForUndoActionName: (NSString*)name;
+- (NSString*) redoMenuTitleForUndoActionName: (NSString*)actionName;
 - (void) registerUndoWithTarget: (id)target
                       selector: (SEL)aSelector
                         object: (id)anObject;
@@ -87,7 +91,7 @@
 - (void) undo;
 - (NSString*) undoActionName;
 - (NSString*) undoMenuItemTitle;
-- (NSString*) undoMenuTitleForUndoActionName: (NSString*)name;
+- (NSString*) undoMenuTitleForUndoActionName: (NSString*)actionName;
 - (void) undoNestedGroup;
 
 @end
Index: Source/NSUndoManager.m
===================================================================
RCS file: /cvsroot/gnustep/gnustep/core/base/Source/NSUndoManager.m,v
retrieving revision 1.10
diff -u -r1.10 NSUndoManager.m
--- Source/NSUndoManager.m      7 Jun 2003 01:24:41 -0000       1.10
+++ Source/NSUndoManager.m      14 Jul 2003 08:42:18 -0000
@@ -55,16 +55,20 @@
  */
 @interface     PrivateUndoGroup : NSObject
 {
-    PrivateUndoGroup   *parent;
-    NSMutableArray     *actions;
+  PrivateUndoGroup     *parent;
+  NSMutableArray       *actions;
+  NSString              *actionName;
 }
 - (NSMutableArray*) actions;
+
+- (NSString*) actionName;
 - (void) addInvocation: (NSInvocation*)inv;
 - (id) initWithParent: (PrivateUndoGroup*)parent;
 - (void) orphan;
 - (PrivateUndoGroup*) parent;
 - (void) perform;
 - (BOOL) removeActionsForTarget: (id)target;
+- (void) setActionName: (NSString*)name;
 @end
 
 @implementation        PrivateUndoGroup
@@ -74,6 +78,11 @@
   return actions;
 }
 
+- (NSString*) actionName
+{
+  return actionName;
+}
+
 - (void) addInvocation: (NSInvocation*)inv
 {
   if (actions == nil)
@@ -87,6 +96,7 @@
 {
   RELEASE(actions);
   RELEASE(parent);
+  RELEASE(actionName);
   [super dealloc];
 }
 
@@ -97,6 +107,7 @@
     {
       parent = RETAIN(p);
       actions = nil;
+      actionName = @"";
     }
   return self;
 }
@@ -147,6 +158,11 @@
   return NO;
 }
 
+- (void) setActionName: (NSString *)name
+{
+  ASSIGNCOPY(actionName,name);
+}
+
 @end
 
 
@@ -169,6 +185,7 @@
        }
       [self beginUndoGrouping];
     }
+  _runLoopGroupingPending = NO;
 }
 @end
 
@@ -179,6 +196,13 @@
  */
 @implementation NSUndoManager
 
+/**
+ * Starts a new grouping of undo actions which can be 
+ * atomically undone by an [-undo] invovation.
+ * This method posts an NSUndoManagerCheckpointNotification
+ * unless an undo is currently in progress.  It posts an
+ * NSUndoManagerDidOpenUndoGroupNotification upon creating the grouping.
+ */
 - (void) beginUndoGrouping
 {
   PrivateUndoGroup     *parent;
@@ -207,6 +231,10 @@
     }
 }
 
+/**
+ * Returns whether the receiver can service redo requests and
+ * posts a NSUndoManagerCheckpointNotification.
+ */
 - (BOOL) canRedo
 {
   [[NSNotificationCenter defaultCenter]
@@ -222,6 +250,13 @@
     }
 }
 
+/**
+ * Returns whether the receiver has any action groupings
+ * on the stack to undo.  It does not imply, that the
+ * receiver is currently in a state to service an undo
+ * request.  Make sure [-endEndGrouping] is invoked before
+ * requesting either an [-undo] or an [-undoNestedGroup].
+ */
 - (BOOL) canUndo
 {
   if ([_undoStack count] > 0)
@@ -242,23 +277,37 @@
                                           argument: nil];
   RELEASE(_redoStack);
   RELEASE(_undoStack);
-  RELEASE(_actionName);
   RELEASE(_group);
   RELEASE(_modes);
   [super dealloc];
 }
 
+/**
+ * Disables the registration of operations with with either
+ * [-registerUndoWithTarget:selector:object:] or
+ * [-forwardInvocation:].  This method may be called multiple
+ * times.  Each will need to be paired to a call of 
+ * [-enableUndoRegistration] before registration is actually
+ * reenabled.
+ */
 - (void) disableUndoRegistration
 {
   _disableCount++;
 }
 
+/**
+ * Matches previous calls of to [-disableUndoRegistration].  
+ * Only call this method to that end.  Once all are matched, 
+ * the registration of [-registerUndoWithTarget:selector:object:]
+ * and [-forwardInvocation:] are reenabled.  If this method is
+ * called without a matching -disableUndoRegistration,
+ * it will raise an NSInternalInconsistencyException.
+ */
 - (void) enableUndoRegistration
 {
   if (_disableCount > 0)
     {
       _disableCount--;
-      _registeredUndo = NO;    /* No operations since registration enabled. */
     }
   else
     {
@@ -267,6 +316,14 @@
     }
 }
 
+/**
+ * Matches previous calls of to [-beginUndoGrouping] and 
+ * puts the group on the undo stack.  This method posts
+ * an NSUndoManagerCheckpointNotification and
+ * a NSUndoManagerWillCloseUndoGroupNotification.  
+ * If there was no matching call to -beginUndoGrouping,
+ * this method will raise an NSInternalInconsistencyException.
+ */
 - (void) endUndoGrouping
 {
   PrivateUndoGroup     *g;
@@ -319,6 +376,32 @@
   RELEASE(g);
 }
 
+/**
+ * Registers the invocation with the current undo grouping.
+ * This method is part of the NSInvocation-based undo registration
+ * as opposed to the simpler [-registerUndoWithTarget:selector:object:]
+ * technique. <br/>
+ * You generally never invoke this method directly.  
+ * Instead invoke [-prepareWithInvocationTarget:] with the target of the
+ * undo action and then invoke the targets method to undo the action
+ * on the return value of -prepareWithInvocationTarget:
+ * which actually is the undo manager.
+ * The runtime will then fallback to -forwardInvocation: to do the actual
+ * registration of the invocation.
+ * The invocation will added to the current grouping.<br/>
+ * If the registrations have been disabled through [-disableUndoRegistration],
+ * this method does nothing.<br/>
+ * Unless the reciever implicitly 
+ * groups operations by event, the this method must have been preceeded
+ * with a [-beginUndoGrouping] message.  Otherwise it will raise an
+ * NSInternalInconsistencyException. <br/>
+ * Unless this method is invoked as part of a [-undo] or [-undoNestedGroup]
+ * processing, the redo stack is cleared.<br/>
+ * If the reciever [-groupsByEvents] and this is the first call to this
+ * method since the last run loop processing, this method sets up
+ * the reciever to process the [-endUndoGrouping] at the
+ * end of the event loop.
+ */
 - (void) forwardInvocation: (NSInvocation*)anInvocation
 {
   if (_disableCount == 0)
@@ -330,8 +413,15 @@
        }
       if (_group == nil)
        {
-         [NSException raise: NSInternalInconsistencyException
-                     format: @"forwardInvocation without beginUndoGrouping"];
+         if ([self groupsByEvent])
+           {
+             [self beginUndoGrouping];
+           }
+         else
+           {
+             [NSException raise: NSInternalInconsistencyException
+                         format: @"forwardInvocation without 
beginUndoGrouping"];
+           }
        }
       [anInvocation setTarget: _nextTarget];
       _nextTarget = nil;
@@ -340,10 +430,23 @@
        {
          [_redoStack removeAllObjects];
        }
-      _registeredUndo = YES;
+      if ((_runLoopGroupingPending == NO) && ([self groupsByEvent] == YES))
+       {
+         [[NSRunLoop currentRunLoop] performSelector: @selector(_loop:)
+                                     target: self
+                                     argument: nil
+                                     order: NSUndoCloseGroupingRunLoopOrdering
+                                     modes: _modes];
+         _runLoopGroupingPending = YES;
+       }
     }
 }
 
+/**
+ * Returns the current number of groupings.  These are the current
+ * groupings which can be nested, not the number of of groups on either
+ * the undo or redo stack.
+ */
 - (int) groupingLevel
 {
   PrivateUndoGroup     *g = (PrivateUndoGroup*)_group;
@@ -357,6 +460,14 @@
   return level;
 }
 
+/**
+ * Returns whether the receiver currently groups undo
+ * operations by events.  When it does, so it implicitly 
+ * invokes [-beginUndoGrouping] upon registration of undo
+ * operations and registers an internal call to insure
+ * the invocation of [-endUndoGrouping] at the end of the 
+ * run loop.
+ */
 - (BOOL) groupsByEvent
 {
   return _groupsByEvent;
@@ -367,7 +478,6 @@
   self = [super init];
   if (self)
     {
-      _actionName = @"";
       _redoStack = [[NSMutableArray alloc] initWithCapacity: 16];
       _undoStack = [[NSMutableArray alloc] initWithCapacity: 16];
       [self setRunLoopModes:
@@ -376,16 +486,25 @@
   return self;
 }
 
+/**
+ * Returns whether the receiver is currently processing a redo.
+ */
 - (BOOL) isRedoing
 {
   return _isRedoing;
 }
 
+/**
+ * Returns whether the receiver is currently processing an undo.
+ */
 - (BOOL) isUndoing
 {
   return _isUndoing;
 }
 
+/**
+ * Returns whether the receiver will currently register undo operations.
+ */
 - (BOOL) isUndoRegistrationEnabled
 {
   if (_disableCount == 0)
@@ -398,17 +517,44 @@
     }
 }
 
+/**
+ * Returns the maximium number of undo groupings the reciever will maintain.
+ * The default value is 0 meaning the number is only limited by
+ * memory availability.
+ */
 - (unsigned int) levelsOfUndo
 {
   return _levelsOfUndo;
 }
 
+/**
+ * Prepares the receiver to registers an invocation-based undo operation.
+ * This method is part of the NSInvocation-based undo registration
+ * as opposed to the simpler [-registerUndoWithTarget:selector:object:]
+ * technique. <br/>
+ * You invoke this method with the target of the
+ * undo action and then invoke the targets method to undo the action
+ * on the return value of this invocation
+ * which actually is the undo manager.
+ * The runtime will then fallback to [-forwardInvocation:] to do the actual
+ * registration of the invocation.
+ */
 - (id) prepareWithInvocationTarget: (id)target
 {
   _nextTarget = target;
   return self;
 }
 
+/**
+ * Performs a redo of previous undo request by taking the top grouping
+ * from the redo stack and invoking them.  This method posts an 
+ * NSUndoManagerCheckpointNotification notification to allow the client
+ * to porcess any pending changes before proceding.  If there are groupings
+ * on the redo stack, the top object is poped of the stack and invoked
+ * within a nested [-beginUndoGrouping]/[-endUndoGrouping].  During this
+ * pocessing, the operations registered for undo are recorded on the undo
+ * stack again.<br\>
+ */
 - (void) redo
 {
   if (_isUndoing || _isRedoing)
@@ -427,54 +573,103 @@
       [[NSNotificationCenter defaultCenter]
          postNotificationName: NSUndoManagerWillRedoChangeNotification
                    object: self];
-      groupToRedo = [_redoStack objectAtIndex: [_redoStack count] - 1];
-      IF_NO_GC([groupToRedo retain]);
-      [_redoStack removeObjectAtIndex: [_redoStack count] - 1];
+
+      groupToRedo = RETAIN([_redoStack lastObject]);
+      [_redoStack removeLastObject];
+
       oldGroup = _group;
       _group = nil;
       _isRedoing = YES;
+
       [self beginUndoGrouping];
       [groupToRedo perform];
       RELEASE(groupToRedo);
       [self endUndoGrouping];
+
       _isRedoing = NO;
       _group = oldGroup;
+
       [[NSNotificationCenter defaultCenter]
          postNotificationName: NSUndoManagerDidRedoChangeNotification
                        object: self];
     }
 }
 
+/**
+ * If the receiver can preform a redo, this method returns
+ * the action name previously associated with the top grouping with
+ * [-setActionName:].  This name should identify the action to be redone.
+ * If there are no items on the redo stack this method returns nil.
+ * If no action name hs been set, this method returns an empty string.
+ */
 - (NSString*) redoActionName
 {
   if ([self canRedo] == NO)
     {
       return nil;
     }
-  return _actionName;
+  return [[_redoStack lastObject] actionName];
 }
 
+/**
+ * Returns the full localized title of the actions to be displayed
+ * as a menu item.  This method first invokes [-redoActionName] and 
+ * passes it to [-redoMenuTitelForUndoActionName:] and returns the result.
+ */
 - (NSString*) redoMenuItemTitle
 {
   return [self redoMenuTitleForUndoActionName: [self redoActionName]];
 }
 
-- (NSString*) redoMenuTitleForUndoActionName: (NSString*)name
+/**
+ * Returns the localized title of the actions to be displayed
+ * as a menu item identified by actionName, by appending a
+ * localized command string like @"Redo <localized(actionName)>".
+ */
+- (NSString*) redoMenuTitleForUndoActionName: (NSString*)actionName
 {
-  if (name)
+  /* 
+   * FIXME: The terms @"Redo" and @"Redo %@" should be localized.
+   * Possibly with the introduction of GSBaseLocalizedString() private
+   * the the library.
+   */
+  if (actionName)
     {
-      if ([name isEqual: @""])
+      if ([actionName isEqual: @""])
        {
          return @"Redo";
        }
       else
        {
-         return [NSString stringWithFormat: @"Redo %@", name];
+         return [NSString stringWithFormat: @"Redo %@", actionName];
        }
     }
-  return name;
+  return actionName;
 }
 
+/**
+ * Registers an undo operation.
+ * This method is the simple target-action-based undo registration
+ * as opposed to the sophisticated [-forwardInvocation:]
+ * mechanism. <br/>
+ * You invoke this method with the target of the
+ * undo action providing the selector which can perform the undo with
+ * the provided object.  The object is often a dictionary of the
+ * identifying the attribute and thier values before the change.
+ * The invocation will added to the current grouping.<br/>
+ * If the registrations have been disabled through [-disableUndoRegistration],
+ * this method does nothing.<br/>
+ * Unless the reciever implicitly 
+ * groups operations by event, the this method must have been preceeded
+ * with a [-beginUndoGrouping] message.  Otherwise it will raise an
+ * NSInternalInconsistencyException. <br/>
+ * Unless this method is invoked as part of a [-undo] or [-undoNestedGroup]
+ * processing, the redo stack is cleared.<br/>
+ * If the reciever [-groupsByEvents] and this is the first call to this
+ * method since the last run loop processing, this method sets up
+ * the reciever to process the [-endUndoGrouping] at the
+ * end of the event loop.
+ */
 - (void) registerUndoWithTarget: (id)target
                       selector: (SEL)aSelector
                         object: (id)anObject
@@ -487,8 +682,15 @@
 
       if (_group == nil)
        {
-         [NSException raise: NSInternalInconsistencyException
-                     format: @"registerUndo without beginUndoGrouping"];
+         if ([self groupsByEvent])
+           {
+             [self beginUndoGrouping];
+           }
+         else
+           {
+             [NSException raise: NSInternalInconsistencyException
+                          format: @"registerUndo without beginUndoGrouping"];
+           }
        }
       g = _group;
       sig = [target methodSignatureForSelector: aSelector];
@@ -501,10 +703,26 @@
        {
          [_redoStack removeAllObjects];
        }
-      _registeredUndo = YES;
+      if ((_runLoopGroupingPending == NO) && ([self groupsByEvent] == YES))
+       {
+         [[NSRunLoop currentRunLoop] performSelector: @selector(_loop:)
+                                     target: self
+                                     argument: nil
+                                     order: NSUndoCloseGroupingRunLoopOrdering
+                                     modes: _modes];
+         _runLoopGroupingPending = YES;
+       }
     }
 }
 
+/**
+ * Removes all grouping stored in the receiver.  This clears the both
+ * the undo and the redo stacks.  This method is if the sole client
+ * of the undo manager will be unable to service any undo or redo events.
+ * The client can call this method in its -dealloc method, unless the
+ * undo manager has several clients, in which case 
+ * [-removeAllActionsWithTarget:] is more apropriate.
+ */
 - (void) removeAllActions
 {
   [_redoStack removeAllObjects];
@@ -514,6 +732,13 @@
   _disableCount = 0;
 }
 
+/**
+ * Removes all actions recorded for the given target.  This method is
+ * is useful when a client of the undo manager will be unable to
+ * service any undo or redo events.  Clients should call this method
+ * in thier dealloc method, unless they are the sole client of the
+ * undo manager in which case [-removeAllActions] is more apropriate.
+ */
 - (void) removeAllActionsWithTarget: (id)target
 {
   unsigned     i;
@@ -542,19 +767,36 @@
     }
 }
 
+/**
+ * Returns the run loop modes in which the receiver registers 
+ * the [-endUndoGroup] processing when it [-groupsByEvent].
+ */
 - (NSArray*) runLoopModes
 {
   return _modes;
 }
 
+/**
+ * Sets the name associated with the actions of the current group.
+ * Typically you can call this method while registering the actions
+ * for the current group.  This name will be used to determine the
+ * name in the [-undoMenuTitleForUndoActionName:] and 
+ * [-redoMenuTitleForUndoActionName:] names typically displayed
+ * in the menu.
+ */
 - (void) setActionName: (NSString*)name
 {
-  if (name != nil && _actionName != name)
+  if ((name != nil) && (_group != nil))
     {
-      ASSIGNCOPY(_actionName, name);
+      [_group setActionName: name];
     }
 }
 
+/**
+ * Sets whether the receiver should implicitly call [-beginUndoGrouping] when
+ * necessary and register a call to invoke [-endUndoGrouping] at the end
+ * of the current event loop.  The grouping is tunred on by default.
+ */
 - (void) setGroupsByEvent: (BOOL)flag
 {
   if (_groupsByEvent != flag)
@@ -563,6 +805,13 @@
     }
 }
 
+/**
+ * Sets the maximum number of groups in either the undo or redo stack.
+ * Use this method to limit memory usage if you either expect very many
+ * actions to be recorded or the recorded objects require a lot of memory.
+ * When set to 0 the stack size is limited by the range of a unsigned int,
+ * available memory.
+ */
 - (void) setLevelsOfUndo: (unsigned)num
 {
   _levelsOfUndo = num;
@@ -579,6 +828,13 @@
     }
 }
 
+/**
+ * Sets the modes in which the reciever registers the calls
+ * with the current run loop to invoke
+ * [-endUndoGroup] when it [-groupsByEvents].  This method
+ * first cancels any pending registrations in the old modes and
+ * registers the invokation in the new modes.
+ */
 - (void) setRunLoopModes: (NSArray*)newModes
 {
   if (_modes != newModes)
@@ -590,11 +846,18 @@
       [[NSRunLoop currentRunLoop] performSelector: @selector(_loop:)
                                           target: self
                                         argument: nil
-                                           order: 0
+                                           order: 
NSUndoCloseGroupingRunLoopOrdering
                                            modes: _modes];
+      _runLoopGroupingPending = YES;
     }
 }
 
+/**
+ * This method performs an undo by invoking [-undoNestedGroup].
+ * If current group of the reciever is the top group this method first
+ * calls [-endUndoGrouping].  This method may only be called on the top
+ * level group, otherwise it will raise an NSInternalInconsistencyException.
+ */
 - (void) undo
 {
   if ([self groupingLevel] == 1)
@@ -609,36 +872,68 @@
   [self undoNestedGroup];
 }
 
+/**
+ * If the receiver can preform an undo, this method returns
+ * the action name previously associated with the top grouping with
+ * [-setActionName:].  This name should identify the action to be undone.
+ * If there are no items on the undo stack this method returns nil.
+ * If no action name hs been set, this method returns an empty string.
+ */
 - (NSString*) undoActionName
 {
   if ([self canUndo] == NO)
     {
       return nil;
     }
-  return _actionName;
+  return [[_undoStack lastObject] actionName];
 }
 
+/**
+ * Returns the full localized title of the actions to be displayed
+ * as a menu item.  This method first invokes [-undoActionName] and 
+ * passes it to [-undoMenuTitelForUndoActionName:] and returns the result.
+ */
 - (NSString*) undoMenuItemTitle
 {
   return [self undoMenuTitleForUndoActionName: [self undoActionName]];
 }
 
-- (NSString*) undoMenuTitleForUndoActionName: (NSString*)name
+/**
+ * Returns the localized title of the actions to be displayed
+ * as a menu item identified by actionName, by appending a
+ * localized command string like @"Undo <localized(actionName)>".
+ */
+- (NSString*) undoMenuTitleForUndoActionName: (NSString*)actionName
 {
-  if (name)
+  /* 
+   * FIXME: The terms @"Undo" and @"Undo %@" should be localized.
+   * Possibly with the introduction of GSBaseLocalizedString() private
+   * the the library.
+   */
+  if (actionName)
     {
-      if ([name isEqual: @""])
+      if ([actionName isEqual: @""])
        {
          return @"Undo";
        }
       else
        {
-         return [NSString stringWithFormat: @"Undo %@", name];
+         return [NSString stringWithFormat: @"Undo %@", actionName];
        }
     }
-  return name;
+  return actionName;
 }
 
+/**
+ * Performs an undo by taking the top grouping
+ * from the undo stack and invoking them.  This method posts an 
+ * NSUndoManagerCheckpointNotification notification to allow the client
+ * to porcess any pending changes before proceding.  If there are groupings
+ * on the undo stack, the top object is poped of the stack and invoked
+ * within a nested beginUndoGrouping/endUndoGrouping.  During this
+ * pocessing, the undo operations registered for undo are recorded on the redo
+ * stack.<br\>
+ */
 - (void) undoNestedGroup
 {
   PrivateUndoGroup     *oldGroup;
@@ -647,34 +942,32 @@
   [[NSNotificationCenter defaultCenter]
       postNotificationName: NSUndoManagerCheckpointNotification
                    object: self];
-#if 0
-/*
- *     The documentation says we should raise an exception - but I can't
- *     make sense of it - raising an exception seems to break everything.
- *     It would make more sense to raise an exception if NO undo operations
- *     had been registered.
- */
-  if (_registeredUndo)
+
+  if (_group != nil)
     {
       [NSException raise: NSInternalInconsistencyException
-                 format: @"undoNestedGroup with registered undo ops"];
+                 format: @"undoNestedGroup before endUndoGrouping"];
     }
-#endif
+
   if (_isUndoing || _isRedoing)
     {
       [NSException raise: NSInternalInconsistencyException
                  format: @"undoNestedGroup while undoing or redoing"];
-      }
-  if (_group != nil && [_undoStack count] == 0)
+    }
+
+  if ([_undoStack count] == 0)
     {
       return;
     }
+
   [[NSNotificationCenter defaultCenter]
       postNotificationName: NSUndoManagerWillUndoChangeNotification
                    object: self];
+
   oldGroup = _group;
   _group = nil;
   _isUndoing = YES;
+
   if (oldGroup)
     {
       groupToUndo = oldGroup;
@@ -684,16 +977,18 @@
     }
   else
     {
-      groupToUndo = [_undoStack objectAtIndex: [_undoStack count] - 1];
-      IF_NO_GC([groupToUndo retain]);
-      [_undoStack removeObjectAtIndex: [_undoStack count] - 1];
+      groupToUndo = RETAIN([_undoStack lastObject]);
+      [_undoStack removeLastObject];
     }
+
   [self beginUndoGrouping];
   [groupToUndo perform];
   RELEASE(groupToUndo);
   [self endUndoGrouping];
+
   _isUndoing = NO;
   _group = oldGroup;
+
   [[NSNotificationCenter defaultCenter]
       postNotificationName: NSUndoManagerDidUndoChangeNotification
                    object: self];