Subject: bug#21609: bug#24837: 26.0.50; term.el: In char mode, displayed and executed commands can differ

From: Phil Sainty <[email-masked]>
Date: Mon, 25 Sep 2017 13:48:57 +1300

On 24/09/17 23:59, Philipp Stephani wrote:
> I haven't checked the code in detail, but it seems to be reasonable.
> Since nobody complained in weeks, maybe just push it to the release
> branch and see whether it breaks anything.

Before we do that, here's a revised patch based on my own follow-ups
to my initial patch.  Changes to that version are as follows:

I've switched the `buffer-read-only' let-binding to `inhibit-read-only',
as you had originally suggested.

It looks like programatically calling `read-only-mode' was incorrect
on my part, so I'm now setting `buffer-read-only' instead.  I've added
a `read-only-mode-hook' function to track when the user toggles the
read-only state, however, so that if the user happens to enable
`read-only-mode' in line mode, toggling to and from char mode will not
revert their selected read-only state.

Lastly I'm trying to avoid messing with mouse selection in char mode,
by *not* moving point back to the process mark if the last command
input event was a mouse event.

So keyboard events cannot leave point in the wrong position; and while
mouse events can do so, as soon as the keyboard is involved again we
jump to where we're supposed to be.

This still happens in post-command-hook only.

Question: Is there a reason to additionally do this in pre-command-hook?
I initially thought I'd need to do so, due to it now being possible for
a (mouse) command to leave point in the wrong position; however I think
it's probably still fine to limit this to post-command-hook?

IIRC the main danger of leaving point in the wrong position is mitigated
by the buffer being read-only, which ensures that the user cannot make
inconsistent changes to the buffer state in char mode; and I'm assuming
that inferior process output is guaranteed to happen at the process mark.

(The user could invoke a command which uses inhibit-read-only to
update the buffer, but that's going to introduce inconsistencies no
matter where point is at the time, so I think that's out of scope.)

The user might also wish to invoke a keyboard command that acts upon
the mouse-selected region, so limiting ourselves to post-command-hook
means we won't interfere with such a command.

New patch is attached.

-Phil
>From 2476e0a6aed6ba9760651df707b814046ffe45ca Mon Sep 17 00:00:00 2001
From: Phil Sainty <[email-masked]>
Date: Sun, 3 Sep 2017 14:30:18 +1200
Subject: [PATCH] Avoid creating inconsistent buffer states in term-char-mode
 (bug#24837)

* lisp/term.el (term-mode, term-char-mode, term-line-mode)
(term-emulate-terminal): Make buffer read-only in `term-char-mode',
except for the process filter's output.  Use `read-only-mode-hook' to
track and restore the user-set state of `buffer-read-only' for
`term-line-mode'.

term-line-mode-buffer-read-only: New variable.
(term-line-mode-buffer-read-only-update): New function.

(term-char-mode, term-line-mode): Use `term-goto-process-mark-maybe'
in post-command-hook to counter-act unexpected changes to point.

(term-goto-process-mark-maybe): New function.
(term-goto-process-mark): New function.
---
 lisp/term.el | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/lisp/term.el b/lisp/term.el
index c748c45..31c78af 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -427,6 +427,8 @@ term-old-mode-map
 (defvar term-old-mode-line-format) ; Saves old mode-line-format while paging.
 (defvar term-pager-old-local-map nil "Saves old keymap while paging.")
 (defvar term-pager-old-filter) ; Saved process-filter while paging.
+(defvar-local term-line-mode-buffer-read-only nil
+  "The `buffer-read-only' state to set in `term-line-mode'.")
 
 (defcustom explicit-shell-file-name nil
   "If non-nil, is file name to use for explicitly requested inferior shell."
@@ -1105,6 +1107,8 @@ term-mode
                     (term-reset-size (cdr size) (car size)))
                   size))
 
+  (add-hook 'read-only-mode-hook #'term-line-mode-buffer-read-only-update nil t)
+
   (easy-menu-add term-terminal-menu)
   (easy-menu-add term-signals-menu)
   (or term-input-ring
@@ -1246,6 +1250,11 @@ term-char-mode
     (easy-menu-add term-terminal-menu)
     (easy-menu-add term-signals-menu)
 
+    ;; Don't allow changes to the buffer or to point which are not
+    ;; caused by the process filter.
+    (setq buffer-read-only 1)
+    (add-hook 'post-command-hook #'term-goto-process-mark-maybe nil t)
+
     ;; Send existing partial line to inferior (without newline).
     (let ((pmark (process-mark (get-buffer-process (current-buffer))))
 	  (save-input-sender term-input-sender))
@@ -1265,9 +1274,16 @@ term-line-mode
 you type \\[term-send-input] which sends the current line to the inferior."
   (interactive)
   (when (term-in-char-mode)
+    (setq buffer-read-only term-line-mode-buffer-read-only)
+    (remove-hook 'post-command-hook #'term-goto-process-mark-maybe t)
     (use-local-map term-old-mode-map)
     (term-update-mode-line)))
 
+(defun term-line-mode-buffer-read-only-update ()
+  "Update the user-set state of `buffer-read-only' in `term-line-mode'."
+  (when (term-in-line-mode)
+    (setq term-line-mode-buffer-read-only buffer-read-only)))
+
 (defun term-update-mode-line ()
   (let ((term-mode
          (if (term-in-char-mode)
@@ -2711,6 +2727,7 @@ term-emulate-terminal
 	   count-bytes ; number of bytes
 	   decoded-substring
 	   save-point save-marker old-point temp win
+	   (inhibit-read-only t)
 	   (buffer-undo-list t)
 	   (selected (selected-window))
 	   last-win
@@ -3109,6 +3126,21 @@ term-emulate-terminal
     (when (get-buffer-window (current-buffer))
       (redisplay))))
 
+(defun term-goto-process-mark-maybe ()
+  "Move point to the term buffer's process-mark upon keyboard input.
+
+Used as a buffer-local `post-command-hook' function in
+`term-char-mode' to prevent commands from putting the buffer into
+an inconsistent state by unexpectedly moving point.
+
+Mouse events are ignored so that mouse selection is unimpeded."
+  (unless (mouse-event-p last-command-event)
+    (term-goto-process-mark)))
+
+(defun term-goto-process-mark ()
+  "Move point to the current process-mark for the term buffer process."
+  (goto-char (process-mark (get-buffer-process (current-buffer)))))
+
 (defun term-handle-deferred-scroll ()
   (let ((count (- (term-current-row) term-height)))
     (when (>= count 0)
-- 
2.8.3