bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Chong Yidong]

In GNU Emacs 25.1.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.18.9)

1. emacs -Q foo.tex

2. C-c C-o RET

   The buffer now contains an `enumerate' skeleton:

\begin{enumerate}
\item 
\end{enumerate}

3. C-/

   After this undo command, the buffer now contains

\begin{

   Expected behavior: C-/ should undo the entire effects of C-c C-o, so
   the buffer should contain nothing at all.  You shouldn't need another
   C-/ to get rid of the "\begin{".

This happens because latex-insert-block uses skeleton-insert, in which
the "interactor" specified for reading a string is lazily invoked only
when the string appears in the skeleton.  Hence, the "\begin{" is
inserted into the buffer first, and then completing-read is called to
ask for the LaTeX block name.  The completing-read adds an undo
boundary, in the midst of the changes by latex-insert-block.

The attached patch, which gets rid of the undo boundary, seems to fix
this:


--- a/lisp/textmodes/tex-mode.el
+++ b/lisp/textmodes/tex-mode.el
@@ -1539,7 +1539,8 @@ 'tex-latex-block
 (define-skeleton latex-insert-block
   "Create a matching pair of lines \\begin{NAME} and \\end{NAME} at point.
 Puts point on a blank line between them."
-  (let ((choice (completing-read (format "LaTeX block name [%s]: "
+  (let* ((buffer-undo-list t) ; Don't add an undo boundary
+         (choice (completing-read (format "LaTeX block name [%s]: "
 					 latex-block-default)
                                  (latex-complete-envnames)
 				 nil nil nil nil latex-block-default)))

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Chong Yidong]

> The attached patch, which gets rid of the undo boundary, seems to fix
> this:

Actually, the previous patch does not DTRT: if you switch back to the
original buffer from the minibuffer, and make further editing changes,
those changes would get lost because buffer-undo-list is temporarily
rebound.

Here is a different patch, which works by removing the undo boundary in
buffer-undo-list if there's one.  It also tweaks HTML mode and Texinfo
mode, which have similar issues.  It defines a new function
`undo-amalgamate', split off from `undo-auto-amalgamate', for
convenience.

