bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

This one has been causing me occasional pain for years, but
only rarely, and I didn't have a recipe to reproduce it until
now.  The following steps don't all need to be followed to
the letter, but hopefully demonstrate the issue well.

0. emacs -Q
1. M-x term (and run your shell; term must be in char-mode)
2. Run "ls" # or anything else to produce a decent amount of
    output.
3. Run "cat >/dev/null" # for safety
4. Using the mouse, select any small piece of text several
    lines above the bottom of the term buffer (e.g. double-
    click a word).
5. Move the mouse to the shell prompt at the end of the buffer
    and middle click to paste that selected text as input.
6. Without moving the mouse, middle click again at the same
    position.
7. Be thankful for step 3.

My impression is that the first middle-click unexpectedly
creates a new primary selection based on the current mouse
position, so the second middle-click then pastes that new
selection as input.

This has bitten me on many occasions when I wanted to repeat
the previously-pasted command as input again, and find that
I've instead asked the shell to execute a ton of unintended
commands! (which, thankfully, tend to only rarely be valid).

This bug exists in all the versions of Emacs I have installed
at present (back to 26.3), and I think this is severe enough
to fix in the emacs-28 branch as well as master.


-Phil




In GNU Emacs 29.0.50 (build 1, x86_64-pc-linux-gnu, X toolkit, cairo
  version 1.15.10, Xaw scroll bars) of 2022-10-18 built on phil-lp
Repository revision: be3d9f717dd317eafc8f511072040a5ff8c1071c
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 
11.0.12008000
System Description: Ubuntu 18.04.6 LTS

Configured using:
  'configure --prefix=/home/phil/emacs/trunk/usr/local
  --with-x-toolkit=lucid --without-sound
  '--program-transform-name=s/^ctags$/ctags_emacs/''

Configured features:
CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBXML2 MODULES NOTIFY INOTIFY PDUMPER PNG RSVG SECCOMP THREADS
TIFF TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM LUCID ZLIB

Important settings:
   value of $LC_MONETARY: en_NZ.UTF-8
   value of $LC_NUMERIC: en_NZ.UTF-8
   value of $LC_TIME: en_NZ.UTF-8
   value of $LANG: en_GB.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8

Major mode: Term

Minor modes in effect:
   minibuffer-line-mode: t
   global-edit-server-edit-mode: t
   savehist-mode: t
   magit-wip-initial-backup-mode: t
   magit-wip-before-change-mode: t
   magit-wip-after-apply-mode: t
   magit-wip-after-save-mode: t
   magit-wip-mode: t
   global-git-commit-mode: t
   magit-auto-revert-mode: t
   shell-dirtrack-mode: t
   my-contextual-help-mode: t
   global-so-long-mode: t
   display-battery-mode: t
   my-visible-bell-mode: t
   global-display-fill-column-indicator-mode: t
   display-fill-column-indicator-mode: t
   minibuffer-depth-indicate-mode: t
   which-key-mode: t
   windmove-mode: t
   winner-mode: t
   global-subword-mode: t
   display-time-mode: t
   keep-buffers-mode: t
   my-keys-local-minor-mode: t
   auto-compile-on-load-mode: t
   auto-compile-on-save-mode: t
   url-handler-mode: t
   tooltip-mode: t
   global-eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   buffer-read-only: t
   column-number-mode: t
   line-number-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
/home/phil/.emacs.d.sandboxes/trunk/HOME/.emacs.d/el-get/scratch/el-get 
hides 
/home/phil/.emacs.d.sandboxes/trunk/HOME/.emacs.d/el-get/el-get/el-get
/home/phil/.emacs.d.sandboxes/trunk/HOME/.emacs.d/el-get/delight/delight 
hides 
/home/phil/.emacs.d.sandboxes/trunk/HOME/.emacs.d/elpa/delight-1.7/delight
/home/phil/.emacs.d.sandboxes/trunk/HOME/.emacs.d/el-get/transient/lisp/transient 
hides 
/home/phil/emacs/trunk/usr/local/share/emacs/29.0.50/lisp/transient
/home/phil/.emacs.d.sandboxes/trunk/HOME/.emacs.d/lisp/python hides 
/home/phil/emacs/trunk/usr/local/share/emacs/29.0.50/lisp/progmodes/python

