bug#49484: 27.2; [PATCH] Undoing a 'RET' in comint and eshell

bug#49484: 27.2; [PATCH] Undoing a 'RET' in comint and eshell

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

1) M-x shell
2) echo foo RET
3) C-/ to undo this 'RET'
   The buffer now contain the shell's prompt and "echo foo".
   However, the process mark is located at eob after "echo foo"
4) Type bar
   The buffer now contain the shell's prompt and "echo foobar"
5) RET
   Shell will output "bar: command not found", because the process mark
   is located before "bar" after "foo".

Similar behaviour can be observed with C-c SPC (comint-accumulate) and
with eshell.

My idea to solve this is to record process mark and related marker
positions as `apply' entries in the undo list. Attached patch implements
this for comint and eshell.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Improve-undo-in-comint-and-eshell.patch --]
[-- Type: text/x-patch, Size: 6567 bytes --]

From fde3b5ce8964e001a9019feff83e267b2cf367dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Fri, 9 Jul 2021 10:57:11 +0200
Subject: [PATCH] Improve undo in comint and eshell

* lisp/simple.el (marker-record-undo): New function.
* etc/NEWS:
* doc/lispref/markers.texi (Moving Markers): Document it.
* lisp/comint.el (comint-send-input):
(comint-accumulate):
(comint-set-process-mark):
* lisp/eshell/esh-mode.el (eshell-reset):
(eshell-update-markers): Use it to record adjustments to various
marker positions in undo list.
---
 doc/lispref/markers.texi | 17 +++++++++++++++++
 etc/NEWS                 |  5 +++++
 lisp/comint.el           | 14 ++++++++++----
 lisp/eshell/esh-mode.el  | 15 ++++++++++-----
 lisp/simple.el           | 18 ++++++++++++++++++
 5 files changed, 60 insertions(+), 9 deletions(-)

diff --git a/doc/lispref/markers.texi b/doc/lispref/markers.texi
index 80f79b67e5..b0c454be8d 100644
--- a/doc/lispref/markers.texi
+++ b/doc/lispref/markers.texi
@@ -395,6 +395,23 @@ Moving Markers
 
 @defun move-marker marker position &optional buffer
 This is another name for @code{set-marker}.
+@end defun
+
+  Function @code{set-marker} does not record marker movement in the
+undo list.  Before moving a marker, you can explicitly record its
+original position as an undo list entry with
+@code{marker-record-undo}.
+
+@defun marker-record-undo &rest markers
+This function records the current position and buffer of each marker
+in MARKERS as an entry in the undo list.  Undoing it will relocate
+these markers to point back to their recorded positions.  Passing
+markers that currently point nowhere is allowed and undoing will
+simply make them point nowhere again.
+
+Undo in region will always ignore entries made with this function.
+Also, this function doesn't do anything if undo is disabled in the
+current buffer.
 @end defun
 
 @node The Mark
diff --git a/etc/NEWS b/etc/NEWS
index da5524a555..2e0e7abc47 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2945,6 +2945,11 @@ The former is now declared obsolete.
 \f
 * Lisp Changes in Emacs 28.1
 
++++
+** New function 'marker-record-undo'.
+To make marker movement undoable, use this function to store a
+marker's current position in the undo list before moving the marker.
+
 ---
 *** ':safe' settings in 'defcustom' are now propagated to the loaddefs files.
 
diff --git a/lisp/comint.el b/lisp/comint.el
index 9e406614b9..f464ecbbe4 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -1931,9 +1931,12 @@ comint-send-input
         (setq comint-input-ring-index nil)
         ;; Update the markers before we send the input
         ;; in case we get output amidst sending the input.
+        (marker-record-undo
+         pmark comint-last-input-start comint-last-input-end
+         comint-accum-marker)
         (set-marker comint-last-input-start pmark)
         (set-marker comint-last-input-end (point))
-        (set-marker (process-mark proc) (point))
+        (set-marker pmark (point))
         ;; clear the "accumulation" marker
         (set-marker comint-accum-marker nil)
         (let ((comint-input-sender-no-newline no-newline))
@@ -3490,6 +3493,7 @@ comint-accumulate
 when you send it."
   (interactive)
   (insert "\n")
+  (marker-record-undo comint-accum-marker)
   (set-marker comint-accum-marker (point))
   (if comint-input-ring-index
       (setq comint-save-input-ring-index
@@ -3525,9 +3529,11 @@ comint-bol-or-process-mark
 (defun comint-set-process-mark ()
   "Set the process mark at point."
   (interactive)
-  (let ((proc (or (get-buffer-process (current-buffer))
-		  (user-error "Current buffer has no process"))))
-    (set-marker (process-mark proc) (point))
+  (let* ((proc (or (get-buffer-process (current-buffer))
+                   (user-error "Current buffer has no process")))
+         (pmark (process-mark proc)))
+    (marker-record-undo pmark)
+    (set-marker pmark (point))
     (message "Process mark set")))
 
 \f
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index f9dbce9770..9aa00016c0 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -534,11 +534,14 @@ eshell-reset
   "Output a prompt on a new line, aborting any current input.
 If NO-HOOKS is non-nil, then `eshell-post-command-hook' won't be run."
   (goto-char (point-max))
