undo bug?

undo bug?

[Katsumi Yamaoka]

Hi,

I use egg[1] for writing Japanese text in Emacs buffers and have
a problem that the point jumps to an improper position when
performing `undo' after writing Japanese text.  This happens with
Emacs 22.1 and newer.  The following functions emulate what egg
does when writing Japanese text in order to reproduce the problem.

The scene it expresses is that I enter the ascii character `a'
within the fence (i.e. |...|) and convert it into the Japanese
character `A'.  Could you try evaluating those Lisp forms, and
performing `M-x x1', `M-x x2', and `M-x undo'?  You will see that
the point does not stay at the position where there was the
Japanese character `A'.

--8<---------------cut here---------------start------------->8---
(defun x1 ()
  (interactive)
  (funcall (if (and (get-buffer "*testing*")
		    (prog1 (string-equal (buffer-name) "*testing*")
		      (kill-buffer "*testing*")))
	       #'switch-to-buffer #'pop-to-buffer)
	   (generate-new-buffer "*testing*"))
  (text-mode)
  (use-local-map (copy-keymap (current-local-map)))
  (local-set-key "\C-m" 'x3)
  (insert "12345678")
  (goto-char 3))

(defun x2 ()
  (interactive)
  (insert "|a|")
  (sit-for 1)
  (setq unread-command-events '(?\C-m)))

(defun x3 ()
  (interactive)
  (delete-backward-char 3)
  (setq buffer-undo-list nil)
  (insert "A")
  (message "%s" buffer-undo-list))
--8<---------------cut here---------------end--------------->8---

Before performing `undo', `buffer-undo-list' has the value
`(nil (3 . 4) 6)', which means that `undo' will delete the thing
existing in the area (3 . 4) and move the point to 6.  Where does
`6' come from?  I'm not sure of it but it might be due to the way
to run the interactive command `x3'.  I.e., it is done by setting
`unread-command-events' to the key which is bound to the `x3'
command.  It doesn't happen in Emacs 21 and 20 anyway.

Now I use the following workaround just after inserting Japanese
text in order to remove `6' from `buffer-undo-list'.  But nothing
can be better than not using such one.  Could anyone look into it?

(if (car buffer-undo-list)
    (if (numberp (cadr buffer-undo-list))
	(setcdr buffer-undo-list (nthcdr 2 buffer-undo-list)))
  (if (numberp (nth 2 buffer-undo-list))
      (setcdr (cdr buffer-undo-list) (nthcdr 3 buffer-undo-list))))

Thanks in advance.

Regards,

[1] Egg v3 was first developed in 1988 for Nemacs, which was the
    enhanced version of Emacs 18 so as to be able to handle
    Japanese text.  It still survives as Emcws, sj3-egg, wnn7egg,
    and the egg-its XEmacs package.

Re: undo bug?

[martin rudalics]

 > (defun x3 ()
 >   (interactive)
 >   (delete-backward-char 3)
 >   (setq buffer-undo-list nil)
 >   (insert "A")
 >   (message "%s" buffer-undo-list))

Can you please test with (CVS) Emacs 23 whether

(defun x3 ()
   (interactive)
   (let ((undo-inhibit-record-point t))
     (delete-backward-char 3)
     (setq buffer-undo-list nil)
     (insert "A")
     (message "%s" buffer-undo-list)))

does what you want here?  If it solves your problem then we probably
should consider a solution like the one I sketched in

http://lists.gnu.org/archive/html/bug-gnu-emacs/2008-03/msg00096.html

Re: undo bug?

[Katsumi Yamaoka]

>>>>> martin rudalics wrote:
> Can you please test with (CVS) Emacs 23 whether

> (defun x3 ()
>   (interactive)
>   (let ((undo-inhibit-record-point t))
>     (delete-backward-char 3)
>     (setq buffer-undo-list nil)
>     (insert "A")
>     (message "%s" buffer-undo-list)))

> does what you want here?  If it solves your problem then we probably
> should consider a solution like the one I sketched in

> http://lists.gnu.org/archive/html/bug-gnu-emacs/2008-03/msg00096.html

Oh, it does the trick.  I hope it is fixed in not only the Emacs
trunk but also Emacs 22.3.  Thanks!

Re: undo bug?

[Stefan Monnier]

> does what you want here?  If it solves your problem then we probably
> should consider a solution like the one I sketched in

> http://lists.gnu.org/archive/html/bug-gnu-emacs/2008-03/msg00096.html

I'm wondering why we have to use last_point_position.
It seems to be asking for problems because it is delimited by command
processing, whereas the undo records are delimited by undo-boundary:
there is a correlation between the two, but no equivalence.

I.e. I suggest we introduce last_undo_boundary_pos and use it in place
of last_point_position in undo.c.
It'd be set to PT in Fundo_boundary (which could/should also set
last_undo_buffer).

WDYT?


        Stefan

Re: undo bug?

[martin rudalics]

 > I'm wondering why we have to use last_point_position.
 > It seems to be asking for problems because it is delimited by command
 > processing, whereas the undo records are delimited by undo-boundary:
 > there is a correlation between the two, but no equivalence.
 >
 > I.e. I suggest we introduce last_undo_boundary_pos and use it in place
 > of last_point_position in undo.c.
 > It'd be set to PT in Fundo_boundary (which could/should also set
 > last_undo_buffer).

Couldn't we do away with undo-boundaries and always undo until the next
recorded position of `point'?

Re: undo bug?

[Stefan Monnier]

>> I'm wondering why we have to use last_point_position.
>> It seems to be asking for problems because it is delimited by command
>> processing, whereas the undo records are delimited by undo-boundary:
>> there is a correlation between the two, but no equivalence.
>> 
>> I.e. I suggest we introduce last_undo_boundary_pos and use it in place
>> of last_point_position in undo.c.
>> It'd be set to PT in Fundo_boundary (which could/should also set
>> last_undo_buffer).

> Couldn't we do away with undo-boundaries and always undo until the next
> recorded position of `point'?

In theory we could, but that may break various packages: of all the
supported undo records, the "undo-boundary" is probably the only one
that is used by external packages ;-(

Instead the current code tries to only insert a point-record when
it's actually necessary.


        Stefan

Re: undo bug?

[martin rudalics]

 >>Couldn't we do away with undo-boundaries and always undo until the next
 >>recorded position of `point'?
 >
 >
 > In theory we could, but that may break various packages: of all the
 > supported undo records, the "undo-boundary" is probably the only one
 > that is used by external packages ;-(

What I had in mind was that `undo-boundary' would simply `record-point'
instead of inserting nil.

 > Instead the current code tries to only insert a point-record when
 > it's actually necessary.

Sometimes.  For example I use the patch below to handle the (pretty
annoying) problem that when I redo an earlier insertion `point' ends up
_before_ the inserted text (my `undo' binds `undo-is-redo' to `equiv'
when calling `undo-more').

*** undo.c.~1.83.~	Sun Jan 20 10:33:56 2008
--- undo.c	Fri Mar  7 19:38:40 2008
***************
*** 57,62 ****
--- 57,66 ----

   int undo_inhibit_record_point;

+ /* Nonzero means undo is redo.  */
+
+ int undo_is_redo;
+
   /* 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
***************
*** 637,643 ****
   			 so that undoing the marker adjustments
   			 put the markers back in the right place.  */
   		      Finsert (1, &membuf);
! 		      SET_PT (pos);
   		    }
   		}
   	      else if (MARKERP (car) && INTEGERP (cdr))
--- 641,648 ----
   			 so that undoing the marker adjustments
   			 put the markers back in the right place.  */
   		      Finsert (1, &membuf);
! 		      if (! undo_is_redo)
! 			SET_PT (pos);
   		    }
   		}
   	      else if (MARKERP (car) && INTEGERP (cdr))