diff --git a/lisp/simple.el b/lisp/simple.el
index e257062..decd737 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2939,18 +2939,17 @@ undo-auto-amalgamate
           ;; Amalgamate all buffers that have changed.
           (dolist (b (cdr undo-auto--last-boundary-cause))
             (when (buffer-live-p b)
-              (with-current-buffer
-                  b
-                (when
-                    ;; The head of `buffer-undo-list' is nil.
-                    ;; `car-safe' doesn't work because
-                    ;; `buffer-undo-list' need not be a list!
-                    (and (listp buffer-undo-list)
-                         (not (car buffer-undo-list)))
-                  (setq buffer-undo-list
-                        (cdr buffer-undo-list))))))
+              (with-current-buffer b
+                (undo-amalgamate))))
         (setq undo-auto--last-boundary-cause 0)))))
 
+(defun undo-amalgamate ()
+  "Amalgamate undo in the current buffer."
+  ;; `car-safe' doesn't work as `buffer-undo-list' need not be a list!
+  (and (listp buffer-undo-list)
+       (null (car buffer-undo-list))
+       (pop buffer-undo-list)))
+
 (defun undo-auto--undoable-change ()
   "Called after every undoable buffer change."
   (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer))
diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index 990c09b..51b7241 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -700,11 +700,15 @@ sgml-tag
 `sgml-transformation-function' to `upcase'."
   (funcall (or skeleton-transformation-function 'identity)
            (setq sgml-tag-last
-		 (completing-read
-		  (if (> (length sgml-tag-last) 0)
-		      (format "Tag (default %s): " sgml-tag-last)
-		    "Tag: ")
-		  sgml-tag-alist nil nil nil 'sgml-tag-history sgml-tag-last)))
+                 ;; Avoid creating an undo boundary.
+                 (prog1
+                     (completing-read
+                      (if (> (length sgml-tag-last) 0)
+                          (format "Tag (default %s): " sgml-tag-last)
+                        "Tag: ")
+                      sgml-tag-alist nil nil nil
+                      'sgml-tag-history sgml-tag-last)
+                   (undo-amalgamate))))
   ?< str |
   (("") -1 '(undo-boundary) (identity "&lt;")) |	; see comment above
   `(("") '(setq v2 (sgml-attributes ,str t)) ?>
diff --git a/lisp/textmodes/tex-mode.el b/lisp/textmodes/tex-mode.el
index b38b147..8b7d98f 100644
--- a/lisp/textmodes/tex-mode.el
+++ b/lisp/textmodes/tex-mode.el
@@ -1539,10 +1539,13 @@ 'tex-latex-block
 (define-skeleton latex-insert-block
   "Create a matching pair of lines \\begin{NAME} and \\end{NAME} at point.
 Puts point on a blank line between them."
-  (let ((choice (completing-read (format "LaTeX block name [%s]: "
-					 latex-block-default)
-                                 (latex-complete-envnames)
-				 nil nil nil nil latex-block-default)))
+  (let* ((buffer-undo-list t)
+         (choice (prog1
+                     (completing-read (format "LaTeX block name [%s]: "
+                                              latex-block-default)
+                                      (latex-complete-envnames)
+                                      nil nil nil nil latex-block-default)
+                   (undo-amalgamate))))
     (setq latex-block-default choice)
     (unless (or (member choice latex-standard-block-names)
 		(member choice latex-block-names))
diff --git a/lisp/textmodes/texinfo.el b/lisp/textmodes/texinfo.el
index ed6022f..fd411e2 100644
--- a/lisp/textmodes/texinfo.el
+++ b/lisp/textmodes/texinfo.el
@@ -653,9 +653,12 @@ texinfo-insert-block
   "Create a matching pair @<cmd> .. @end <cmd> at point.
 Puts point on a blank line between them."
   (setq texinfo-block-default
-	(completing-read (format "Block name [%s]: " texinfo-block-default)
-			 texinfo-environments
-			 nil nil nil nil texinfo-block-default))
+        (prog1
+            (completing-read (format "Block name [%s]: "
+                                     texinfo-block-default)
+                             texinfo-environments
+                             nil nil nil nil texinfo-block-default)
+          (undo-amalgamate)))
   \n "@" str
   ;; Blocks that take parameters: all the def* blocks take parameters,
   ;;  plus a few others.

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

Chong Yidong <cyd@gnu.org> writes:

>> The attached patch, which gets rid of the undo boundary, seems to fix
>> this:
>
> Actually, the previous patch does not DTRT: if you switch back to the
> original buffer from the minibuffer, and make further editing changes,
> those changes would get lost because buffer-undo-list is temporarily
> rebound.
>
> Here is a different patch, which works by removing the undo boundary in
> buffer-undo-list if there's one.  It also tweaks HTML mode and Texinfo
> mode, which have similar issues.  It defines a new function
> `undo-amalgamate', split off from `undo-auto-amalgamate', for
> convenience.


In and off itself, the patch seems fine, but my concern is that that the
previous heuristic did the right thing, the new heuristic does not. If
you've found three instances where it's causing a problem, then there
will be others also.

I'm not 100% sure why the old system didn't insert an undo-boundary.
But, if we could solve this entirely in the undo system without changes
to client code that would be nicer.

Not sure how yet -- need a few days to think about it. Perhaps,
suppressing the auto-boundary functionality when only the mini-buffer
has changed.

Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

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

phillip.lord@russet.org.uk (Phillip Lord) writes:

> Chong Yidong <cyd@gnu.org> writes:
>
>>> The attached patch, which gets rid of the undo boundary, seems to fix
>>> this:
>>
>> Actually, the previous patch does not DTRT: if you switch back to the
>> original buffer from the minibuffer, and make further editing changes,
>> those changes would get lost because buffer-undo-list is temporarily
>> rebound.
>>
>> Here is a different patch, which works by removing the undo boundary in
>> buffer-undo-list if there's one.  It also tweaks HTML mode and Texinfo
>> mode, which have similar issues.  It defines a new function
>> `undo-amalgamate', split off from `undo-auto-amalgamate', for
>> convenience.
>
>
> In and off itself, the patch seems fine, but my concern is that that the
> previous heuristic did the right thing, the new heuristic does not. If
> you've found three instances where it's causing a problem, then there
> will be others also.
>
> I'm not 100% sure why the old system didn't insert an undo-boundary.
> But, if we could solve this entirely in the undo system without changes
> to client code that would be nicer.
>
> Not sure how yet -- need a few days to think about it. Perhaps,
> suppressing the auto-boundary functionality when only the mini-buffer
> has changed.

I've debugged this now.

The problem, I think, is that latex-insert-block uses recursive editing
(via `completing-read', then `read-from-minibuffer') -- so the
minibuffer is edited, then the exit-minibuffer command runs, causing an
undo boundary to be added to minibuffer (correctly) but also to the
latex buffer because it has also changed.

The patch below seems to fix -- I need to test it out tomorrow in case
it has any other unexpected effects.

What worries me is that it just deals with the minibuffer. I wonder
whether there are other circumstances where a recursive edit is going to
break things.

Stefan, would welcome your opinion here.

Incidentally, this is a nightmare to debug. Emacs needs to be able to
write to standard out, so I could log without changing any buffers!



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Stop-mini-buffer-causing-undo-boundaries.patch --]
[-- Type: text/x-diff, Size: 1925 bytes --]

From 9116e7f00f90fb14857d21698b1e6870fcf98bbd Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Mon, 30 May 2016 22:50:36 +0100
Subject: [PATCH] Stop mini-buffer causing undo boundaries

* lisp/simple.el (undo-auto--boundaries): Check whether minibuffer is
  current, and if so limit undo-boundaries to it.

Addresses #23632
---
 lisp/simple.el | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index c5aa292..788cbb2 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2887,12 +2887,21 @@ undo-auto--boundaries
   "Check recently changed buffers and add a boundary if necessary.
 REASON describes the reason that the boundary is being added; see
 `undo-last-boundary' for more information."
-  (dolist (b undo-auto--undoably-changed-buffers)
-          (when (buffer-live-p b)
-            (with-current-buffer b
-              (unless undo-auto-disable-boundaries
-                (undo-auto--ensure-boundary cause)))))
-  (setq undo-auto--undoably-changed-buffers nil))
+  ;; We treat the minibuffer specially, because some commands use the
+  ;; minibuffer after changing the buffer that they are launched
+  ;; from. Changes in the minibuffer force an undo-boundary in the
+  ;; launched buffer without this handling. (see bug #23632)
+  (if (minibufferp)
+      (progn
+        (undo-auto--ensure-boundary cause)
+        (setq undo-auto--undoably-changed-buffers
+              (delq (current-buffer) undo-auto--undoably-changed-buffers)))
+    (dolist (b undo-auto--undoably-changed-buffers)
+      (when (buffer-live-p b)
+        (with-current-buffer b
+          (unless undo-auto-disable-boundaries
+            (undo-auto--ensure-boundary cause)))))
+    (setq undo-auto--undoably-changed-buffers nil)))
 
 (defun undo-auto--boundary-timer ()
   "Timer which will run `undo--auto-boundary-timer'."
-- 
2.8.3

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

> What worries me is that it just deals with the minibuffer.  I wonder
> whether there are other circumstances where a recursive edit is going to
> break things.

I guess we could introduce a new var (call it
`undo-auto-current-buffer-only' or `undo-auto-ignore-other-buffers' or
what have you) which packages could let-bind around recursive edits.
We could also change the minibuf.c code to bind this var, so you could
check the var instead of hard-coding (minibufferp) in your patch.

The main use-case I can think of would be debug/edebug.

This said, if the changes in other buffers are due to process-filters,
then they still should get an undo-boundary during
minibuffer/recursive edits.  So maybe instead of "only push
undo-boundaries in current-buffer", we should have a variable holding
a list of buffers where we shouldn't push undo-boundaries (unless
they're the current-buffer).

Or an alternative would be to do what Viper does (well, did): keep
pushing boundaries as before, but when we return from the minibuffer,
remove any boundaries that were inserted into the current-buffer's
undo-list during the recursive edit.

> Incidentally, this is a nightmare to debug.  Emacs needs to be able to
> write to standard out, so I could log without changing any buffers!

What I do is to push to a variable, and then observe the var from M-x
ielm or some such.


        Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> What worries me is that it just deals with the minibuffer.  I wonder
>> whether there are other circumstances where a recursive edit is going to
>> break things.
>
> I guess we could introduce a new var (call it
> `undo-auto-current-buffer-only' or `undo-auto-ignore-other-buffers' or
> what have you) which packages could let-bind around recursive edits.
> We could also change the minibuf.c code to bind this var, so you could
> check the var instead of hard-coding (minibufferp) in your patch.
>
> The main use-case I can think of would be debug/edebug.
>
> This said, if the changes in other buffers are due to process-filters,
> then they still should get an undo-boundary during
> minibuffer/recursive edits.  So maybe instead of "only push
> undo-boundaries in current-buffer", we should have a variable holding
> a list of buffers where we shouldn't push undo-boundaries (unless
> they're the current-buffer).
>
> Or an alternative would be to do what Viper does (well, did): keep
> pushing boundaries as before, but when we return from the minibuffer,
> remove any boundaries that were inserted into the current-buffer's
> undo-list during the recursive edit.

What I dislike about this is that other packages are responsible for
getting things right.

What about this

1) undo-auto--undoably-changed-buffers becomes an alist

 ((0 . (buffers*))
  (1 . (buffers*))
  (2 . (buffers*)))

where the key is the return of (recursion-depth)

2) undo-auto--boundaries operates only on buffers at the
current-recursion-depth. Or, probably, at the current of greater
recursion depth, to ensure that undo-buffers happens when a recursive
edit exits.

This way, process buffers will still get an undo-boundary if they change
during the recursive edit.



>> Incidentally, this is a nightmare to debug.  Emacs needs to be able to
>> write to standard out, so I could log without changing any buffers!
>
> What I do is to push to a variable, and then observe the var from M-x
> ielm or some such.

That's a good idea -- I've been doing the former, then using C-hv.

Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

> 1) undo-auto--undoably-changed-buffers becomes an alist

>  ((0 . (buffers*))
>   (1 . (buffers*))
>   (2 . (buffers*)))

> where the key is the return of (recursion-depth)

> 2) undo-auto--boundaries operates only on buffers at the
> current-recursion-depth. Or, probably, at the current of greater
> recursion depth, to ensure that undo-buffers happens when a recursive
> edit exits.

Hmm... sounds pretty good, and I think you can do it more simply:
just let-bind undo-auto--undoably-changed-buffers to nil when entering
a recursive edit.


        Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

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

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> 1) undo-auto--undoably-changed-buffers becomes an alist
>
>>  ((0 . (buffers*))
>>   (1 . (buffers*))
>>   (2 . (buffers*)))
>
>> where the key is the return of (recursion-depth)
>
>> 2) undo-auto--boundaries operates only on buffers at the
>> current-recursion-depth. Or, probably, at the current of greater
>> recursion depth, to ensure that undo-buffers happens when a recursive
>> edit exits.
>
> Hmm... sounds pretty good, and I think you can do it more simply:
> just let-bind undo-auto--undoably-changed-buffers to nil when entering
> a recursive edit.

Tried it before I got your mail. Seems to function.

Simple let binding would not give quite the same functionality, because
of the last part -- I also add a boundary to buffers with a greater
recursive depth; with a let binding, I think these would be unbound for
commands that lower the recursion depth.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-undo-boundaries-based-on-recursion-depth.patch --]
[-- Type: text/x-diff, Size: 8031 bytes --]

From 018429dafe1395e0927934fd95d65b74c69b2d07 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Mon, 30 May 2016 22:50:36 +0100
Subject: [PATCH] Add undo-boundaries based on recursion depth

* lisp/simple.el (undo-auto--undoably-changed-buffers): Now an alist
  which stores recursion depth as well as
  changes.
  (undo-auto--boundaries): Add boundaries only to buffers at
  current or higher recursion depth.
  (undo-auto--undoable-change): Adjust for other changes.
  (undo-auto--extract-buffers, undo-auto--undoable-change-1): New
  functions.

Addresses #23632
---
 lisp/simple.el                | 66 +++++++++++++++++++++++++-----
 test/automated/simple-test.el | 94 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 149 insertions(+), 11 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index c5aa292..d55d9b0 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2867,7 +2867,7 @@ undo-auto--last-boundary-amalgamating-number
 that calls `undo-auto-amalgamate'."
   (car-safe undo-auto--last-boundary-cause))
 
-(defun undo-auto--ensure-boundary (cause)
+(defun undo-auto--ensure-boundary (changed-buffers cause)
   "Add an `undo-boundary' to the current buffer if needed.
 REASON describes the reason that the boundary is being added; see
 `undo-auto--last-boundary' for more information."
@@ -2880,19 +2880,44 @@ undo-auto--ensure-boundary
             (if (eq 'amalgamate cause)
                 (cons
                  (if last-amalgamating (1+ last-amalgamating) 0)
-                 undo-auto--undoably-changed-buffers)
+                 changed-buffers)
               cause)))))
 
+(defun undo-auto--extract-buffers (depth buffer-list &optional no-change)
+  "Extract buffers from BUFFER-LIST at DEPTH or deeper.
+
+This expects BUFFER-LIST to be of the form of
+`undo-auto--undoably-changed-buffers'. The relevant buffers are
+returned, and removed from BUFFER-LIST by side effect.
+
+If NO-CHANGE is non-nil, do not alter BUFFER-LIST."
+  (let ((rtn))
+    (dolist (level-and-buffer buffer-list)
+      (when (<= depth
+                (car level-and-buffer))
+        (dolist (b (cdr level-and-buffer))
+          (setq rtn (cons b rtn)))
+        (unless no-change
+          (setcdr level-and-buffer nil))))
+    rtn))
+
 (defun undo-auto--boundaries (cause)
   "Check recently changed buffers and add a boundary if necessary.
 REASON describes the reason that the boundary is being added; see
 `undo-last-boundary' for more information."
-  (dolist (b undo-auto--undoably-changed-buffers)
-          (when (buffer-live-p b)
-            (with-current-buffer b
-              (unless undo-auto-disable-boundaries
-                (undo-auto--ensure-boundary cause)))))
-  (setq undo-auto--undoably-changed-buffers nil))
+  ;; We must account for recusion depth in general, and also
+  ;; specifically, to cope with changes in the minibuffer (see bug
+  ;; #23632).
+  (let ((changed-buffers
+         (undo-auto--extract-buffers
+          (recursion-depth)
+          ;; This is updated by side effect.
+          undo-auto--undoably-changed-buffers)))
+    (dolist (b changed-buffers)
+      (when (buffer-live-p b)
+        (with-current-buffer b
+          (unless undo-auto-disable-boundaries
+            (undo-auto--ensure-boundary changed-buffers cause)))))))
 
 (defun undo-auto--boundary-timer ()
   "Timer which will run `undo--auto-boundary-timer'."
@@ -2912,6 +2937,10 @@ undo-auto--undoably-changed-buffers
 `undo-auto--boundaries' and can be affected by changes to their
 default values.
 
+It is an list of conses whose car is the last returned value of
+`recursion-depth', and whose cdr is the list of changed buffers at that
+depth.
+
 See also `undo-auto--buffer-undoably-changed'.")
 
 (defun undo-auto--add-boundary ()
@@ -2955,9 +2984,26 @@ undo-auto-amalgamate
                         (cdr buffer-undo-list))))))
         (setq undo-auto--last-boundary-cause 0)))))
 
-(defun undo-auto--undoable-change ()
+(defun undo-auto--undoable-change-1 (depth buffer changed-buffers)
   "Called after every undoable buffer change."
-  (add-to-list 'undo-auto--undoably-changed-buffers (current-buffer))
+  (let ((buffers-for-depth (assoc depth changed-buffers)))
+    (if buffers-for-depth
+        (progn
+          (setcdr buffers-for-depth
+                  (cons buffer (cdr buffers-for-depth)))
+          changed-buffers)
+      (cons
+       (list
+        depth
+        buffer)
+       changed-buffers))))
+
+(defun undo-auto--undoable-change ()
+  (setq undo-auto--undoably-changed-buffers
+        (undo-auto--undoable-change-1
+         (recursion-depth)
+         (current-buffer)
+         undo-auto--undoably-changed-buffers))
   (undo-auto--boundary-ensure-timer))
 ;; End auto-boundary section
 
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 12ebc75..57dbbf0 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -237,7 +237,9 @@ simple-test--transpositions
    (with-temp-buffer
      (setq buffer-undo-list nil)
      (insert "hello")
-     (member (current-buffer) undo-auto--undoably-changed-buffers)))
+     (member (current-buffer)
+             (assoc 0
+                    undo-auto--undoably-changed-buffers))))
   ;; The head of buffer-undo-list should be the insertion event, and
   ;; therefore not nil
   (should
@@ -310,6 +312,96 @@ undo-test-point-after-forward-kill
    (= 6
       (undo-test-point-after-forward-kill))))
 
+(ert-deftest simple-test-undo--auto-undoable-change-1 ()
+  (should
+   (equal
+    '((0 a))
+    (undo-auto--undoable-change-1 0 'a nil)))
+  (should
+   (equal
+    '((0 b a))
+    (undo-auto--undoable-change-1
+     0 'b
+     '((0 a)))))
+  (should
+   (equal
+    '((1 c)
+      (0 a))
+    (undo-auto--undoable-change-1
+     1 'c '((0 a))))))
+
+(ert-deftest simple-test-undo-auto--extract-buffers ()
+  (should
+   (equal
+    (list
+     '((1)
+       (0 a))
+     '(c))
+    (let
+        ((buffer-list
+          '((1 c)
+            (0 a))))
+      (list
+       buffer-list
+       (undo-auto--extract-buffers
+        1 buffer-list))))))
+
+;; These macros are lifted from assess-robot.el, and should be removed
+;; when that comes into core.
+(defmacro simple-test-with-switched-buffer (buffer &rest body)
+  (declare (indent 1) (debug t))
+  (let ((before-buffer (make-symbol "before-buffer")))
+    `(let ((,before-buffer (current-buffer)))
+       (unwind-protect
+           (progn
+             (switch-to-buffer ,buffer)
+             ,@body)
+         (switch-to-buffer ,before-buffer)))))
+
+(defmacro simple-test-with-temp-switched-buffer (&rest body)
+  (declare (indent 0) (debug t))
+  (let ((temp-buffer (make-symbol "temp-buffer")))
+    `(let ((,temp-buffer (generate-new-buffer " *temp*")))
+       (simple-test-with-switched-buffer ,temp-buffer
+         (unwind-protect
+             (progn
+               (setq buffer-undo-list nil)
+               ,@body)
+           (and (buffer-name ,temp-buffer)
+                ;(kill-buffer ,temp-buffer)
+                ))))))
+
+(ert-deftest simple-test-undo-amalgamation ()
+  ;; We should undo 0123456789 but not hello
+  (should
+   (string=
+    "hello
+"
+    (simple-test-with-temp-switched-buffer
+      (execute-kbd-macro
+       (read-kbd-macro
+        "
+hello        ;; self-insert-command
+RET          ;; newline
+0123456789   ;; self-insert-command
+C-/          ;; undo
+"
+        ))
+      (buffer-substring-no-properties (point-min)
+                                      (point-max)))))
+  ;; we should leave 20 characters in the buffer
+  (should
+   (string=
+    "012345678901234567890"
+    (simple-test-with-temp-switched-buffer
+      (execute-kbd-macro
+       (read-kbd-macro
+        "
+012345678901234567890123456789   ;; self-insert-command
+C-/      ;; undo"))
+      (buffer-substring-no-properties
+       (point-min)
+       (point-max))))))
 
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.8.3

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

> Simple let binding would not give quite the same functionality, because
> of the last part -- I also add a boundary to buffers with a greater
> recursive depth; with a let binding, I think these would be unbound for
> commands that lower the recursion depth.

Ah, you mean that the value of undo-auto--undoably-changed-buffers needs
to be propagated "out" when we leave the let-binding.  You're right.
So instead of a simple `let', it needs to be something like:

    (let ((tmp ()))
      (unwind-protect
          (let ((undo-auto--undoably-changed-buffers nil))
            (unwind-protect
                <do-it-all>
              (setq tmp undo-auto--undoably-changed-buffers)))
        (setq undo-auto--undoably-changed-buffers
              (append tmp undo-auto--undoably-changed-buffers))))

Or

    (let ((tmp undo-auto--undoably-changed-buffers))
      (unwind-protect
          (progn
            (setq undo-auto--undoably-changed-buffers nil)
            <do-it-all>)
        (setq undo-auto--undoably-changed-buffers
              (append undo-auto--undoably-changed-buffers tmp))))

Maybe a simple alternative would be to do

    (let ((undo-auto--undoably-changed-buffers nil))
      (unwind-protect
          <do-it-all>
        (undo-auto--ensure-boundary undo-auto--undoably-changed-buffers)))


-- Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Simple let binding would not give quite the same functionality, because
>> of the last part -- I also add a boundary to buffers with a greater
>> recursive depth; with a let binding, I think these would be unbound for
>> commands that lower the recursion depth.
>
> Ah, you mean that the value of undo-auto--undoably-changed-buffers needs
> to be propagated "out" when we leave the let-binding.

I *think* so -- I'm not entirely sure. It might make no difference.


> You're right. So instead of a simple `let', it needs to be something
> like:
>
>     (let ((tmp ()))
>       (unwind-protect
>           (let ((undo-auto--undoably-changed-buffers nil))
>             (unwind-protect
>                 <do-it-all>
>               (setq tmp undo-auto--undoably-changed-buffers)))
>         (setq undo-auto--undoably-changed-buffers
>               (append tmp undo-auto--undoably-changed-buffers))))
>
> Or
>
>     (let ((tmp undo-auto--undoably-changed-buffers))
>       (unwind-protect
>           (progn
>             (setq undo-auto--undoably-changed-buffers nil)
>             <do-it-all>)
>         (setq undo-auto--undoably-changed-buffers
>               (append undo-auto--undoably-changed-buffers tmp))))
>
> Maybe a simple alternative would be to do
>
>     (let ((undo-auto--undoably-changed-buffers nil))
>       (unwind-protect
>           <do-it-all>
>         (undo-auto--ensure-boundary undo-auto--undoably-changed-buffers)))

I use this variable in several different places in two different places
though -- once when we capture the undoable changes (which happens
often) and once on at the end of each command. I'd have to do this let
binding in the command loop?

My current solution seems simpler, even if it does feel like I have
created "recursion-level" local variables.

Or am I totally mis-understanding what you are suggesting? I'd be
happier with a simpler implementation if possible.

Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

>>> Simple let binding would not give quite the same functionality, because
>>> of the last part -- I also add a boundary to buffers with a greater
>>> recursive depth; with a let binding, I think these would be unbound for
>>> commands that lower the recursion depth.
>> Ah, you mean that the value of undo-auto--undoably-changed-buffers needs
>> to be propagated "out" when we leave the let-binding.
> I *think* so -- I'm not entirely sure. It might make no difference.

It makes a difference, since otherwise we may forget that some changes
were made in a buffer and fail to push a boundary for them.  Not super
terribly serious, admittedly.

> I use this variable in several different places in two different places
> though

Not sure what you mean by "use", and there's clearly some typo about
"places" which makes the meaning even more murky.

> -- once when we capture the undoable changes (which happens
> often) and once on at the end of each command.

Right.  I see no need for any changes there.

> I'd have to do this let binding in the command loop?

We'd need this right when we enter a recursive-edit (minibuffer or not),
so maybe doing it when we enter the command loop would work.

> My current solution seems simpler, even if it does feel like I have
> created "recursion-level" local variables.

My impression that a let-binding plus a call to
undo-auto--ensure-boundary will be simpler than your patch.  But it's
hard to be sure until it's actually implemented.


        Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

On Sat, June 4, 2016 4:05 am, Stefan Monnier wrote:
>>>> Simple let binding would not give quite the same functionality,
>>>> because of the last part -- I also add a boundary to buffers with a
>>>> greater recursive depth; with a let binding, I think these would be
>>>> unbound for commands that lower the recursion depth.
>>> Ah, you mean that the value of undo-auto--undoably-changed-buffers
>>> needs to be propagated "out" when we leave the let-binding.
>> I *think* so -- I'm not entirely sure. It might make no difference.
>>
>
> It makes a difference, since otherwise we may forget that some changes
> were made in a buffer and fail to push a boundary for them.  Not super
> terribly serious, admittedly.

Yes. This is assuming that commands *both* change recursion depth *and*
change a buffer. If they are separate then there will no changes in the
buffer. Since I don't know this to be true, I am assuming that it isn't.

>
>> I use this variable in several different places in two different places
>>  though
>
> Not sure what you mean by "use", and there's clearly some typo about
> "places" which makes the meaning even more murky.

I change the value in this variable at one point (after each
undoable-change) by adding a buffer to it, access at one point (to find
boundaries that need amalgamating) and access it then nil parts of it at
another (to find buffers that need boundaries).

>
>> -- once when we capture the undoable changes (which happens
>> often) and once on at the end of each command.
>
> Right.  I see no need for any changes there.

Really? I have to know the recursion depth at this point

>> I'd have to do this let binding in the command loop?
>>
>
> We'd need this right when we enter a recursive-edit (minibuffer or not),
> so maybe doing it when we enter the command loop would work.
>
>> My current solution seems simpler, even if it does feel like I have
>> created "recursion-level" local variables.
>
> My impression that a let-binding plus a call to
> undo-auto--ensure-boundary will be simpler than your patch.  But it's hard
> to be sure until it's actually implemented.

To be clear, though, to do this I need to augment recursive-edit in C? I
need the let binding to last the life of the recursive edit?

Not being difficult here, just struggling to understand.

Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

> Yes. This is assuming that commands *both* change recursion depth *and*
> change a buffer.

Yes, it's like a fairly rare occurrence, where a command does both:
- modify some buffer(s)
- exit a recursive edit
So maybe we can live without paying attention to it.

>>> -- once when we capture the undoable changes (which happens
>>> often) and once on at the end of each command.
>> Right.  I see no need for any changes there.
> Really? I have to know the recursion depth at this point

No, let-bind the var to nil around each recursive edit should take care
of "everything" so you don't need to change anything else (including,
non need to pay any attention to the recursion depth).

> To be clear, though, to do this I need to augment recursive-edit in C? I
> need the let binding to last the life of the recursive edit?

That's right.  A call to `specbind' at the right spot might even be all
it takes.


        Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

On Sat, June 4, 2016 5:49 pm, Stefan Monnier wrote:
>> Yes. This is assuming that commands *both* change recursion depth *and*
>>  change a buffer.
>
> Yes, it's like a fairly rare occurrence, where a command does both:
> - modify some buffer(s)
> - exit a recursive edit
> So maybe we can live without paying attention to it.
>
>
>>>> -- once when we capture the undoable changes (which happens
>>>> often) and once on at the end of each command.
>>> Right.  I see no need for any changes there.
>>>
>> Really? I have to know the recursion depth at this point
>>
>
> No, let-bind the var to nil around each recursive edit should take care
> of "everything" so you don't need to change anything else (including, non
> need to pay any attention to the recursion depth).
>
>> To be clear, though, to do this I need to augment recursive-edit in C?
>> I
>> need the let binding to last the life of the recursive edit?
>
> That's right.  A call to `specbind' at the right spot might even be all
> it takes.