Features:
(shadow sort project-local-variables ecomplete mail-extr emacsbug term
disp-table ehelp dired-aux elisp-slime-nav etags fileloop generator xref
project hl-sexp lexbind-mode idle-highlight-mode tramp-sh warnings
docker-tramp tramp-cache time-stamp tramp tramp-loaddefs trampver
tramp-integration cus-start files-x tramp-compat parse-time iso8601
ls-lisp tabify minibuffer-line edit-server my-org my-projects my-session
savehist desktop frameset my-theme zenburn-theme my-mail autoloads
my-libraries sudo my-version-control magit-wip magit-log which-func
imenu edebug debug backtrace find-func magit-diff smerge-mode diff
git-commit rx log-edit message sendmail yank-media puny rfc822 mml
mml-sec epa epg rfc6068 epg-config gnus-util text-property-search
time-date mm-decode mm-bodies mm-encode mail-parse rfc2231 rfc2047
rfc2045 mm-util ietf-drums mail-prsvr mailabbrev mail-utils gmm-utils
mailheader pcvs-util add-log magit-core magit-autorevert autorevert
filenotify magit-margin magit-transient magit-process with-editor shell
pcomplete comint ansi-osc server ansi-color magit-mode transient pcase
edmacro kmacro compat compat-macs format-spec magit-git magit-section
magit-utils crm dash my-text my-programming my-python so-long
my-whitespace ws-trim my-rectangles my-utilities browse-kill-ring
my-configuration cus-edit pp cus-load wid-edit dired-details dired-x
highlight-parentheses battery delight delsel cua-base ffap
display-fill-column-indicator mb-depth which-key framemove windmove
winner ring cap-words superword subword hl-line time my-externals
.loaddefs windcycle transpose-frame simple-wiki derived sdcv-mode
noutline outline icons sauron rainbow-mode notify dbus xml
multiple-cursors mc-separate-operations rectangular-region-mode
mc-mark-pop mc-mark-more thingatpt mc-cycle-cursors mc-edit-lines
multiple-cursors-core rect mo-git-blame keep-buffers iedit fic-mode
dtrt-indent browse-at-remote vc-git diff-mode easy-mmode vc-dispatcher s
el-get cl-extra help-mode autoload loaddefs-gen radix-tree lisp-mnt cl
dired dired-loaddefs jka-compr my-local my-keybindings auto-compile
packed compat-autoloads etags-select-autoloads info
project-local-variables-autoloads advice wtf-autoloads package
browse-url url url-proxy url-privacy url-expand url-methods url-history
url-cookie generate-lisp-file url-domsuf url-util mailcap url-handlers
url-parse auth-source cl-seq eieio eieio-core cl-macs password-cache
json subr-x map byte-opt gv bytecomp byte-compile cconv url-vars
cl-loaddefs cl-lib rmc iso-transl tooltip eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
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 composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 257815 38654)
  (symbols 48 24320 0)
  (strings 32 74590 14132)
  (string-bytes 1 2492927)
  (vectors 16 39953)
  (vector-slots 8 456801 46856)
  (floats 8 202 110)
  (intervals 56 886 0)
  (buffers 1000 14))

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

On 2022-10-18 23:10, Phil Sainty wrote:
> 4. Using the mouse, select any small piece of text several
>    lines above the bottom of the term buffer (e.g. double-
>    click a word).
> 5. Move the mouse to the shell prompt at the end of the buffer
>    and middle click to paste that selected text as input.
> 6. Without moving the mouse, middle click again at the same
>    position.

