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

From: [email-masked] (Phillip Lord)
Date: Fri, 03 Jun 2016 17:13:19 +0100

Stefan Monnier <[email-masked]> 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.

>From 018429dafe1395e0927934fd95d65b74c69b2d07 Mon Sep 17 00:00:00 2001
From: Phillip Lord <[email-masked]>
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