Okay, I will take a good look -- I've not done this before, but can try.

I guess "recursive-edit" is the only way to enter a recursive edit?

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

> I guess "recursive-edit" is the only way to enter a recursive edit?

AFAICT, `recursive_edit_1' is the C function that's used by both
`recursive-edit' and `read-from-minibuffer', so it should catch all cases.


        Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

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



Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> I guess "recursive-edit" is the only way to enter a recursive edit?
>
> AFAICT, `recursive_edit_1' is the C function that's used by both
> `recursive-edit' and `read-from-minibuffer', so it should catch all cases.


So, tried it, and AFAICT, you are correct. The attached patch seems to
fix. And it is significantly simpler than the last fix.

It currently does not deal with the case where there "left over" after a
command which changes a recursive edit level. The lists of buffers in
undo-auto-undoably-changed-buffer will be lost as we come out of the
specbind.

I do not know whether this is a problem or not. Potential solutions:

1) before we exit recursive_edit_1, append the value of
undo-auto-undoably-changed-buffer on a new variable
("undoably-changed-buffer-recursive"). Then, when undo-auto-boundary
runs append and nil this. Seems like a lot of effort for an occasional
issue.

2) Call undo-auto--add-boundary before exiting recursive_edit_1. This
should nil undoably-changed-buffer and add boundaries.

