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

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

[Philipp Stephani]

emacs -Q
M-x term RET RET
Make sure the terminal is in char mode.
Enter foo-bar C-<backspace> RET

The string "foo-" is now shown in the term buffer, but "foo-bar" has
been sent to the shell.  Typical output is "foo-bar: command not
found".
This is dangerous because a different command is executed than the one
that is visible in the buffer.
Either Emacs should make sure that after C-<backspace> the same command
that is displayed is sent to the shell, or it should disable
C-<backspace> in char mode altogether.


In GNU Emacs 26.0.50.14 (x86_64-unknown-linux-gnu, GTK+ Version 3.10.8)
 of 2016-10-31 built on localhost
Repository revision: 8e7b1af1d708dcf41695cf3fbeff9d35cdb8e5b6
Windowing system distributor 'The X.Org Foundation', version 11.0.11501000
System Description:	Ubuntu 14.04 LTS

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --with-modules --enable-checking
 --enable-check-lisp-object-type 'CFLAGS=-ggdb3 -O0''

Configured features:
XPM JPEG TIFF GIF PNG SOUND GSETTINGS NOTIFY GNUTLS FREETYPE XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Term

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message subr-x puny seq byte-opt gv
bytecomp byte-compile cl-extra help-mode cconv cl-loaddefs pcase cl-lib
dired dired-loaddefs format-spec rfc822 mml mml-sec password-cache epa
derived epg epg-config gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils term
disp-table easymenu ehelp ring time-date mule-util tooltip eldoc
electric uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win
x-win term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list newcomment elisp-mode lisp-mode prog-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese charscript case-table epa-hook
jka-cmpr-hook help simple abbrev obarray minibuffer cl-preloaded nadvice
loaddefs button faces cus-face macroexp files text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget
hashtable-print-readable backquote inotify dynamic-setting
system-font-setting font-render-setting move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 101998 8071)
 (symbols 48 20928 0)
 (miscs 40 339 145)
 (strings 32 19121 4563)
 (string-bytes 1 622106)
 (vectors 16 15237)
 (vector-slots 8 465841 4221)
 (floats 8 185 39)
 (intervals 56 269 0)
 (buffers 976 13)
 (heap 1024 47494 1043))

-- 
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle

Diese E-Mail ist vertraulich.  Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und löschen
Sie die E-Mail und alle Anhänge.  Vielen Dank.

This e-mail is confidential.  If you are not the right addressee please do not
forward it, please inform the sender, and please erase this e-mail including
any attachments.  Thanks.

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

[Phil Sainty]

On 2016-11-01 03:10, Philipp Stephani wrote:
> This is dangerous because a different command is executed than the one
> that is visible in the buffer.
> Either Emacs should make sure that after C-<backspace> the same command
> that is displayed is sent to the shell, or it should disable
> C-<backspace> in char mode altogether.


This is a duplicate of bug #21609 -- any command which directly
modifies the state of the terminal buffer can cause the apparent
state to be out of sync with the 'actual' state (i.e. the state
according to the inferior process).

In my own config I use the following, and something along these
lines might form a useful solution? It would be convenient if
there was a symbol property to identify standard commands which
should be disabled, but that may well be excessive/unworkable
in practice.

For killing and yanking text, many cases might be accounted for
by detecting the issue within `kill-region' and `insert-for-yank'
respectively. That isn't comprehensive (at minimum rectangles are
different), but might still be worth considering.