In fact "Without moving the mouse" is irrelevant in step 6.
All that matters is where the mouse was when you middle-clicked
in step 5.  All middle clicks subsequent to step 5 have the same
(bad) effect regardless of the mouse position, pasting the same
unintended selection each time.

I presume that having an *active* selection at step 5 makes the
difference.

More testing confirms that the initial middle click in step 5
updates the selection with the text between the start of the
active selection and the position of the middle click.

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

The `deactivate-mark' call in `term-send-raw-string' is the direct
cause of the unwanted change to the selection.  In more detail...

In line mode middle click calls `mouse-yank-primary', but in char mode
middle click calls:

(defun term-mouse-paste (click)
   "Insert the primary selection at the position clicked on."
   (interactive "e")
   ;; Give temporary modes such as isearch a chance to turn off.
   (run-hooks 'mouse-leave-buffer-hook)
   (setq this-command 'yank)
   (mouse-set-point click)
   (term-send-raw-string (gui-get-primary-selection)))


I traced the following:

(trace-function 'mouse-set-point nil
		(lambda () (format " [%s]" (gui-get-primary-selection))))

(trace-function 'term-mouse-paste nil
		(lambda () (format " [%s]" (gui-get-primary-selection))))

(trace-function 'gui-get-primary-selection)

(trace-function 'term-send-raw-string)

Notice that I'm triggering `gui-get-primary-selection' more often
than it would otherwise be called, by also calling it for context.

My terminal buffer contained the line "this is a test" and I selected
"this" with the mouse, and then middle clicked after the word "test".

We see `term-mouse-paste' passing `term-send-raw-string' the word
"this" after obtaining it from `gui-get-primary-selection'; but then,
after the return of `term-send-raw-string' but *before* the return of
`term-mouse-paste', my call-for-context to `gui-get-primary-selection'
fires, returning the longer string "this is a test".

======================================================================
1 -> (term-mouse-paste (mouse-2 (#<window 8 on *terminal*> 457 (191 . 
226) 421854684 nil 457 (21 . 12) nil (65 . 10) (9 . 18)))) [this]
| 2 -> (gui-get-primary-selection)
| 2 <- gui-get-primary-selection: #("this" 0 1 (font-lock-face term 
fontified t) 1 2 (font-lock-face term fontified t) 2 3 (font-lock-face 
term fontified t) 3 4 (font-lock-face term fontified t))
| 2 -> (mouse-set-point (mouse-2 (#<window 8 on *terminal*> 457 (191 . 
226) 421854684 nil 457 (21 . 12) nil (65 . 10) (9 . 18)))) [this]
| | 3 -> (gui-get-primary-selection)
| | 3 <- gui-get-primary-selection: #("this" 0 1 (font-lock-face term 
fontified t) 1 2 (font-lock-face term fontified t) 2 3 (font-lock-face 
term fontified t) 3 4 (font-lock-face term fontified t))
| 2 <- mouse-set-point: 457 [this]
| 2 -> (gui-get-primary-selection)
| 2 <- gui-get-primary-selection: #("this" 0 1 (font-lock-face term 
fontified t) 1 2 (font-lock-face term fontified t) 2 3 (font-lock-face 
term fontified t) 3 4 (font-lock-face term fontified t))
| 2 -> (term-send-raw-string #("this" 0 1 (font-lock-face term fontified 
t) 1 2 (font-lock-face term fontified t) 2 3 (font-lock-face term 
fontified t) 3 4 (font-lock-face term fontified t)))
| 2 <- term-send-raw-string: nil
| 2 -> (gui-get-primary-selection)
| 2 <- gui-get-primary-selection: #("this is a test" 0 1 (font-lock-face 
term fontified t) 1 2 (font-lock-face term fontified t) 2 3 
(font-lock-face term fontified t) 3 4 (font-lock-face term fontified t) 
4 5 (font-lock-face term fontified t) 5 6 (font-lock-face term fontified 
t) 6 7 (font-lock-face term fontified t) 7 8 (font-lock-face term 
fontified t) 8 9 (font-lock-face term fontified t) 9 10 (font-lock-face 
term fontified t) 10 11 (font-lock-face term fontified t) 11 12 
(font-lock-face term fontified t) 12 13 (font-lock-face term fontified 
t) 13 14 (font-lock-face term fontified t))
1 <- term-mouse-paste: nil [this is a test]
======================================================================

