bug#37700: 27.0.50; undo mouse-drag-and-drop-region ineffective

["Mattias Engdegård" <mattiase@acm.org>]

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

28 okt. 2019 kl. 21.01 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> Indeed, that's also my experience [ and as a result I stay away from
> text's drag-and-drop since I find C-w ... C-y to be easier since it
> avoids this trial-and-error problem ;-) ]

Thanks for taking a look. It seems that the only happy users are those that use redo+.el or similar packages that do not restrict undo to region.

Unfortunately, even with the patch, undoing a drag-and-drop does not leave the region active the way it was before the undo, so the user has to reselect the text in order to try again. Reactivating the region automatically on undo is tempting but incompatible with undo-in-region (the same problem but in the other direction). Partly because of this, I believe that providing an option to disable undo-in-region altogether is a better solution.

> Rather than add a special new kind of entry you can use some version of
> (apply DELTA BEG END FUN-NAME . ARGS), presumably with DELTA=0 and
> BEG=END, so you don't need to modify the docstring of `buffer-undo-list`
> nor the implementation of primitive-undo.

I'm not sure how that would be done in practice since 'undo-elt-in-region' is nil for any (apply ...) element. This could be remedied, of course, but that would entail undo machinery changes which we wanted to avoid in the first place. In addition, it is unclear how the 'apply' mechanism could be used in a way that is sensitive to whether it's the first record to be undone.