3) Just not worry about it.

Assuming we go for 3, is everyone happy to patch the Emacs-25 branch?

Phil



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-regression-for-recursive-editing-in-undo.patch --]
[-- Type: text/x-diff, Size: 1284 bytes --]

From 92ec383f9c732af76f6ba18c87a6989a4b6f16e8 Mon Sep 17 00:00:00 2001
From: Phillip Lord <phillip.lord@russet.org.uk>
Date: Mon, 6 Jun 2016 09:35:17 +0100
Subject: [PATCH] Fix regression for recursive editing in undo

---
 src/keyboard.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/keyboard.c b/src/keyboard.c
index e3858a5..b5b603c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -679,6 +679,8 @@ recursive_edit_1 (void)
      recursive edit, the original redisplay leading to the recursive
      edit will be unwound.  The outcome should therefore be safe.  */
   specbind (Qinhibit_redisplay, Qnil);
+
+  specbind (Qundo_auto__undoably_changed_buffers, Qnil);
   redisplaying_p = 0;
 
   val = command_loop ();
@@ -689,6 +691,7 @@ recursive_edit_1 (void)
   if (STRINGP (val))
     xsignal1 (Qerror, val);
 
+
   return unbind_to (count, Qnil);
 }
 
@@ -10956,6 +10959,8 @@ syms_of_keyboard (void)
   DEFSYM (Qpost_command_hook, "post-command-hook");
 
   DEFSYM (Qundo_auto__add_boundary, "undo-auto--add-boundary");
