undo after repeatedly calling 'repeat' leaves cursor in the wrong place

undo after repeatedly calling 'repeat' leaves cursor in the wrong place

[Chris Moore]

Type
  "x C-x z z z z C-x u"
(to insert an 'x', repeat the insert 4 times, then undo the last of the 4)

Before the "C-x u" there were 5 'x's with the cursor after them all.
After the "C-x u" there are 4 'x's, as expected, but the cursor is on
the 2nd one, whereas I would expect it to be after the 4th one.

If "C-x z" is used in full each time, instead of just hitting 'z' over
and over, then the undo works as expected.

Re: undo after repeatedly calling 'repeat' leaves cursor in the wrong place

[martin rudalics]

 > Type
 >   "x C-x z z z z C-x u"
 > (to insert an 'x', repeat the insert 4 times, then undo the last of the 4)
 >
 > Before the "C-x u" there were 5 'x's with the cursor after them all.
 > After the "C-x u" there are 4 'x's, as expected, but the cursor is on
 > the 2nd one, whereas I would expect it to be after the 4th one.
 >
 > If "C-x z" is used in full each time, instead of just hitting 'z' over
 > and over, then the undo works as expected.

`repeat' has the loop

         (while (eq (read-event) repeat-repeat-char)
	  ;; Make each repetition undo separately.
	  (undo-boundary)
           (repeat repeat-arg))

Note that `read-event' does not update `last_point_position' and
`undo-boundary' inserts nil into `buffer-undo-list' which prevents
combining the insertions.  `record_insert' calls `record_point' with the
positions where an "x" shall be inserted (2, 3, ...) but `record_point'
still relies on the old position (namely 2) stored for
`last_point_position', decides that it is not equal to point in

       if (last_point_position != pt)
	current_buffer->undo_list
	  = Fcons (make_number (last_point_position), current_buffer->undo_list);

and repeatedly inserts a 2 into `buffer-undo-list'.  Note that
replacing the latter by

       if (last_point_position != pt)
	{
	  current_buffer->undo_list
	    = Fcons (make_number (last_point_position), current_buffer->undo_list);
	  last_point_position = pt;
	}

still gets an off-by-one error.  I think two changes are needed:

(1) For a self-insert-command `repeat' should not construct undo
boundaries.  This would correct the bug reported by Chris but fail for
more complex commands.