***************
*** 735,740 ****
--- 740,749 ----
     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;
+
+   DEFVAR_BOOL ("undo-is-redo", &undo_is_redo,
+ 	       doc: /* Non-nil means undo is redo.  */);
+   undo_is_redo = 0;
   }

   /* arch-tag: d546ee01-4aed-4ffb-bb8b-eefaae50d38a

Re: undo bug?

[Stefan Monnier]

>>> Couldn't we do away with undo-boundaries and always undo until the next
>>> recorded position of `point'?
>> In theory we could, but that may break various packages: of all the
>> supported undo records, the "undo-boundary" is probably the only one
>> that is used by external packages ;-(

> What I had in mind was that `undo-boundary' would simply `record-point'
> instead of inserting nil.

Yes, that's indeed what I had understood from your earlier message and
what I was responding to.  You're remove the undo boundaries (i.e. the
nil entries) and replace them by point-record entries.  That would work
fine, except that there are packages out there that look for the
nil entries.

>> Instead the current code tries to only insert a point-record when
>> it's actually necessary.

> Sometimes.  For example I use the patch below to handle the (pretty
> annoying) problem that when I redo an earlier insertion `point' ends up
> _before_ the inserted text (my `undo' binds `undo-is-redo' to `equiv'
> when calling `undo-more').

Have you tried to analyze the source of the problem?  Maybe there are
cases where we incorrectly decide not to put a point-record even tho it
is needed.


        Stefan

Re: undo bug?

[martin rudalics]

 > Yes, that's indeed what I had understood from your earlier message and
 > what I was responding to.  You're remove the undo boundaries (i.e. the
 > nil entries) and replace them by point-record entries.  That would work
 > fine, except that there are packages out there that look for the
 > nil entries.

Too bad, indeed.  Are they looking out just for entries they created
themselves?

 >>Sometimes.  For example I use the patch below to handle the (pretty
 >>annoying) problem that when I redo an earlier insertion `point' ends up
 >>_before_ the inserted text (my `undo' binds `undo-is-redo' to `equiv'
 >>when calling `undo-more').
 >
 > Have you tried to analyze the source of the problem?  Maybe there are
 > cases where we incorrectly decide not to put a point-record even tho it
 > is needed.

I think we should enclose every undoable character-modifiying buffer
change by a `record-point' before and a `record-point' after it.  An
`undo' would reestablish the position recorded before, a `redo' the
position recorded after the change.  This way we would handle all
flavors of inserting text before/after `point' as well as undoing and
redoing.

Re: undo bug?

[Stefan Monnier]

>> Yes, that's indeed what I had understood from your earlier message and
>> what I was responding to.  You're remove the undo boundaries (i.e. the
>> nil entries) and replace them by point-record entries.  That would work
>> fine, except that there are packages out there that look for the
>> nil entries.

> Too bad, indeed.  Are they looking out just for entries they created
> themselves?

>>> Sometimes.  For example I use the patch below to handle the (pretty
>>> annoying) problem that when I redo an earlier insertion `point' ends up
>>> _before_ the inserted text (my `undo' binds `undo-is-redo' to `equiv'
>>> when calling `undo-more').
>> 
>> Have you tried to analyze the source of the problem?  Maybe there are
>> cases where we incorrectly decide not to put a point-record even tho it
>> is needed.

> I think we should enclose every undoable character-modifiying buffer
> change by a `record-point' before and a `record-point' after it.  An
> `undo' would reestablish the position recorded before, a `redo' the
> position recorded after the change.  This way we would handle all
> flavors of inserting text before/after `point' as well as undoing and
> redoing.

The record-point after it wouldn't make anny difference, would it?
Hmm... unless undoing a `record-point' would forcefully add another
record-point entry i nthe undo-list... I guess that might work.

Still, I'm wondering what will be the effect of such a thing on the
length of the undo-list.  It's a pretty minor issue, as far as I can
tell, so it had better not have any noticeable downside.


        Stefan

Re: undo bug?

[martin rudalics]

 > The record-point after it wouldn't make anny difference, would it?

It's needed for getting there after a redo.

 > Hmm... unless undoing a `record-point' would forcefully add another
 > record-point entry i nthe undo-list... I guess that might work.

Any `record-point' would simply have to preserve the symmetry of

UB RPB buffer-changes RPA UB RPB buffer-changes RPA UB

where UB is the undo-boundary, RPB and RPA the `record-point's before
and after the buffer-changes, and the buffer-changes would not contain
any undo-boundaries or point-recordings.

 > Still, I'm wondering what will be the effect of such a thing on the
 > length of the undo-list.  It's a pretty minor issue, as far as I can
 > tell, so it had better not have any noticeable downside.

We could remove certain text-properties from undo-lists (mine are
usually full of 'fontified properties in deleted text).