> As a user of undo-in-region, I think I'd be surprised if my undo started
> to modify parts of the buffer outside the region, so I think a better
> approach would be to only pay attention to the `unconfined' marker when
> it appears at the top of the `buffer-undo-list` (i.e. only if the
> drag-and-drop was the very last operation).

The patch has now been modified to that effect. (I'm still not sure it's the right solution.)

>  Also I think it would
> deserve a message in the minibuffer explaining that the undo is not
> confined to the region.

It was tricky to do that cleanly, given the structure of the undo code, so it went into a separate patch.


[-- Attachment #2: 0001-Don-t-confine-undo-of-mouse-drag-and-drop-to-the-reg.patch --]
[-- Type: application/octet-stream, Size: 5001 bytes --]

From a857757e9f7d92b2acb9817399c9a4af49a4154c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 16 Oct 2019 15:32:22 +0200
Subject: [PATCH 1/2] Don't confine undo of mouse-drag-and-drop to the region
 (bug#37700)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Since the region is active after a mouse-drag-and-drop, the change
cannot immediately be undone.  To get around this, tell the undo
machinery to ignore the region confinement for this operation if
it is undone right away.

* lisp/simple.el (primitive-undo, undo-make-selective-list):
* src/buffer.c (buffer-undo-list):
New `buffer-undo-list’ element `unconfined'.
* lisp/mouse.el (mouse-drag-and-drop-region):
Mark the operation as unconfined upon undo.
---
 lisp/mouse.el  |  7 ++++++-
 lisp/simple.el | 13 +++++++++++--
 src/buffer.c   |  4 ++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lisp/mouse.el b/lisp/mouse.el
index 76fec507e7..533ad606fb 100644
--- a/lisp/mouse.el
+++ b/lisp/mouse.el
@@ -2657,7 +2657,12 @@ mouse-drag-and-drop-region
                 (let (deactivate-mark)
                   (dolist (overlay mouse-drag-and-drop-overlays)
                     (delete-region (overlay-start overlay)
-                                   (overlay-end overlay)))))
+                                   (overlay-end overlay))))
+                ;; Since we will leave the destination text selected,
+                ;; make sure an undo operation disregards the region
+                ;; or the operation will only be partially undone.
+                (when (consp buffer-undo-list)
+                  (push 'unconfined buffer-undo-list)))
             ;; When source buffer and destination buffer are different,
             ;; keep (set back the original text as region) or remove the
             ;; original text.
diff --git a/lisp/simple.el b/lisp/simple.el
index 29e195bca6..83be410074 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2753,6 +2753,7 @@ primitive-undo
              (set-marker marker
                          (- marker offset)
                          (marker-buffer marker))))
+          (unconfined)                  ; Ignore this directive here.
           (_ (error "Unrecognized entry in undo list %S" next))))
       (setq arg (1- arg)))
     ;; Make sure an apply entry produces at least one undo entry,
@@ -2855,9 +2856,15 @@ undo-make-selective-list
   (let ((ulist buffer-undo-list)
         ;; A list of position adjusted undo elements in the region.
         (selective-list (list nil))
+        (unconfined nil)   ; Whether to include whole record unconditionally.
         ;; A list of undo-deltas for out of region undo elements.
         undo-deltas
         undo-elt)
+    ;; The `unconfined' directive only applies to the very first record.
+    (pcase ulist
+      (`(nil unconfined . ,rest)
+       (setq unconfined t)
+       (setq ulist rest)))
     (while ulist
       (when undo-no-redo
         (while (gethash ulist undo-equiv-table)
@@ -2867,7 +2874,8 @@ undo-make-selective-list
        ((null undo-elt)
         ;; Don't put two nils together in the list
         (when (car selective-list)
-          (push nil selective-list)))
+          (push nil selective-list))
+        (setq unconfined nil))
        ((and (consp undo-elt) (eq (car undo-elt) t))
         ;; This is a "was unmodified" element.  Keep it
         ;; if we have kept everything thus far.
@@ -2877,10 +2885,11 @@ undo-make-selective-list
        ;; on finding them after (TEXT . POS) elements
        ((markerp (car-safe undo-elt))
         nil)
+       ((eq undo-elt 'unconfined))  ; This directive has no further effect.
        (t
         (let ((adjusted-undo-elt (undo-adjust-elt undo-elt
                                                   undo-deltas)))
-          (if (undo-elt-in-region adjusted-undo-elt start end)
+          (if (or (undo-elt-in-region adjusted-undo-elt start end) unconfined)
               (progn
                 (setq end (+ end (cdr (undo-delta adjusted-undo-elt))))
                 (push adjusted-undo-elt selective-list)
diff --git a/src/buffer.c b/src/buffer.c
index 80eaa971a3..16012a97a2 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -6128,6 +6128,10 @@ from (abs POSITION).  If POSITION is positive, point was at the front
 Entries with value nil mark undo boundaries.  The undo command treats
 the changes between two undo boundaries as a single step to be undone.
 
+An entry with the value `unconfined' disables selective undo until the
+next undo boundary even if the region is active.  It only has effect
+on the first record to be undone.
+
 If the value of the variable is t, undo information is not recorded.  */);
 
   DEFVAR_PER_BUFFER ("mark-active", &BVAR (current_buffer, mark_active), Qnil,
-- 
2.21.0 (Apple Git-122)


[-- Attachment #3: 0002-Adapt-undo-message-when-undoing-unconfined-actions-i.patch --]
[-- Type: application/octet-stream, Size: 4154 bytes --]

From 71bc4e8352c6662458d5d18f328bd955054a91cc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Wed, 30 Oct 2019 19:13:47 +0100
Subject: [PATCH 2/2] Adapt undo message when undoing unconfined actions in
 region

* lisp/simple.el (undo): Don't say "Undo in region" when the action
undone explicitly ignores region confinement (bug#37700).
(undo--first-record-unconfined): New.
(undo-make-selective-list): Use undo--first-record-unconfined.
---
 lisp/simple.el | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/lisp/simple.el b/lisp/simple.el
index 83be410074..e08c9b6a89 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2508,6 +2508,8 @@ undo
 	 (base-buffer (or (buffer-base-buffer) (current-buffer)))
 	 (recent-save (with-current-buffer base-buffer
 			(recent-auto-save-p)))
+         (unconfined nil)
+         (count (if (numberp arg) (prefix-numeric-value arg) 1))
 	 message)
     ;; If we get an error in undo-start,
     ;; the next command should not be a "consecutive undo".
@@ -2527,7 +2529,9 @@ undo
       (setq undo-in-region
 	    (or (region-active-p) (and arg (not (numberp arg)))))
       (if undo-in-region
-	  (undo-start (region-beginning) (region-end))
+          (progn
+            (setq unconfined (undo--first-record-unconfined buffer-undo-list))
+            (undo-start (region-beginning) (region-end)))
 	(undo-start))
       ;; get rid of initial undo boundary
       (undo-more 1))
@@ -2537,20 +2541,20 @@ undo
     ;; so, ask the user whether she wants to skip the redo/undo pair.
     (let ((equiv (gethash pending-undo-list undo-equiv-table)))
       (or (eq (selected-window) (minibuffer-window))
-	  (setq message (format "%s%s"
-                                (if (or undo-no-redo (not equiv))
-                                    "Undo" "Redo")
-                                (if undo-in-region " in region" ""))))
+	  (setq message (concat
+                         (if (or undo-no-redo (not equiv))
+                             "Undo" "Redo")
+                         (cond ((and unconfined (> count 1))
+                                " in region (except first action)")
+                               ((and undo-in-region (not unconfined))
+                                " in region")))))
       (when (and (consp equiv) undo-no-redo)
 	;; The equiv entry might point to another redo record if we have done
 	;; undo-redo-undo-redo-... so skip to the very last equiv.
 	(while (let ((next (gethash equiv undo-equiv-table)))
 		 (if next (setq equiv next))))
 	(setq pending-undo-list equiv)))
-    (undo-more
-     (if (numberp arg)
-	 (prefix-numeric-value arg)
-       1))
+    (undo-more count)
     ;; Record the fact that the just-generated undo records come from an
     ;; undo operation--that is, they are redo records.
     ;; In the ordinary case (not within a region), map the redo
@@ -2847,6 +2851,13 @@ undo-start
 ;; "ccaabad", as though the first "d" became detached from the
 ;; original "ddd" insertion.  This quirk is a FIXME.
 
+(defun undo--first-record-unconfined (undo-list)
+  "If the first record of UNDO-LIST is marked unconfined, return the argument
+without the leading `unconfined' directive; otherwise nil."
+  (pcase undo-list
+    (`(nil unconfined . ,rest)
+     rest)))
+
 (defun undo-make-selective-list (start end)
   "Return a list of undo elements for the region START to END.
 The elements come from `buffer-undo-list', but we keep only the
@@ -2861,10 +2872,10 @@ undo-make-selective-list
         undo-deltas
         undo-elt)
     ;; The `unconfined' directive only applies to the very first record.
-    (pcase ulist
-      (`(nil unconfined . ,rest)
-       (setq unconfined t)
-       (setq ulist rest)))
+    (let ((ul (undo--first-record-unconfined ulist)))
+      (when ul
+        (setq ulist ul)
+        (setq unconfined t)))
     (while ulist
       (when undo-no-redo
         (while (gethash ulist undo-equiv-table)
-- 
2.21.0 (Apple Git-122)