+  DEFSYM (Qundo_auto__undoably_changed_buffers,
+          "undo-auto--undoably-changed-buffers");
 
   DEFSYM (Qdeferred_action_function, "deferred-action-function");
   DEFSYM (Qdelayed_warnings_hook, "delayed-warnings-hook");
-- 
2.8.3

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Stefan Monnier]

> @@ -689,6 +691,7 @@ recursive_edit_1 (void)
>    if (STRINGP (val))
>      xsignal1 (Qerror, val);
> 
> +
>    return unbind_to (count, Qnil);
>  }

I'll let someone else decide whether it's acceptable for emacs-25.
AFAIC the patch looks good except for the above hunk.


        Stefan

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> @@ -689,6 +691,7 @@ recursive_edit_1 (void)
>>    if (STRINGP (val))
>>      xsignal1 (Qerror, val);
>> 
>> +
>>    return unbind_to (count, Qnil);
>>  }
>
> I'll let someone else decide whether it's acceptable for emacs-25.
> AFAIC the patch looks good except for the above hunk.

Ah, sorry. Will add some more documentation also.

Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Date: Mon, 06 Jun 2016 15:33:12 +0100
> Cc: Chong Yidong <cyd@gnu.org>, 23632@debbugs.gnu.org
> 
> Assuming we go for 3, is everyone happy to patch the Emacs-25 branch?