So immediately after (term-send-raw-string (gui-get-primary-selection))
has inserted "this" a second (gui-get-primary-selection) is returning
"this is a test"; so `term-send-raw-string' itself seems like a factor.


I then added some messaging like so:

(defun term-mouse-paste (click)
   "Insert the primary selection at the position clicked on."
   (interactive "e")
   ;; Give temporary modes such as isearch a chance to turn off.
   (run-hooks 'mouse-leave-buffer-hook)
   (setq this-command 'yank)
   (mouse-set-point click)
   (message "before: %s" (gui-get-primary-selection))
   (term-send-raw-string (gui-get-primary-selection))
   (message "after: %s" (gui-get-primary-selection)))

(defun term-send-raw-string (chars)
   (message "0: %s" (gui-get-primary-selection))
   (deactivate-mark)
   (message "1: %s" (gui-get-primary-selection))
   ...)

Which gave me these *Messages*:

before: this
0: this
1: this is a test
after: this is a test

So the `deactivate-mark' call in `term-send-raw-string' causes the
unwanted change to the selection.


-Phil

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

Looking at the docstring for `deactivate-mark' told me about
`select-active-regions'.  I don't know whether this is the correct
solution, but I can confirm that this change to `term-mouse-paste'
appears (after only cursory testing) to fix the bug:

-  (term-send-raw-string (gui-get-primary-selection)))
+  (let ((select-active-regions nil))
+    (term-send-raw-string (gui-get-primary-selection))))

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

Or with documentation included, in case that *is* the correct fix:

-  (term-send-raw-string (gui-get-primary-selection)))
+  ;; Prevent the `deactivate-mark' call in `term-send-raw-string'
+  ;; from changing this selection.
+  (let ((select-active-regions nil))
+    (term-send-raw-string (gui-get-primary-selection))))

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

> In line mode middle click calls `mouse-⁠yank-⁠primary'

I realised I should look at this as well, and sure enough it has
some handling for `select-active-regions':

(defun mouse-yank-primary (click)
   "Insert the primary selection at the position clicked on.
Move point to the end of the inserted text, and set mark at
beginning.  If `mouse-yank-at-point' is non-nil, insert at point
regardless of where you click."
   (interactive "e")
   ;; Give temporary modes such as isearch a chance to turn off.
   (run-hooks 'mouse-leave-buffer-hook)
   ;; Without this, confusing things happen upon e.g. inserting into
   ;; the middle of an active region.
   (when select-active-regions
     (let (select-active-regions)
       (deactivate-mark)))
   (or mouse-yank-at-point (mouse-set-point click))
   (let ((primary (gui-get-primary-selection)))
     (push-mark)
     (insert-for-yank primary)))


The call to (mouse-set-point click) is also conditional here:

(or mouse-yank-at-point (mouse-set-point click))

As opposed to:

(defun term-mouse-paste (click)
   "Insert the primary selection at the position clicked on."
   (interactive "e")
   ;; Give temporary modes such as isearch a chance to turn off.
   (run-hooks 'mouse-leave-buffer-hook)
   (setq this-command 'yank)
   (mouse-set-point click)
   (term-send-raw-string (gui-get-primary-selection)))


I don't know if `mouse-yank-at-point' needs consideration here too?

I wondered whether both of these could be rewritten to use a common
subroutine, but it might be a bit awkward.  Failing that, I would
cross-reference them in code comments (are there any other similar
functions besides?).

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

On 2022-10-19 16:22, Phil Sainty wrote:
> I don't know if `mouse-yank-at-point' needs consideration here too?

No, I don't think it does.

"If non-nil, mouse yank commands yank at point instead of at click."

But in `term-mouse-paste' (which is only for char mode), we're neither
yanking at point or click, but instead sending input to the inferior
process, so I believe `mouse-yank-at-point' is irrelevant here.

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Eli Zaretskii]

