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

From: Phil Sainty <[email-masked]>
Date: Wed, 15 Mar 2023 01:46:28 +1300

On 2022-10-20 18:36, Eli Zaretskii wrote:
>> Date: Thu, 20 Oct 2022 11:14:50 +1300
>> From: Phil Sainty <[email-masked]>
>> 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 <[email-masked]>
   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)
commit 2d7502b55b52b9668c2c920f1bfa5bd437fb3b96
Author: Richard M. Stallman <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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))))))