Can you summarize its effect?  It doesn't disable undo in recursive
editing, does it?

Thanks.

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Date: Mon, 06 Jun 2016 15:33:12 +0100
>> Cc: Chong Yidong <cyd@gnu.org>, 23632@debbugs.gnu.org
>> 
>> Assuming we go for 3, is everyone happy to patch the Emacs-25 branch?
>
> Can you summarize its effect?  It doesn't disable undo in recursive
> editing, does it?

No. It limits the addition of undo-boundaries to those buffers that have
changed in at the same level of recursion. So, for example, undo will
function in mini-buffer during completing-read, but changes in the
mini-buffer will not result in the addition of undo-boundaries to
buffers that changed during the command that lead to the
completing-read.

Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: monnier@iro.umontreal.ca,  cyd@gnu.org,  23632@debbugs.gnu.org
> Date: Mon, 06 Jun 2016 16:38:54 +0100
> 
> > Can you summarize its effect?  It doesn't disable undo in recursive
> > editing, does it?
> 
> No. It limits the addition of undo-boundaries to those buffers that have
> changed in at the same level of recursion. So, for example, undo will
> function in mini-buffer during completing-read, but changes in the
> mini-buffer will not result in the addition of undo-boundaries to
> buffers that changed during the command that lead to the
> completing-read.