> Date: Wed, 19 Oct 2022 14:19:20 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> 
> Looking at the docstring for `deactivate-mark' told me about
> `select-active-regions'.  I don't know whether this is the correct
> solution, but I can confirm that this change to `term-mouse-paste'
> appears (after only cursory testing) to fix the bug:
> 
> -  (term-send-raw-string (gui-get-primary-selection)))
> +  (let ((select-active-regions nil))
> +    (term-send-raw-string (gui-get-primary-selection))))

I think it could be important to understand why select-active-regions
causes this problem in your case.

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

On 2022-10-20 00:05, Eli Zaretskii wrote:
> From: Phil Sainty <psainty@orcon.net.nz>
>> Looking at the docstring for `deactivate-mark' told me about
>> `select-active-regions'.  I don't know whether this is the correct
>> solution, but I can confirm that this change to `term-mouse-paste'
>> appears (after only cursory testing) to fix the bug:
>> 
>> -  (term-send-raw-string (gui-get-primary-selection)))
>> +  (let ((select-active-regions nil))
>> +    (term-send-raw-string (gui-get-primary-selection))))
> 
> I think it could be important to understand why select-active-regions
> causes this problem in your case.

I think mechanically it's because the middle click moves point to the
click position, and the call to `deactivate-mark' then causes the
primary selection to be updated based on the current point and mark
(unless we mess with select-active-regions).

You've made me wonder, though... this command is intended only for
term char mode, so should a middle click *really* be setting point?
If all we're trying to do is send the selection text to the inferior
process, that bit might be wrong.

There's an explicit (mouse-set-point click) there, but if I comment
out both that and my interim binding of `select-active-regions' then
this also seems to do the right thing (once more with only very cursory
testing).

(defun term-mouse-paste (click)
   "Insert the primary selection at the position clicked on."
   (interactive "e")
   ;; Give temporary modes such as isearch a chance to turn off.
   (run-hooks 'mouse-leave-buffer-hook)
   (setq this-command 'yank)
   ;; (mouse-set-point click)
   ;; (let ((select-active-regions nil))
   (term-send-raw-string (gui-get-primary-selection)))
   ;;)

That's more of a change, but perhaps it's the correct thing to do.

I'm now looking at that (setq this-command 'yank) as well, and
wondering whether it's important for anything under the impression
that a `yank' just happened to also see point at the location of
the yank.  I'm not sure whether a middle click in a terminal to
send the primary selection directly to the inferior process *should*
be treated as `yank' though -- maybe that code is also wrong.

These changes are more nebulous to me, as they represent more of
a functional change than binding `select-active-regions' does.


-Phil

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Eli Zaretskii]

> Date: Thu, 20 Oct 2022 11:14:50 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 58608@debbugs.gnu.org
> 
> > I think it could be important to understand why select-active-regions
> > causes this problem in your case.
> 
> I think mechanically it's because the middle click moves point to the
> click position, and the call to `deactivate-mark' then causes the
> primary selection to be updated based on the current point and mark
> (unless we mess with select-active-regions).
> 
> You've made me wonder, though... this command is intended only for
> term char mode, so should a middle click *really* be setting point?
> If all we're trying to do is send the selection text to the inferior
> process, that bit might be wrong.

It could be, but maybe looking at the Git history of that code will
tell you why we have that part there?  I mean, maybe there are use
cases where that is important?

If nothing comes up, I think you are right, and that move should be
removed.  On a GUI frame, a middle click leaves point _at_the_end_ of
the inserted text, not where I click.

But if we aren't sure, it's okay to momentarily disable
select-active-regions here, we just need a comment with the
explanation you wrote above.

> I'm now looking at that (setq this-command 'yank) as well, and
> wondering whether it's important for anything under the impression
> that a `yank' just happened to also see point at the location of
> the yank.  I'm not sure whether a middle click in a terminal to
> send the primary selection directly to the inferior process *should*
> be treated as `yank' though -- maybe that code is also wrong.

