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

[Phil Sainty]

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)

term-mouse-paste-mouse-set-point.diff
Description: Text Data