I guess I'm still confused a bit, because now I'm not sure what
difference does the change make.  Does it record each change only
once, where previously we could record it several times?  If not, what
does the above-mentioned limitation achieve, in terms of user-visible
effects?

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: phillip.lord@russet.org.uk (Phillip Lord)
>> Cc: monnier@iro.umontreal.ca,  cyd@gnu.org,  23632@debbugs.gnu.org
>> Date: Mon, 06 Jun 2016 16:38:54 +0100
>> 
>> > Can you summarize its effect?  It doesn't disable undo in recursive
>> > editing, does it?
>> 
>> No. It limits the addition of undo-boundaries to those buffers that have
>> changed in at the same level of recursion. So, for example, undo will
>> function in mini-buffer during completing-read, but changes in the
>> mini-buffer will not result in the addition of undo-boundaries to
>> buffers that changed during the command that lead to the
>> completing-read.
>
> I guess I'm still confused a bit, because now I'm not sure what
> difference does the change make.  Does it record each change only
> once, where previously we could record it several times?  If not, what
> does the above-mentioned limitation achieve, in terms of user-visible
> effects?


Hopefully, in most cases no user-visible effects, but it fixes the
original bug report.

It happens like this:

call some command
command changes local buffer
command asks for more information from minibuffer with recursive edit
change in the minibuffer forces undo-boundary into local buffer