Indeed, we don't by default treat middle click as yank on GUI frames.
So maybe you are right -- but please note that there are some user
options which perhaps do cause the middle click to be treated as yank
as optional behavior, in which case they should do the same in the
term case.

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

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

On 2022-10-20 18:36, Eli Zaretskii wrote:
>> Date: Thu, 20 Oct 2022 11:14:50 +1300
>> From: Phil Sainty <psainty@orcon.net.nz>
>> You've made me wonder, though... this command is intended only for
>> term char mode, so should a middle click *really* be setting point?
>> If all we're trying to do is send the selection text to the inferior
>> process, that bit might be wrong.
> 
> It could be, but maybe looking at the Git history of that code will
> tell you why we have that part there?  I mean, maybe there are use
> cases where that is important?
> 
> If nothing comes up, I think you are right, and that move should be
> removed.  On a GUI frame, a middle click leaves point _at_the_end_ of
> the inserted text, not where I click.

I don't have my head around all of this, but I've done some digging...

The call to `mouse-set-point' in `term-mouse-paste' dates back to its
origins in commit 4060e6ee3b796ecac506b1ca54217b100795d73a (by RMS)
dated 1994, and then there are a couple of subsequent commits of
interest: commit aade4ab28e774bc2d74a6567aae24e805f30e78a by Per Bothner
in 1995 removed the call to `mouse-set-point' (amongst other changes),
and then in 2004 Richard explicitly added it back again:

   commit 2d7502b55b52b9668c2c920f1bfa5bd437fb3b96
   Author: Richard M. Stallman <rms@gnu.org>
   Date:   Fri Jan 30 16:53:11 2004 +0000

       (term-mouse-paste): Call mouse-set-point.

Richard, I've CC'd you on this basis, as it's not clear to me why the
`term-mouse-paste' command (which I would expect to do nothing besides
send the primary selection text to the inferior process) should be
setting point.  Do you recall the use-case for that?