(eval-after-load "term"
   '(progn
      ;; Fix forward/backward word when (term-in-char-mode).
      (define-key term-raw-map (kbd "<C-left>")
        (lambda () (interactive) (term-send-raw-string "\eb")))
      (define-key term-raw-map (kbd "<M-left>")
        (lambda () (interactive) (term-send-raw-string "\eb")))
      (define-key term-raw-map (kbd "<C-right>")
        (lambda () (interactive) (term-send-raw-string "\ef")))
      (define-key term-raw-map (kbd "<M-right>")
        (lambda () (interactive) (term-send-raw-string "\ef")))
      ;; Disable killing and yanking in char mode (term-raw-map).
      (mapc
       (lambda (func)
         (eval `(define-key term-raw-map [remap ,func]
                  (lambda () (interactive) (ding)))))
       '(backward-kill-paragraph
         backward-kill-sentence backward-kill-sexp backward-kill-word
         bookmark-kill-line kill-backward-chars kill-backward-up-list
         kill-forward-chars kill-line kill-paragraph kill-rectangle
         kill-region kill-sentence kill-sexp kill-visual-line
         kill-whole-line kill-word subword-backward-kill subword-kill
         yank yank-pop yank-rectangle))))

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

[Philipp Stephani]

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

Phil Sainty <psainty@orcon.net.nz> schrieb am Mo., 31. Okt. 2016 um
21:46 Uhr:

> On 2016-11-01 03:10, Philipp Stephani wrote:
> > This is dangerous because a different command is executed than the one
> > that is visible in the buffer.
> > Either Emacs should make sure that after C-<backspace> the same command
> > that is displayed is sent to the shell, or it should disable
> > C-<backspace> in char mode altogether.
>
>
> This is a duplicate of bug #21609 -- any command which directly
> modifies the state of the terminal buffer can cause the apparent
> state to be out of sync with the 'actual' state (i.e. the state
> according to the inferior process).
>

Should maybe terminal buffers in char-mode be read-only? The process filter
could then use inhibit-read-only.

[-- Attachment #2: Type: text/html, Size: 1335 bytes --]

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

[Phil Sainty]

On 24/11/16 08:44, Philipp Stephani wrote:
> Phil Sainty <psainty@orcon.net.nz> schrieb am Mo., 31. Okt. 2016 um
>> This is a duplicate of bug #21609 -- any command which directly
>> modifies the state of the terminal buffer can cause the apparent
>> state to be out of sync with the 'actual' state (i.e. the state
>> according to the inferior process).
> 
> Should maybe terminal buffers in char-mode be read-only? The process
> filter could then use inhibit-read-only.

That's an interesting thought, and may be worth investigating (offhand
I've no idea whether it's workable), but note that it's not sufficient
to deal with all cases -- any Emacs command which moves point can create
an inconsistent state without modifying the buffer contents.

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

[Philipp Stephani]

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

Phil Sainty <psainty@orcon.net.nz> schrieb am Mi., 23. Nov. 2016 um
21:09 Uhr:

> On 24/11/16 08:44, Philipp Stephani wrote:
> > Phil Sainty <psainty@orcon.net.nz> schrieb am Mo., 31. Okt. 2016 um
> >> This is a duplicate of bug #21609 -- any command which directly
> >> modifies the state of the terminal buffer can cause the apparent
> >> state to be out of sync with the 'actual' state (i.e. the state
> >> according to the inferior process).
> >
> > Should maybe terminal buffers in char-mode be read-only? The process
> > filter could then use inhibit-read-only.
>
> That's an interesting thought, and may be worth investigating (offhand
> I've no idea whether it's workable), but note that it's not sufficient
> to deal with all cases -- any Emacs command which moves point can create
> an inconsistent state without modifying the buffer contents.
>
>
Hmm, then maybe the entire buffer also needs to be made intangible, except
for the actual position of the terminal cursor?

[-- Attachment #2: Type: text/html, Size: 1712 bytes --]

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

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 23 Nov 2016 20:21:56 +0000
> Cc: 24837@debbugs.gnu.org, 21609@debbugs.gnu.org
> 
> Phil Sainty <psainty@orcon.net.nz> schrieb am Mi., 23. Nov. 2016 um 21:09 Uhr:
> 
>  On 24/11/16 08:44, Philipp Stephani wrote:
>  > Phil Sainty <psainty@orcon.net.nz> schrieb am Mo., 31. Okt. 2016 um
>  >> This is a duplicate of bug #21609 -- any command which directly
>  >> modifies the state of the terminal buffer can cause the apparent
>  >> state to be out of sync with the 'actual' state (i.e. the state
>  >> according to the inferior process).
>  >
>  > Should maybe terminal buffers in char-mode be read-only? The process
>  > filter could then use inhibit-read-only.
> 
>  That's an interesting thought, and may be worth investigating (offhand
>  I've no idea whether it's workable), but note that it's not sufficient
>  to deal with all cases -- any Emacs command which moves point can create
>  an inconsistent state without modifying the buffer contents.
> 
> Hmm, then maybe the entire buffer also needs to be made intangible, except for the actual position of the
> terminal cursor? 

This bug is currently one of those marked to block the release of
Emacs 26.1.  Given that it existed for quite some time, I tend to
remove the blocking status, but if someone has practical ideas how to
fix this, I think we should do that now.

Thanks.

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

[Phil Sainty]

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

On 03/09/17 02:14, Eli Zaretskii wrote:
> This bug is currently one of those marked to block the release of
> Emacs 26.1.  Given that it existed for quite some time, I tend to
> remove the blocking status, but if someone has practical ideas how
> to fix this, I think we should do that now.

Well here's a starter for discussion.

I've performed only cursory testing, but at first glance this seems
to do the trick, so I'll see what other people think...

Firstly I'm using Philipp Stephani's suggestion that the buffer be
read-only in `term-char-mode' (and removing that in `term-line-mode';
this code doesn't attempt to remember the pre-existing states if
the user had changed it manually).  The default term process filter
`term-emulate-terminal' then binds `buffer-read-only' to nil.

Secondly, I've added a local `post-command-hook' function in
`term-char-mode' which simply moves point back to the local process
mark position.

Might such a simple approach be usable?  I'm not very familiar with
the code, so maybe there are glaring holes that I'm not seeing.


-Phil

[-- Attachment #2: 0001-Avoid-creating-inconsistent-buffer-states-in-term-ch.patch --]
[-- Type: text/x-patch, Size: 2354 bytes --]

From edeb0ae7ae6fefe15f277029792617da030c5a9b Mon Sep 17 00:00:00 2001
From: Phil Sainty <psainty@orcon.net.nz>
Date: Sun, 3 Sep 2017 14:30:18 +1200
Subject: [PATCH] Avoid creating inconsistent buffer states in
 `term-char-mode'.

* lisp/term.el (term-char-mode, term-line-mode, term-emulate-terminal):
Make buffer read-only in `term-char-mode' except for the process filter,
and use post-command-hook function `term-goto-process-mark' to avoid
unexpected changes to point.

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

diff --git a/lisp/term.el b/lisp/term.el
index 12a37ca..3ba6ee7 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -1246,6 +1246,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.
+    (read-only-mode 1)
+    (add-hook 'post-command-hook #'term-goto-process-mark 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,6 +1270,8 @@ term-line-mode
 you type \\[term-send-input] which sends the current line to the inferior."
   (interactive)
   (when (term-in-char-mode)
+    (read-only-mode 0)
+    (remove-hook 'post-command-hook #'term-goto-process-mark t)
     (use-local-map term-old-mode-map)
     (term-update-mode-line)))
 
@@ -2711,6 +2718,7 @@ term-emulate-terminal
 	   count-bytes ; number of bytes
 	   decoded-substring
 	   save-point save-marker old-point temp win
+	   buffer-read-only
 	   (buffer-undo-list t)
 	   (selected (selected-window))
 	   last-win
@@ -3109,6 +3117,14 @@ term-emulate-terminal
     (when (get-buffer-window (current-buffer))
       (redisplay))))
 
+(defun term-goto-process-mark ()
+  "Move point to the current process-mark for the term buffer process.
+
+Used as a buffer-local `post-command-hook' in `term-char-mode' to
+prevent commands from putting the buffer into an inconsistent
+state by unexpectedly moving point."
+  (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

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

[Phil Sainty]

On 03/09/17 14:58, Phil Sainty wrote:
> Firstly I'm using Philipp Stephani's suggestion that the buffer be
> read-only in `term-char-mode' [...] The default term process filter
> `term-emulate-terminal' then binds `buffer-read-only' to nil.

In fact Philipp actually suggested binding `inhibit-read-only' in the
process filter, which is presumably preferable.

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

[Phil Sainty]

On 03/09/17 14:58, Phil Sainty wrote:
> Secondly, I've added a local `post-command-hook' function in
> `term-char-mode' which simply moves point back to the local process
> mark position.
> 
> Might such a simple approach be usable?  I'm not very familiar with
> the code, so maybe there are glaring holes that I'm not seeing.

I've realised that one such glaring hole is mouse input, as clicking
then tends to create a selection between where you click and (forcibly)
the mark position at (most likely) the end of the buffer.

I'm not sure whether that's a deal breaker, or something which is
essentially incompatible with any solution to the problem and should
perhaps be disabled when in term-char-mode?

Inhibiting mouse events (or similar) sounds a little bit drastic;
however if unimpeded mouse selection is possible, and this allows the
user a way to move point from the process mark, then that just seems
contradictory to what we're trying to achieve in the first place,
which is to keep the state of the buffer (including point) consistent
with what the terminal process believes it to be.

We could automatically switch to term-line-mode upon mouse clicks,
but offhand I don't see how we could switch back automatically
without simply triggering the initial problem, and requiring the
user to manually switch back doesn't seem so user-friendly.


-Phil

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

[Phil Sainty]

On 04/09/17 21:55, Phil Sainty wrote:
> We could automatically switch to term-line-mode upon mouse clicks,
> but offhand I don't see how we could switch back automatically
> without simply triggering the initial problem
Maybe switching to line mode and temporarily shifting the post-command
hook to pre-command, such that nothing happens post-command for the
mouse events, but afterwards, at pre-command, any keyboard events
first trigger a move to the process mark (at which point we could
shift the behaviour back to post-command again).

(All speculation; I'm not sure if that would actually work.)

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

[Phil Sainty]

On 04/09/17 21:55, Phil Sainty wrote:
> We could automatically switch to term-line-mode upon mouse clicks,

Actually, that's probably a non-starter -- if you paste via the mouse
whilst in char mode, it is reasonable (and perhaps necessary) that the
paste will execute in char mode.

However the simpler approach of using the following for both pre-command
and post-command hooks whilst in char mode doesn't seem awful:

  (unless (mouse-event-p last-command-event)
    (term-goto-process-mark))

Mouse events aren't inhibited, but as soon as the keyboard is involved
we jump to where we're supposed to be.

Potentially that's a reasonable compromise.


-Phil

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

[Philipp Stephani]

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

Phil Sainty <psainty@orcon.net.nz> schrieb am So., 3. Sep. 2017 um
04:58 Uhr:

> On 03/09/17 02:14, Eli Zaretskii wrote:
> > This bug is currently one of those marked to block the release of
> > Emacs 26.1.  Given that it existed for quite some time, I tend to
> > remove the blocking status, but if someone has practical ideas how
> > to fix this, I think we should do that now.
>
> Well here's a starter for discussion.
>
> I've performed only cursory testing, but at first glance this seems
> to do the trick, so I'll see what other people think...
>
> Firstly I'm using Philipp Stephani's suggestion that the buffer be
> read-only in `term-char-mode' (and removing that in `term-line-mode';
> this code doesn't attempt to remember the pre-existing states if
> the user had changed it manually).  The default term process filter
> `term-emulate-terminal' then binds `buffer-read-only' to nil.
>
> Secondly, I've added a local `post-command-hook' function in
> `term-char-mode' which simply moves point back to the local process
> mark position.
>
> Might such a simple approach be usable?  I'm not very familiar with
> the code, so maybe there are glaring holes that I'm not seeing.
>
>
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.

[-- Attachment #2: Type: text/html, Size: 1799 bytes --]

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

[Phil Sainty]

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

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

[-- Attachment #2: 0001-Avoid-creating-inconsistent-buffer-states-in-term-ch.patch --]
[-- Type: text/x-patch, Size: 4129 bytes --]

From 2476e0a6aed6ba9760651df707b814046ffe45ca Mon Sep 17 00:00:00 2001
From: Phil Sainty <psainty@orcon.net.nz>
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

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

[Phil Sainty]

On 2017-09-25 13:48, Phil Sainty wrote:
> 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.

Thinking on this further, it might be even better to use 
pre-command-hook
to establish whether the pre-command position of point is equal to the
process mark, and then act conditionally on that in post-command-hook,
so that if the pre-command point did not already match the process mark,
we do *not* forcibly move it to the process mark afterwards.

The intention being that in term-char-mode:

1. Unless mouse activity takes place, a command cannot leave point in
    an inconsistent position.

2. The user *can* still use the mouse in char mode (e.g. to move point
    and/or select a region).

3. Once the user has used the mouse to move point, they may continue
    to invoke arbitrary commands without it being forced back to the
    process mark (so exchange-point-and-mark would do what they expected,
    for example).  The buffer will still be read-only of course.

4. Upon process output, point would move back to the process mark as
    normal.  When this happens (or if the user manually moves point to
    that position) the user would need to use the mouse once more to
    move point elsewhere.

Obviously they can switch to term-line-mode at any time and do anything
to the buffer, but the above would give some ability to the mouse and
certain keybindings in char mode as well.


-Phil

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

[Eli Zaretskii]

> Date: Mon, 25 Sep 2017 16:09:22 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 24837@debbugs.gnu.org, 21609@debbugs.gnu.org, bug-gnu-emacs
>  <bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org>
> 
> On 2017-09-25 13:48, Phil Sainty wrote:
> > 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.
> 
> Thinking on this further, it might be even better to use 
> pre-command-hook
> to establish whether the pre-command position of point is equal to the
> process mark, and then act conditionally on that in post-command-hook,
> so that if the pre-command point did not already match the process mark,
> we do *not* forcibly move it to the process mark afterwards.

Do you have a patch along these lines?

Thanks.

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

[Phil Sainty]

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

On 29/09/17 21:37, Eli Zaretskii wrote:
>> On 2017-09-25 13:48, Phil Sainty wrote:
>> Thinking on this further, it might be even better to use 
>> pre-command-hook
>> to establish whether the pre-command position of point is equal to the
>> process mark, and then act conditionally on that in post-command-hook,
>> so that if the pre-command point did not already match the process mark,
>> we do *not* forcibly move it to the process mark afterwards.
> 
> Do you have a patch along these lines?

I've found a little time to work on this some more.

New patch attached, with the aforementioned changes.

I think the remaining task would be to add user options to disable
the new behaviours, as some users might object to them?


-Phil

[-- Attachment #2: 0001-Avoid-creating-inconsistent-buffer-states-in-term-ch.patch --]
[-- Type: text/x-patch, Size: 5114 bytes --]

From 01ebb45b5e6bc22a5db55343bb6084d9a1800f2e Mon Sep 17 00:00:00 2001
From: Phil Sainty <psainty@orcon.net.nz>
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-set-goto-process-mark'
in pre-command-hook, and `term-goto-process-mark-maybe' in
post-command-hook to counter-act unexpected changes to point.

term-goto-process-mark: New buffer-local variable.
(term-set-goto-process-mark): New function.
(term-goto-process-mark-maybe): New function.
(term-process-mark): New function.
---
 lisp/term.el | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/lisp/term.el b/lisp/term.el
index c748c45..32dcb5a 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,12 @@ 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 'pre-command-hook #'term-set-goto-process-mark nil t)
+    (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 +1275,17 @@ 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 'pre-command-hook #'term-set-goto-process-mark t)
+    (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 +2729,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 +3128,39 @@ term-emulate-terminal
     (when (get-buffer-window (current-buffer))
       (redisplay))))
 