-  (setq eshell-last-input-start (point-marker)
-	eshell-last-input-end (point-marker)
-	eshell-last-output-start (point-marker)
-	eshell-last-output-block-begin (point)
-	eshell-last-output-end (point-marker))
+  (marker-record-undo
+   eshell-last-input-start eshell-last-input-end
+   eshell-last-output-start eshell-last-output-end)
+  (set-marker eshell-last-input-start (point))
+  (set-marker eshell-last-input-end (point))
+  (set-marker eshell-last-output-start (point))
+  (set-marker eshell-last-output-end (point))
+  (setq eshell-last-output-block-begin (point))
   (eshell-begin-on-new-line)
   (unless no-hooks
     (run-hooks 'eshell-post-command-hook)
@@ -568,6 +571,8 @@ eshell-parse-command-input
 
 (defun eshell-update-markers (pmark)
   "Update the input and output markers relative to point and PMARK."
+  (marker-record-undo eshell-last-input-start eshell-last-input-end
+                      eshell-last-output-end)
   (set-marker eshell-last-input-start pmark)
   (set-marker eshell-last-input-end (point))
   (set-marker eshell-last-output-end (point)))
diff --git a/lisp/simple.el b/lisp/simple.el
index f746d738a6..337cfe6234 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3466,6 +3466,24 @@ undo-adjust-pos
             ;; comments.
             (max (car d) (- pos (cdr d)))))))
 