(n.b. I've attached the above-mentioned commits to this email.)


> But if we aren't sure, it's okay to momentarily disable
> select-active-regions here, we just need a comment with the
> explanation you wrote above.

I'm certainly unsure of much of this.

I can add that bug#2449 from 2009 is where (deactivate-mark) in
`term-send-raw-string' originates, so I think that will be when the
`term-mouse-paste' problem I've reported was effectively introduced,
as it's the `deactivate-mark' call which causes the primary selection
to be updated.

The commits for that were:

commit c5f894821d04f6fbaf3b8c62308692439ec598d3    2009-03-08 19:33
commit 212bb1a81ade52dcbcbb1bceb680e3e0e0b6264c    2009-03-08 19:37
commit 1cb6d11e13c5e28f96558b85f15344abd56f5e18    2009-03-13 01:43

I haven't read through that bug report, but as `deactivate-mark' was the
solution to that, I don't imagine removing that call would be a viable
thing to do now, so let-binding `select-active-regions' still feels like
a reasonable option for the current problem.



>> I'm now looking at that (setq this-command 'yank) as well, and
>> wondering whether it's important for anything under the impression
>> that a `yank' just happened to also see point at the location of
>> the yank.  I'm not sure whether a middle click in a terminal to
>> send the primary selection directly to the inferior process *should*
>> be treated as `yank' though -- maybe that code is also wrong.
> 
> Indeed, we don't by default treat middle click as yank on GUI frames.
> So maybe you are right -- but please note that there are some user
> options which perhaps do cause the middle click to be treated as yank
> as optional behavior, in which case they should do the same in the
> term case.

This code also dates back to the original implementation of
`term-mouse-paste', and bug#6845 from 2010 seems notable for this part.

That bug was about `term-mouse-paste' pasting the current kill instead
of the primary selection.  I don't believe the changes for that are
relevant to the *main* problem I've reported, but when the command was
pasting from the kill ring the (setq this-command 'yank) call would
have made sense, so I suspect this call should have been removed in
commit ff98b2dd51e84b812e061859fa8c682d22b2e459 ?


All in all:

* The (deactivate-mark) in `term-send-raw-string' seems important.
* I don't understand the `mouse-set-point' call, but it may be wanted.
* Disabling `select-active-regions' in `term-mouse-paste' still seems
   like the way to go if point is being set (or maybe regardless).
* (setq this-command 'yank) looks like an old remnant (now a bug).


-Phil (not currently subscribed to the lists; please keep me CC'd)

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: term-mouse-paste-mouse-set-point.diff --]
[-- Type: text/x-diff; name=term-mouse-paste-mouse-set-point.diff, Size: 2569 bytes --]

commit 2d7502b55b52b9668c2c920f1bfa5bd437fb3b96
Author: Richard M. Stallman <rms@gnu.org>
Date:   Fri Jan 30 16:53:11 2004 +0000

    (term-mouse-paste): Call mouse-set-point.

diff --git a/lisp/term.el b/lisp/term.el
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -1163,16 +1163,17 @@

 (defun term-mouse-paste (click arg)
   "Insert the last stretch of killed text at the position clicked on."
   (interactive "e\nP")
   (term-if-xemacs
    (term-send-raw-string (or (condition-case () (x-get-selection) (error ()))
 			     (x-get-cutbuffer)
 			     (error "No selection or cut buffer available"))))
   (term-ifnot-xemacs
    ;; Give temporary modes such as isearch a chance to turn off.
    (run-hooks 'mouse-leave-buffer-hook)
    (setq this-command 'yank)
+   (mouse-set-point click)
    (term-send-raw-string (current-kill (cond
 					((listp arg) 0)
 					((eq arg '-) -1)
 					(t (1- arg)))))))




commit aade4ab28e774bc2d74a6567aae24e805f30e78a
Author: Per Bothner <bothner@cygnus.com>
Date:   Thu Mar 16 02:23:24 1995 +0000

    (term-mouse-paste):  Make work for xemacs.  Minor GNU emacs fixes.

diff --git a/lisp/term.el b/lisp/term.el
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -669,10 +669,16 @@

 (defun term-mouse-paste (click arg)
   "Insert the last stretch of killed text at the position clicked on."
   (interactive "e\nP")
-  (mouse-set-point click)
-  (setq this-command 'yank)
-  (term-send-raw-string (current-kill (cond
-				       ((listp arg) 0)
-				       ((eq arg '-) -1)
-				       (t (1- arg))))))
+  (term-if-xemacs
+   (term-send-raw-string (or (condition-case () (x-get-selection) (error ()))
+			     (x-get-cutbuffer)
+			     (error "No selection or cut buffer available"))))
+  (term-ifnot-xemacs
+   ;; Give temporary modes such as isearch a chance to turn off.
+   (run-hooks 'mouse-leave-buffer-hook)
+   (setq this-command 'yank)
+   (term-send-raw-string (current-kill (cond
+					((listp arg) 0)
+					((eq arg '-) -1)
+					(t (1- arg)))))))




And part of commit 4060e6ee3b796ecac506b1ca54217b100795d73a:
Author: Richard M. Stallman <rms@gnu.org>
Date:   Thu Oct 13 06:30:49 1994 +0000

    Initial revision

diff --git a/lisp/term.el b/lisp/term.el
--- /dev/null
+++ b/lisp/term.el
@@ -0,0 +641,10 @@
+
+(defun term-mouse-paste (click arg)
+  "Insert the last stretch of killed text at the position clicked on."
+  (interactive "e\nP")
+  (mouse-set-point click)
+  (setq this-command 'yank)
+  (term-send-raw-string (current-kill (cond
+				       ((listp arg) 0)
+				       ((eq arg '-) -1)
+				       (t (1- arg))))))

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

On 2023-03-15 01:46, Phil Sainty wrote:
> when the command was pasting from the kill ring the
> (setq this-command 'yank) call would have made sense

On second thought, I'm not sure it made any sense then either -- that
"paste" was still sending the text to the inferior process rather than
inserting it into the buffer, and it's entirely up to the process as
to whether anything at all gets inserted into the buffer as a result,
so I don't think of that as a `yank' as far as Emacs is concerned.

(I'd still hazard a guess that (setq this-command 'yank) was added on
account of dealing with the kill ring, but perhaps it was just a mistake
all along.)


-Phil

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Eli Zaretskii]

> Date: Wed, 15 Mar 2023 03:28:25 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 58608@debbugs.gnu.org, "Richard M. Stallman" <rms@gnu.org>
> 
> On 2023-03-15 01:46, Phil Sainty wrote:
> > when the command was pasting from the kill ring the
> > (setq this-command 'yank) call would have made sense
> 
> On second thought, I'm not sure it made any sense then either -- that
> "paste" was still sending the text to the inferior process rather than
> inserting it into the buffer, and it's entirely up to the process as
> to whether anything at all gets inserted into the buffer as a result,
> so I don't think of that as a `yank' as far as Emacs is concerned.
> 
> (I'd still hazard a guess that (setq this-command 'yank) was added on
> account of dealing with the kill ring, but perhaps it was just a mistake
> all along.)

Thanks for all the forensics.

I think we should install on emacs-29 a change that binds
select-active-regions to nil around the call to term-send-raw-string,
and install on master a change that removes the setting of
this-command.

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Phil Sainty]

On 2023-03-16 20:17, Eli Zaretskii wrote:
> I think we should install on emacs-29 a change that binds
> select-active-regions to nil around the call to term-send-raw-string,
> and install on master a change that removes the setting of
> this-command.

I've installed those changes to emacs-29 and master accordingly.

The question of why this command sets point in the first place is
still outstanding, but I think we'd need a follow-up from Richard
to establish the reason behind that.

In any case, the major issue is now resolved for Emacs 29+.  I'd
happily fix it in the emacs-28 branch as well, on the offchance
that doing so might help someone (as this issue can be harmful if
you're unlucky enough to trigger it); but if that's a no-go then
I think this can be closed.

-Phil

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Eli Zaretskii]

> Date: Mon, 27 Mar 2023 01:45:19 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 58608@debbugs.gnu.org, rms@gnu.org
> 
> In any case, the major issue is now resolved for Emacs 29+.  I'd
> happily fix it in the emacs-28 branch as well, on the offchance
> that doing so might help someone (as this issue can be harmful if
> you're unlucky enough to trigger it); but if that's a no-go then
> I think this can be closed.

This is up to Stefan Kangas.

bug#58608: 29.0.50; Nasty bug with pasting primary selection in term buffers

[Eli Zaretskii]

> Date: Mon, 27 Mar 2023 01:45:19 +1300
> From: Phil Sainty <psainty@orcon.net.nz>
> Cc: 58608@debbugs.gnu.org, rms@gnu.org
> 
> On 2023-03-16 20:17, Eli Zaretskii wrote:
> > I think we should install on emacs-29 a change that binds
> > select-active-regions to nil around the call to term-send-raw-string,
> > and install on master a change that removes the setting of
> > this-command.
> 
> I've installed those changes to emacs-29 and master accordingly.

Thanks.