+(defvar-local term-goto-process-mark t
+  "Whether to reset point to the current process mark after each command.
+
+Set in `pre-command-hook' by `term-set-goto-process-mark'.")
+
+(defun term-set-goto-process-mark ()
+  "Sets `term-goto-process-mark'.  Called during `pre-command-hook'.
+
+Ensures that when point is equal to the process mark at the
+pre-command stage, it will be returned to the process mark at the
+post-command stage.  See also `term-goto-process-mark-maybe'."
+  (setq term-goto-process-mark
+        (eq (point) (marker-position (term-process-mark)))))
+
+(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.
+
+Only acts when the pre-command position of point was equal to the
+current process mark.
+
+Mouse events are ignored so that mouse selection is unimpeded."
+  (unless (mouse-event-p last-command-event)
+    (when term-goto-process-mark
+      (goto-char (term-process-mark)))))
+
+(defun term-process-mark ()
+  "The current process-mark for the term buffer process."
+  (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

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

[Phil Sainty]

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

On 09/10/17 01:39, Phil Sainty wrote:
> I think the remaining task would be to add user options to disable
> the new behaviours, as some users might object to them?

This is now done, and rebased onto the emacs-26 branch.  The new user
options are enabled by default, and a NEWS entry added.  New patch is
attached; please review.


-Phil

[-- Attachment #2: 0001-Avoid-creating-inconsistent-buffer-states-in-term-ch.patch --]
[-- Type: text/x-patch, Size: 7949 bytes --]

From ecd56a565bd4260bcd88e63b0f99cd7eeec71714 Mon Sep 17 00:00:00 2001
From: Phil Sainty <psainty@orcon.net.nz>
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-char-mode-buffer-read-only: New user option.
term-line-mode-buffer-read-only: New buffer-local variable.
(term-line-mode-buffer-read-only-update): New function.

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

term-char-mode-point-at-process-mark: New user option.
term-goto-process-mark: New buffer-local variable.
(term-set-goto-process-mark): New function.
(term-goto-process-mark-maybe): New function.
(term-process-mark): New function.

* etc/NEWS: Mention the new user options.
---
 etc/NEWS     | 12 ++++++++
 lisp/term.el | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index a9a4bc6..2149829 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1148,6 +1148,18 @@ languages.  (Version 2.1.0 or later of Enchant is required.)
 +++
 *** Emacs no longer prompts the user before killing Flymake processes on exit.
 
+** Term
+
++++
+*** New user options `term-char-mode-buffer-read-only' and
+`term-char-mode-point-at-process-mark' are enabled by default to avoid
+buffer states being created in `term-char-mode' which are inconsistent
+with the terminal state expected by the inferior process.
+
+Respectively, these options prevent buffer changes which are not
+caused by the process filter, and counter-act movements of point away
+from the process mark (with the exception of mouse events).
+
 \f
 * New Modes and Packages in Emacs 26.1
 
diff --git a/lisp/term.el b/lisp/term.el
index c748c45..ffd0e92 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."
@@ -487,6 +489,36 @@ term-input-ring-file-name
   :type 'boolean
   :group 'term)
 
+(defcustom term-char-mode-buffer-read-only t
+  "If non-nil, only the process filter may modify the buffer in char mode.
+
+This makes the buffer read-only in char mode (except for the
+process filter), to prevent editing commands from causing a
+buffer state which is inconsistent with the state understood by
+the inferior process."
+  :type 'boolean
+  :group 'term)
+
+(defcustom term-char-mode-point-at-process-mark t
+  "If non-nil, keep point at the process mark in char mode.
+
+This prevents commands that move point from causing a buffer
+state which is inconsistent with the state understood by the
+inferior process.
+
+Takes effect during `post-command-hook', provided that the
+pre-command point position was also at the process mark.
+
+Mouse selection and other mouse events are ignored, so moving
+point is still possible in char mode via the mouse, and other
+commands can then be invoked at the mouse-selected point or
+region, until the process filter (or user) moves point to the
+process mark once again.
+
+This option has no effect in line mode."
+  :type 'boolean
+  :group 'term)
+
 (defcustom term-scroll-to-bottom-on-output nil
   "Controls whether interpreter output causes window to scroll.
 If nil, then do not scroll.  If t or `all', scroll all windows showing buffer.
@@ -1105,6 +1137,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 +1280,13 @@ 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.
+    (when term-char-mode-buffer-read-only
+      (setq buffer-read-only t))
+    (add-hook 'pre-command-hook #'term-set-goto-process-mark nil t)
+    (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 +1306,20 @@ term-line-mode
 you type \\[term-send-input] which sends the current line to the inferior."
   (interactive)
   (when (term-in-char-mode)
+    (when term-char-mode-buffer-read-only
+      (setq buffer-read-only term-line-mode-buffer-read-only))
+    (remove-hook 'pre-command-hook #'term-set-goto-process-mark t)
+    (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'.
+
+Called as a buffer-local `read-only-mode-hook' function."
+  (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 +2763,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 +3162,46 @@ term-emulate-terminal
     (when (get-buffer-window (current-buffer))
       (redisplay))))
 
+(defvar-local term-goto-process-mark t
+  "Whether to reset point to the current process mark after this command.
+
+Set in `pre-command-hook' in char mode by `term-set-goto-process-mark'.")
+
+(defun term-set-goto-process-mark ()
+  "Sets `term-goto-process-mark'.
+
+Always set to nil if `term-char-mode-point-at-process-mark' is nil.
+
+Called as a buffer-local `pre-command-hook' function in
+`term-char-mode' so that when point is equal to the process mark
+at the pre-command stage, we know to restore point to the process
+mark at the post-command stage.
+
+See also `term-goto-process-mark-maybe'."
+  (setq term-goto-process-mark
+        (and term-char-mode-point-at-process-mark
+             (eq (point) (marker-position (term-process-mark))))))
+
+(defun term-goto-process-mark-maybe ()
+  "Move point to the term buffer's process mark upon keyboard input.
+
+Called 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.
+
+Only acts when the pre-command position of point was equal to the
+process mark, and the `term-char-mode-point-at-process-mark'
+option is enabled.  See `term-set-goto-process-mark'."
+  (when term-goto-process-mark
+    (unless (mouse-event-p last-command-event)
+      (goto-char (term-process-mark)))))
+
+(defun term-process-mark ()
+  "The current `process-mark' for the term buffer process."
+  (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

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

[Eli Zaretskii]

> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: p.stephani2@gmail.com, 24837@debbugs.gnu.org, 21609@debbugs.gnu.org,
>  bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org
> Date: Tue, 10 Oct 2017 03:04:24 +1300
> 
> This is now done, and rebased onto the emacs-26 branch.  The new user
> options are enabled by default, and a NEWS entry added.  New patch is
> attached; please review.

Thanks, see below.

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1148,6 +1148,18 @@ languages.  (Version 2.1.0 or later of Enchant is required.)
>  +++
>  *** Emacs no longer prompts the user before killing Flymake processes on exit.
>  
> +** Term
> +
> ++++

This indication means the change is already described in the manual.
But since the patch doesn't include any doc changes except NEWS, I
presume you decided this shouldn't be mentioned in the manual (and I
agree), so the indication should be "---" instead.

> +*** New user options `term-char-mode-buffer-read-only' and
> +`term-char-mode-point-at-process-mark' are enabled by default to avoid
> +buffer states being created in `term-char-mode' which are inconsistent
> +with the terminal state expected by the inferior process.
> +
> +Respectively, these options prevent buffer changes which are not
> +caused by the process filter, and counter-act movements of point away
> +from the process mark (with the exception of mouse events).

I have a difficulty understanding why the user would like to change
the values of these options from their defaults.  How about revising
this text to describe (a) the default behavior, and (b) when would
someone want to change that by tweaking these options?

> +(defcustom term-char-mode-buffer-read-only t
> +  "If non-nil, only the process filter may modify the buffer in char mode.
> +
> +This makes the buffer read-only in char mode (except for the
> +process filter), to prevent editing commands from causing a
> +buffer state which is inconsistent with the state understood by
> +the inferior process."
> +  :type 'boolean
> +  :group 'term)
> +
> +(defcustom term-char-mode-point-at-process-mark t
> +  "If non-nil, keep point at the process mark in char mode.

Same comment about the doc strings of these options.

Also, please add a :version tag to these options.

Thanks.

bug#28777: Clarifying the temporary documentation update marks in etc/NEWS

[Phil Sainty]

On 10/10/17 04:32, Eli Zaretskii wrote:
>> +++
>
> This indication means the change is already described in the manual.
> But since the patch doesn't include any doc changes except NEWS, I
> presume you decided this shouldn't be mentioned in the manual (and
> I agree), so the indication should be "---" instead.

I conclude that I'm a little confused about the description of the
two marks.  I had intentionally used "+++" because "all necessary
documentation updates are complete" (but I can see now that this
could make "---" redundant, unless both marks were used together).

etc/NEWS says:

> +++ indicates that all necessary documentation updates are complete.
>     (This means all relevant manuals in doc/ AND lisp doc-strings.)
> --- means no change in the manuals is needed.

If I now understand correctly, "---" means that all relevant lisp doc
strings have been updated, and "+++" means the same thing and that
there have additionally been updates to the manual?

If so, then I think this text could be clarified:

> +++ indicates that all necessary documentation updates are complete.
>     (This means all relevant manuals in doc/ AND lisp doc-strings.)
> --- indicates that all lisp doc-string updates are complete and
>     that no change in the manuals is needed.


-Phil

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

[Phil Sainty]

On 10/10/17 04:32, Eli Zaretskii wrote:
>> +*** New user options `term-char-mode-buffer-read-only' and
>> +`term-char-mode-point-at-process-mark'

> I have a difficulty understanding why the user would like to change
> the values of these options from their defaults.  How about revising
> this text to describe (a) the default behavior, and (b) when would
> someone want to change that by tweaking these options?

I'm likewise not sure whether users will *actually* want to do this.
I just wanted to make sure that they had the ability to do so, in case
these changes somehow interfered with their workflows (given that these
are notable changes to some long-standing behaviour).

I could 'demote' them to plain defvars, if we think that their use is
so unlikely that they don't actually warrant being included in the
customize group?

In any case I'll see if I can improve the wording...


-Phil

bug#28777: Clarifying the temporary documentation update marks in etc/NEWS

[Eli Zaretskii]

> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: Eli Zaretskii <eliz@gnu.org>
> Date: Tue, 10 Oct 2017 23:16:16 +1300
> 
> etc/NEWS says:
> 
> > +++ indicates that all necessary documentation updates are complete.
> >     (This means all relevant manuals in doc/ AND lisp doc-strings.)
> > --- means no change in the manuals is needed.
> 
> If I now understand correctly, "---" means that all relevant lisp doc
> strings have been updated, and "+++" means the same thing and that
> there have additionally been updates to the manual?
> 
> If so, then I think this text could be clarified:

Thanks, I've clarified it.

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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 24837@debbugs.gnu.org, 21609@debbugs.gnu.org,
>  bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org
> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Wed, 11 Oct 2017 00:11:17 +1300
> 
> On 10/10/17 04:32, Eli Zaretskii wrote:
> >> +*** New user options `term-char-mode-buffer-read-only' and
> >> +`term-char-mode-point-at-process-mark'
> 
> > I have a difficulty understanding why the user would like to change
> > the values of these options from their defaults.  How about revising
> > this text to describe (a) the default behavior, and (b) when would
> > someone want to change that by tweaking these options?
> 
> I'm likewise not sure whether users will *actually* want to do this.

In that case, the text should say something like

  Customize these options to nil if you want the previous behavior.

> In any case I'll see if I can improve the wording...

Thanks.

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

[Phil Sainty]

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

On 11/10/17 01:35, Eli Zaretskii wrote:
> In that case, the text should say something like
>   Customize these options to nil if you want the previous behavior.
> 
>> In any case I'll see if I can improve the wording...

I've revised the text for the NEWS entry and both user options.


[-- Attachment #2: 0001-Avoid-creating-inconsistent-buffer-states-in-term-ch.patch --]
[-- Type: text/x-patch, Size: 8388 bytes --]

From c0c1cb126dfc68a44be2b8d4b4a9350823d3d47c Mon Sep 17 00:00:00 2001
From: Phil Sainty <psainty@orcon.net.nz>
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-char-mode-buffer-read-only: New user option.
term-line-mode-buffer-read-only: New buffer-local variable.
(term-line-mode-buffer-read-only-update): New function.

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

term-char-mode-point-at-process-mark: New user option.
term-goto-process-mark: New buffer-local variable.
(term-set-goto-process-mark): New function.
(term-goto-process-mark-maybe): New function.
(term-process-mark): New function.

* etc/NEWS: Mention the new behaviour and user options.
---
 etc/NEWS     | 17 +++++++++++
 lisp/term.el | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+)

diff --git a/etc/NEWS b/etc/NEWS
index 87a82de..6b26a69 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1164,6 +1164,23 @@ provided.
 The old Flymake behaviour is preserved in the so-called "legacy
 backend", which has been updated to benefit from the new UI features.
 
+** Term
+
+---
+*** `term-char-mode' now avoids buffer states which are inconsistent
+with the state of the terminal understood by the inferior process.
+
+The buffer is made read-only to prevent changes from being made by
+anything other than the process filter; and movements of point away
+from the process mark are counter-acted so that the cursor is in the
+correct position after each command.
+
+New user options `term-char-mode-buffer-read-only' and
+`term-char-mode-point-at-process-mark' control these behaviours, and
+are enabled by default.  Customize these options to nil if you want
+the previous behaviours.
+
+
 \f
 * New Modes and Packages in Emacs 26.1
 
diff --git a/lisp/term.el b/lisp/term.el
index c748c45..2046578 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."
@@ -487,6 +489,41 @@ term-input-ring-file-name
   :type 'boolean
   :group 'term)
 
+(defcustom term-char-mode-buffer-read-only t
+  "If non-nil, only the process filter may modify the buffer in char mode.
+
+A non-nil value makes the buffer read-only in `term-char-mode',
+which prevents editing commands from making the buffer state
+inconsistent with the state of the terminal understood by the
+inferior process.  Only the process filter is allowed to make
+changes to the buffer.
+
+Customize this option to nil if you want the previous behaviour."
+  :version "26.1"
+  :type 'boolean
+  :group 'term)
+
+(defcustom term-char-mode-point-at-process-mark t
+  "If non-nil, keep point at the process mark in char mode.
+
+A non-nil value causes point to be moved to the current process
+mark after each command in `term-char-mode' (provided that the
+pre-command point position was also at the process mark).  This
+prevents commands that move point from making the buffer state
+inconsistent with the state of the terminal understood by the
+inferior process.
+
+Mouse events are not affected, so moving point and selecting text
+is still possible in char mode via the mouse, after which other
+commands can be invoked on the mouse-selected point or region,
+until the process filter (or user) moves point to the process
+mark once again.
+
+Customize this option to nil if you want the previous behaviour."
+  :version "26.1"
+  :type 'boolean
+  :group 'term)
+
 (defcustom term-scroll-to-bottom-on-output nil
   "Controls whether interpreter output causes window to scroll.
 If nil, then do not scroll.  If t or `all', scroll all windows showing buffer.
@@ -1105,6 +1142,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 +1285,13 @@ 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.
+    (when term-char-mode-buffer-read-only
+      (setq buffer-read-only t))
+    (add-hook 'pre-command-hook #'term-set-goto-process-mark nil t)
+    (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 +1311,20 @@ term-line-mode
 you type \\[term-send-input] which sends the current line to the inferior."
   (interactive)
   (when (term-in-char-mode)
+    (when term-char-mode-buffer-read-only
+      (setq buffer-read-only term-line-mode-buffer-read-only))
+    (remove-hook 'pre-command-hook #'term-set-goto-process-mark t)
+    (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'.
+
+Called as a buffer-local `read-only-mode-hook' function."
+  (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 +2768,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 +3167,46 @@ term-emulate-terminal
     (when (get-buffer-window (current-buffer))
       (redisplay))))
 
+(defvar-local term-goto-process-mark t
+  "Whether to reset point to the current process mark after this command.
+
+Set in `pre-command-hook' in char mode by `term-set-goto-process-mark'.")
+
+(defun term-set-goto-process-mark ()
+  "Sets `term-goto-process-mark'.
+
+Always set to nil if `term-char-mode-point-at-process-mark' is nil.
+
+Called as a buffer-local `pre-command-hook' function in
+`term-char-mode' so that when point is equal to the process mark
+at the pre-command stage, we know to restore point to the process
+mark at the post-command stage.
+
+See also `term-goto-process-mark-maybe'."
+  (setq term-goto-process-mark
+        (and term-char-mode-point-at-process-mark
+             (eq (point) (marker-position (term-process-mark))))))
+
+(defun term-goto-process-mark-maybe ()
+  "Move point to the term buffer's process mark upon keyboard input.
+
+Called 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.
+
+Only acts when the pre-command position of point was equal to the
+process mark, and the `term-char-mode-point-at-process-mark'
+option is enabled.  See `term-set-goto-process-mark'."
+  (when term-goto-process-mark
+    (unless (mouse-event-p last-command-event)
+      (goto-char (term-process-mark)))))
+
+(defun term-process-mark ()
+  "The current `process-mark' for the term buffer process."
+  (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

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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 24837@debbugs.gnu.org, 21609@debbugs.gnu.org,
>  bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org
> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Thu, 12 Oct 2017 23:54:42 +1300
> 
> I've revised the text for the NEWS entry and both user options.

Thanks, this is fine with me, but please in the future use the US
English spelling of words such as "behavior".  (No need to send
another patch with that fixed this time.)

I will look into pushing this in a few days, if no objections or
comments surface.

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

[Eli Zaretskii]

> Cc: p.stephani2@gmail.com, 24837@debbugs.gnu.org, 21609@debbugs.gnu.org,
>  bug-gnu-emacs-bounces+psainty=orcon.net.nz@gnu.org
> From: Phil Sainty <psainty@orcon.net.nz>
> Date: Thu, 12 Oct 2017 23:54:42 +1300
> 
> On 11/10/17 01:35, Eli Zaretskii wrote:
> > In that case, the text should say something like
> >   Customize these options to nil if you want the previous behavior.
> > 
> >> In any case I'll see if I can improve the wording...
> 
> I've revised the text for the NEWS entry and both user options.

Thanks, pushed to the release branch.