+(defun marker-record-undo (&rest markers)
+  "Record positions of MARKERS in the undo list.
+Undoing this entry will make each marker in MARKERS point to its
+recorded position and buffer, or nowhere if it currently points
+nowhere.  Undo in region will always ignore these entries.
+
+If undo is disabled in the current buffer, this function does
+nothing."
+  (let ((undo-list buffer-undo-list))
+    (unless (eq undo-list t)
+      (dolist (marker markers)
+        (push (list 'apply #'set-marker marker
+                    (marker-position marker) (marker-buffer marker))
+              undo-list))
+      (setq buffer-undo-list
+            `((apply ,#'marker-record-undo ,@markers)
+              ,@undo-list)))))
+
 ;; Return the first affected buffer position and the delta for an undo element
 ;; delta is defined as the change in subsequent buffer positions if we *did*
 ;; the undo.
-- 
2.32.0

bug#49484: 27.2; [PATCH] Undoing a 'RET' in comint and eshell

[Lars Ingebrigtsen]

miha@kamnitnik.top writes:

> My idea to solve this is to record process mark and related marker
> positions as `apply' entries in the undo list. Attached patch implements
> this for comint and eshell.

Hm, interesting...  The patch looks good to me, but I'm not really that
familiar with undo internals myself, so it'd be good to get more
opinions on this first.  So I've added Stefan to the CCs.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#49484: 27.2; [PATCH] Undoing a 'RET' in comint and eshell

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 725 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> miha@kamnitnik.top writes:
>
>> My idea to solve this is to record process mark and related marker
>> positions as `apply' entries in the undo list. Attached patch implements
>> this for comint and eshell.
>
> Hm, interesting...  The patch looks good to me, but I'm not really that
> familiar with undo internals myself, so it'd be good to get more
> opinions on this first.  So I've added Stefan to the CCs.

So after thinking about this some more, I arrived at a simpler solution:
deleting and reinserting text to generate suitable undo list entries
instead of adding them explicitly.
As opposed to the first patch, this one should also handle
undo-in-region reasonably well.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Improve-undoing-of-RET-in-comint-and-eshell.patch --]
[-- Type: text/x-patch, Size: 3680 bytes --]

From ad98e21545e5d248e13ef8b124b42ca4f0215f6e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Fri, 16 Jul 2021 17:08:12 +0200
Subject: [PATCH] Improve undoing of RET in comint and eshell

* lisp/comint.el (comint-send-input):
(comint-accumulate):
* lisp/eshell/esh-mode.el (eshell-send-input): Before sending input to
the process, delete it and reinsert it again.  Undoing this
insertion with 'C-/' will delete the region, moving the process mark
back to its original position.
---
 lisp/comint.el          | 24 +++++++++++++++++++++++-
 lisp/eshell/esh-mode.el |  8 ++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 9e406614b9..f24ec4b6bf 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -1890,6 +1890,14 @@ comint-send-input
                           (delete-region pmark start)
                           copy))))
 
+        ;; Delete and reinsert input.  This seems like a no-op, except
+        ;; for the resulting entries in the undo list: undoing this
+        ;; insertion will delete the region, moving the process mark
+        ;; back to its original position.
+        (let ((inhibit-read-only t))
+          (delete-region pmark (point))
+          (insert input))
+
         (unless no-newline
           (insert ?\n))
 
@@ -1933,7 +1941,7 @@ comint-send-input
         ;; in case we get output amidst sending the input.
         (set-marker comint-last-input-start pmark)
         (set-marker comint-last-input-end (point))
-        (set-marker (process-mark proc) (point))
+        (set-marker pmark (point))
         ;; clear the "accumulation" marker
         (set-marker comint-accum-marker nil)
         (let ((comint-input-sender-no-newline no-newline))
@@ -3489,6 +3497,20 @@ comint-accumulate
 The entire accumulated text becomes one item in the input history
 when you send it."
   (interactive)
+  (when-let* ((proc (get-buffer-process (current-buffer)))
+              (pmark (process-mark proc))
+              ((or (marker-position comint-accum-marker)
+                   (set-marker comint-accum-marker pmark)
+                   t))
+              ((>= (point) comint-accum-marker pmark)))
+    ;; Delete and reinsert input.  This seems like a no-op, except for
+    ;; the resulting entries in the undo list: undoing this insertion
+    ;; will delete the region, moving the accumulation marker back to
+    ;; its original position.
+    (let ((text (buffer-substring comint-accum-marker (point)))
+          (inhibit-read-only t))
+      (delete-region comint-accum-marker (point))
+      (insert text)))
   (insert "\n")
   (set-marker comint-accum-marker (point))
   (if comint-input-ring-index
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index f9dbce9770..92e1e9eb6a 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -614,6 +614,14 @@ eshell-send-input
 		  (and eshell-send-direct-to-subprocesses
 		       proc-running-p))
 	(insert-before-markers-and-inherit ?\n))
+      ;; Delete and reinsert input.  This seems like a no-op, except
+      ;; for the resulting entries in the undo list: undoing this
+      ;; insertion will delete the region, moving the process mark
+      ;; back to its original position.
+      (let ((text (buffer-substring eshell-last-output-end (point)))
+            (inhibit-read-only t))
+        (delete-region eshell-last-output-end (point))
+        (insert text))
       (if proc-running-p
 	  (progn
 	    (eshell-update-markers eshell-last-output-end)
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

bug#49484: 27.2; [PATCH] Undoing a 'RET' in comint and eshell

[Lars Ingebrigtsen]

<miha@kamnitnik.top> writes:

> So after thinking about this some more, I arrived at a simpler solution:
> deleting and reinserting text to generate suitable undo list entries
> instead of adding them explicitly.
> As opposed to the first patch, this one should also handle
> undo-in-region reasonably well.

Thanks, I've now finally tested this, and it seems to work perfectly, so
I've now pushed it to Emacs 29.  Sorry for the delay.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no