So, you get an undo-boundary where you did not expect.

With this change:

call some command
command changes local buffer
command asks for more information
changes in minibuffer force undo-boundary only in mini-buffer


Phil

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Eli Zaretskii]

> From: phillip.lord@russet.org.uk (Phillip Lord)
> Cc: monnier@iro.umontreal.ca,  cyd@gnu.org,  23632@debbugs.gnu.org
> Date: Tue, 07 Jun 2016 12:20:27 +0100
> 
> It happens like this:
> 
> call some command
> command changes local buffer
> command asks for more information from minibuffer with recursive edit
> change in the minibuffer forces undo-boundary into local buffer
> 
> So, you get an undo-boundary where you did not expect.
> 
> With this change:
> 
> call some command
> command changes local buffer
> command asks for more information
> changes in minibuffer force undo-boundary only in mini-buffer

OK, thanks for the explanations.  The changes are okay for the
emacs-25 branch.

bug#23632: 25.1.50; Gratuitous undo boundary in latex-insert-block

[Chong Yidong]

phillip.lord@russet.org.uk (Phillip Lord) writes:

> In and off itself, the patch seems fine, but my concern is that that
> the previous heuristic did the right thing, the new heuristic does
> not. If you've found three instances where it's causing a problem,
> then there will be others also.

You mentioned "the new heuristic" and "the old system", but to be clear,
this behavior is not a new one.  I can reproduce it in Emacs 24.3, and
it probably goes back farther than that.

Also, the solution needs to handle the case where the user switches from
the minibuffer back to the original buffer and does some editing,
including possible calling undo.  The undo boundaries thus created ought
to be preserved.  Only the undo boundary created specially for
completing-read ought to be automatically removed.