(2) Use a separate variable, locally bound in `repeat' around the
recursive call to itself, to assert that `record_point' updates
`last_point_position' before comparing it with `pt'.  Maybe someone has
an idea how to avoid such a variable.  I'd rather not remove such items
from `buffer-undo-list' manually.

Re: undo after repeatedly calling 'repeat' leaves cursor in the wrong place

[martin rudalics]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

> Type
>   "x C-x z z z z C-x u"
> (to insert an 'x', repeat the insert 4 times, then undo the last of the 4)
> 
> Before the "C-x u" there were 5 'x's with the cursor after them all.
> After the "C-x u" there are 4 'x's, as expected, but the cursor is on
> the 2nd one, whereas I would expect it to be after the 4th one.
> 
> If "C-x z" is used in full each time, instead of just hitting 'z' over
> and over, then the undo works as expected.

Could you please try the attached patch - against EMACS_22_BASE - and
tell whether it does the right thing.

[-- Attachment #2: repeat-undo.patch --]
[-- Type: text/plain, Size: 5217 bytes --]

*** undo.c	Wed Jul 25 07:15:32 2007
--- undo.c	Thu Oct  4 10:04:52 2007
***************
*** 53,60 ****
     an undo-boundary.  */
  Lisp_Object pending_boundary;
  
  /* Record point as it was at beginning of this command (if necessary)
!    And prepare the undo info for recording a change.
     PT is the position of point that will naturally occur as a result of the
     undo record that will be added just after this command terminates.  */
  
--- 53,64 ----
     an undo-boundary.  */
  Lisp_Object pending_boundary;
  
+ /* Nonzero means do not record point in record_point.  */
+ 
+ int undo_inhibit_record_point;
+ 
  /* Record point as it was at beginning of this command (if necessary)
!    and prepare the undo info for recording a change.
     PT is the position of point that will naturally occur as a result of the
     undo record that will be added just after this command terminates.  */
  
***************
*** 64,69 ****
--- 68,81 ----
  {
    int at_boundary;
  
+   /* Don't record position of pt when undo_inhibit_record_point holds.
+      This should avoid inserting a position record in buffer-undo-list
+      when last_point_position has not been set up correctly by
+      command_loop_1, for example, when running a repeat-repeat-char
+      event.  */
+   if (undo_inhibit_record_point)
+     return;
+ 	    
    /* Allocate a cons cell to be the undo boundary after this command.  */
    if (NILP (pending_boundary))
      pending_boundary = Fcons (Qnil, Qnil);
***************
*** 719,724 ****
--- 731,740 ----
  Garbage collection is inhibited around the call to this function,
  so it must make sure not to do a lot of consing.  */);
    Vundo_outer_limit_function = Qnil;
+ 
+   DEFVAR_BOOL ("undo-inhibit-record-point", &undo_inhibit_record_point,
+ 	       doc: /* Non-nil means do not record `point' in `buffer-undo-list'.  */);
+   undo_inhibit_record_point = 0;
  }
  
  /* arch-tag: d546ee01-4aed-4ffb-bb8b-eefaae50d38a

*** repeat.el	Wed Aug 29 10:36:26 2007
--- repeat.el	Thu Oct  4 10:13:58 2007
***************
*** 200,205 ****
--- 200,213 ----
  (defvar repeat-previous-repeated-command nil
    "The previous repeated command.")
  
+ ;; The following variable counts repeated self-insertions.  The idea is
+ ;; that repeating a self-insertion command and subsequently undoing it
+ ;; should have nearly the same effect as if the characters were inserted
+ ;; manually.  The basic difference is that we leave in one undo-boundary
+ ;; between the original insertion and its first repetition.
+ (defvar repeat-undo-count nil
+   "Number of self-insertions since last `undo-boundary'.")
+ 
  ;;;###autoload
  (defun repeat (repeat-arg)
    "Repeat most recently executed command.
***************
*** 293,303 ****
--- 301,323 ----
  		  (i 0))
  	      ;; Run pre- and post-command hooks for self-insertion too.
  	      (run-hooks 'pre-command-hook)
+ 	      (cond
+ 	       ((not repeat-undo-count))
+ 	       ((< repeat-undo-count 20)
+ 		;; Don't make an undo-boundary for 20 repetitions just
+ 		;; as the command loop does.
+ 		(setq repeat-undo-count (1+ repeat-undo-count)))
+ 	       (t
+ 		;; Make an undo-boundary now.
+ 		(undo-boundary)
+ 		(setq repeat-undo-count 1)))
  	      (while (< i count)
  		(repeat-self-insert insertion)
  		(setq i (1+ i)))
  	      (run-hooks 'post-command-hook)))
  	(let ((indirect (indirect-function last-repeatable-command)))
+ 	  ;; Make each repetition undo separately.
+ 	  (undo-boundary)
  	  (if (or (stringp indirect)
  		  (vectorp indirect))
  	      ;; Bind real-last-command so that executing the macro does
***************
*** 314,325 ****
        ;; (only 32 repetitions are possible given the default value of 200 for
        ;; max-lisp-eval-depth), but if I now locally disable the repeat char I
        ;; can iterate indefinitely here around a single level of recursion.
!       (let (repeat-on-final-keystroke)
  	(setq real-last-command 'repeat)
!         (while (eq (read-event) repeat-repeat-char)
! 	  ;; Make each repetition undo separately.
! 	  (undo-boundary)
!           (repeat repeat-arg))
          (setq unread-command-events (list last-input-event))))))
  
  (defun repeat-self-insert (string)
--- 334,353 ----
        ;; (only 32 repetitions are possible given the default value of 200 for
        ;; max-lisp-eval-depth), but if I now locally disable the repeat char I
        ;; can iterate indefinitely here around a single level of recursion.
!       (let (repeat-on-final-keystroke
! 	    ;; Bind `undo-inhibit-record-point' to t in order to avoid
! 	    ;; recording point in `buffer-undo-list' here.  We have to
! 	    ;; do this since the command loop does not set the last
! 	    ;; position of point thus confusing the point recording
! 	    ;; mechanism when inserting or deleting text.
! 	    (undo-inhibit-record-point t))
  	(setq real-last-command 'repeat)
! 	(setq repeat-undo-count 1)
! 	(unwind-protect
! 	    (while (eq (read-event) repeat-repeat-char)
! 	      (repeat repeat-arg))
! 	  ;; Make sure `repeat-undo-count' is reset.
! 	  (setq repeat-undo-count nil))
          (setq unread-command-events (list last-input-event))))))
  
  (defun repeat-self-insert (string)


[-- Attachment #3: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel