Yanking wrong text

Yanking wrong text

[Juri Linkov]

I propose removing annoying behavior of yanking text.

Steps to reproduce: with delete-selection-mode or cua-selection-mode
I copy some region into the kill ring (`M-w').  After that I select
another region, and type `C-y' to replace the selected region,
but instead of inserting the copied region from the kill ring,
it inserts some completely unrelated text.  Then I need to undo this
action and start looking why does it insert wrong text.  When I start
trying to copy this region again, I realize that the reason of this
behavior is an "evil" optimization: if the text to replace is the same
as the text on the top of the kill ring, then it inserts the text from the
second element of the kill ring.

The comment in the implementation says that this was done because
otherwise yank would be a no-op.  So what?  I still need to insert
the last copied region even if it happens to be the same as the selected
region, and not insert some arbitrary text from the rest of the kill ring.

Index: lisp/delsel.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/delsel.el,v
retrieving revision 1.38
diff -c -r1.38 delsel.el
*** lisp/delsel.el	21 Jan 2007 03:53:12 -0000	1.38
--- lisp/delsel.el	22 Jun 2007 20:59:00 -0000
***************
*** 87,99 ****
  	  (cond ((eq type 'kill)
  		 (delete-active-region t))
  		((eq type 'yank)
- 		 ;; Before a yank command,
- 		 ;; make sure we don't yank the same region
- 		 ;; that we are going to delete.
- 		 ;; That would make yank a no-op.
- 		 (when (string= (buffer-substring-no-properties (point) (mark))
- 				(car kill-ring))
- 		   (current-kill 1))
  		 (delete-active-region))
  		((eq type 'supersede)
  		 (let ((empty-region (= (point) (mark))))
--- 87,92 ----

Index: lisp/emulation/cua-base.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/emulation/cua-base.el,v
retrieving revision 1.78
diff -c -r1.78 cua-base.el
*** lisp/emulation/cua-base.el	24 Feb 2007 21:52:43 -0000	1.78
--- lisp/emulation/cua-base.el	22 Jun 2007 20:59:10 -0000
***************
*** 898,906 ****
       (t
        ;; Must save register here, since delete may override reg 0.
        (if mark-active
- 	  ;; Before a yank command, make sure we don't yank
- 	  ;; the same region that we are going to delete.
- 	  ;; That would make yank a no-op.
  	  (if cua--rectangle
  	      (progn
  		(goto-char (min (mark) (point)))
--- 898,903 ----
***************
*** 908,916 ****
  		(setq paste-lines (cua--delete-rectangle))
  		(if (= paste-lines 1)
  		    (setq paste-lines nil))) ;; paste all
- 	    (if (string= (filter-buffer-substring (point) (mark))
- 			 (car kill-ring))
- 		(current-kill 1))
  	    (cua-delete-region)))
        (cond
         (regtxt
--- 905,910 ----

Re: Yanking wrong text

[Stefan Monnier]

> The comment in the implementation says that this was done because
> otherwise yank would be a no-op.  So what?  I still need to insert
> the last copied region even if it happens to be the same as the selected
> region, and not insert some arbitrary text from the rest of the kill ring.

I do not understand.  Please explain in more details your scenario.  I don't
really understand what you're trying to do and what you're getting instead.


        Stefan

Re: Yanking wrong text

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The comment in the implementation says that this was done because
>> otherwise yank would be a no-op.  So what?  I still need to insert
>> the last copied region even if it happens to be the same as the
>> selected region, and not insert some arbitrary text from the rest
>> of the kill ring.
>
> I do not understand.  Please explain in more details your scenario.
> I don't really understand what you're trying to do and what you're
> getting instead.

I could imagine that for keyboard macros, such cleverness could have
absolutely devastating results.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: Yanking wrong text

[Juri Linkov]

>> The comment in the implementation says that this was done because
>> otherwise yank would be a no-op.  So what?  I still need to insert
>> the last copied region even if it happens to be the same as the selected
>> region, and not insert some arbitrary text from the rest of the kill ring.
>
> I do not understand.  Please explain in more details your scenario.  I don't
> really understand what you're trying to do and what you're getting instead.

I'll describe step by step what I do.  Let's suppose that I have some
text (e.g. "garbage") on the top of the kill ring.  I copied this text
to the kill ring long ago (with e.g. M-w or C-w) and forgot about its
existence in the kill ring.

Now I want to copy some region to another place (no matter in the
same buffer or another buffer).  I select this region and add it
to the kill ring with M-w.  Now I go to another place, select a region
I want to replace with the text added to the kill ring from the first
region.  Now I select this region, and I expect that typing C-y will
replace it with the text of the first region from the kill ring.
When delete-selection-mode and cua-selection-mode are enabled, and
transient-mark-mode is also enabled, then yanked text from the top of the
kill ring replaces the active selection.  But really C-y replaces the
selection not with the text of the first region from the top of the kill
ring, but with the text "garbage" which is the second element of the kill
ring.  That's because the selected region happens to be the same as the
first region added to the top of the kill ring, and due to optimization
C-y inserts the second element of the kill ring.

You may ask why do I want to replace a region with exactly same text.
Usually text is very large, so I don't see that these regions have
no differences.  My goal is to copy the region to another place regardless
of the fact that it happens to be the same as the selected region to be
replaced in another place.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Yanking wrong text

[Richard Stallman]

      That's because the selected region happens to be the same as the
    first region added to the top of the kill ring, and due to optimization
    C-y inserts the second element of the kill ring.

    You may ask why do I want to replace a region with exactly same text.
    Usually text is very large, so I don't see that these regions have
    no differences.  My goal is to copy the region to another place regardless
    of the fact that it happens to be the same as the selected region to be
    replaced in another place.

This heuristic avoids annoying results in a common case.
I wonder if we can tune it to detect that common case
while not triggering in your case.

Re: Yanking wrong text

[Stefan Monnier]

>       That's because the selected region happens to be the same as the
>     first region added to the top of the kill ring, and due to optimization
>     C-y inserts the second element of the kill ring.

>     You may ask why do I want to replace a region with exactly same text.
>     Usually text is very large, so I don't see that these regions have
>     no differences.  My goal is to copy the region to another place regardless
>     of the fact that it happens to be the same as the selected region to be
>     replaced in another place.

I think he's right.

> This heuristic avoids annoying results in a common case.

IIUC the current code tries to handle the following situation:
- select text "toto" with the mouse to "cut" or "copy" it.
- select text "titi" with the mouse.
- hit C-y to replace "titi" with "toto".
What really happens instead is that "titi" gets replaced with "titi" because
the selection of "titi" with the mouse did an implicit M-w.

But in Juri's case he did not select with the mouse, so "titi" wasn't added
to the kill-ring.  Instead in his case "titi" and "toto" are actually the
same string.

> I wonder if we can tune it to detect that common case
> while not triggering in your case.

I think the key is to check not just whether the two strings are equal, but
also whether the head of the kill-ring really comes from the currently
active region.  E.g. check that last-command is one that uses
copy-region-as-kill.


        Stefan

Re: Yanking wrong text

[Juri Linkov]

> IIUC the current code tries to handle the following situation:
> - select text "toto" with the mouse to "cut" or "copy" it.
> - select text "titi" with the mouse.
> - hit C-y to replace "titi" with "toto".
> What really happens instead is that "titi" gets replaced with "titi" because
> the selection of "titi" with the mouse did an implicit M-w.
>
> But in Juri's case he did not select with the mouse, so "titi" wasn't added
> to the kill-ring.  Instead in his case "titi" and "toto" are actually the
> same string.
>
>> I wonder if we can tune it to detect that common case
>> while not triggering in your case.
>
> I think the key is to check not just whether the two strings are equal, but
> also whether the head of the kill-ring really comes from the currently
> active region.  E.g. check that last-command is one that uses
> copy-region-as-kill.

Should we care only for situation of selecting and yanking with the mouse?
In this case we could check last-command-event for mouse keys.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Yanking wrong text

[Richard Stallman]

    I think the key is to check not just whether the two strings are equal, but
    also whether the head of the kill-ring really comes from the currently
    active region.  E.g. check that last-command is one that uses
    copy-region-as-kill.

It might work.  Would you like to try implementing that?

Re: Yanking wrong text

[Juri Linkov]

>     I think the key is to check not just whether the two strings are equal, but
>     also whether the head of the kill-ring really comes from the currently
>     active region.  E.g. check that last-command is one that uses
>     copy-region-as-kill.
>
> It might work.  Would you like to try implementing that?

This patch should handle this:

Index: lisp/delsel.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/delsel.el,v
retrieving revision 1.38
diff -c -r1.38 delsel.el
*** lisp/delsel.el	21 Jan 2007 03:53:12 -0000	1.38
--- lisp/delsel.el	29 Jun 2007 16:31:31 -0000
***************
*** 91,98 ****
  		 ;; make sure we don't yank the same region
  		 ;; that we are going to delete.
  		 ;; That would make yank a no-op.
! 		 (when (string= (buffer-substring-no-properties (point) (mark))
! 				(car kill-ring))
  		   (current-kill 1))
  		 (delete-active-region))
  		((eq type 'supersede)
--- 91,105 ----
  		 ;; make sure we don't yank the same region
  		 ;; that we are going to delete.
  		 ;; That would make yank a no-op.
! 		 (when (and (string= (buffer-substring-no-properties (point) (mark))
! 				     (car kill-ring))
! 			    ;; Check also whether the head of the kill-ring
! 			    ;; really comes from the currently active region,
! 			    ;; i.e. check that last-command is one that uses
! 			    ;; copy-region-as-kill or kill-new
! 			    (memq last-command
! 				  '(mouse-set-region mouse-drag-region
! 				    mouse-save-then-kill mouse-secondary-save-then-kill)))
  		   (current-kill 1))
  		 (delete-active-region))
  		((eq type 'supersede)

Index: lisp/emulation/cua-base.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/emulation/cua-base.el,v
retrieving revision 1.78
diff -c -r1.78 cua-base.el
*** lisp/emulation/cua-base.el	24 Feb 2007 21:52:43 -0000	1.78
--- lisp/emulation/cua-base.el	29 Jun 2007 16:31:41 -0000
***************
*** 898,906 ****
       (t
        ;; Must save register here, since delete may override reg 0.
        (if mark-active
- 	  ;; Before a yank command, make sure we don't yank
- 	  ;; the same region that we are going to delete.
- 	  ;; That would make yank a no-op.
  	  (if cua--rectangle
  	      (progn
  		(goto-char (min (mark) (point)))
--- 898,903 ----
***************
*** 908,915 ****
  		(setq paste-lines (cua--delete-rectangle))
  		(if (= paste-lines 1)
  		    (setq paste-lines nil))) ;; paste all
! 	    (if (string= (filter-buffer-substring (point) (mark))
! 			 (car kill-ring))
  		(current-kill 1))
  	    (cua-delete-region)))
        (cond
--- 905,922 ----
  		(setq paste-lines (cua--delete-rectangle))
  		(if (= paste-lines 1)
  		    (setq paste-lines nil))) ;; paste all
! 	    ;; Before a yank command, make sure we don't yank
! 	    ;; the same region that we are going to delete.
! 	    ;; That would make yank a no-op.
! 	    (if (and (string= (filter-buffer-substring (point) (mark))
! 			      (car kill-ring))
! 		     ;; Check also whether the head of the kill-ring
! 		     ;; really comes from the currently active region,
! 		     ;; i.e. check that last-command is one that uses
! 		     ;; copy-region-as-kill or kill-new
! 		     (memq last-command
! 			   '(mouse-set-region mouse-drag-region
! 			     mouse-save-then-kill mouse-secondary-save-then-kill)))
  		(current-kill 1))
  	    (cua-delete-region)))
        (cond

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Yanking wrong text

[Richard Stallman]

If no problems in your patch arise in a few days, would you please
install it?

Re: Yanking wrong text

[Richard Stallman]

No one reported any problem in your patch.  Would you please
install it and ack?

Re: Yanking wrong text

[Juri Linkov]

> No one reported any problem in your patch.  Would you please
> install it and ack?

Done.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Yanking wrong text

[Richard Stallman]

[I sent this message a few weeks ago but did not get a response.]

    If no problems in your patch arise in a few days, would you please
    install it?

If no one has pointed out any problem so far, would you please install
it now?  Then please ack.

Re: Yanking wrong text

[Juri Linkov]

> [I sent this message a few weeks ago but did not get a response.]
>
>     If no problems in your patch arise in a few days, would you please
>     install it?
>
> If no one has pointed out any problem so far, would you please install
> it now?  Then please ack.

Ack that it was installed.

-- 
Juri Linkov
http://www.jurta.org/emacs/

Re: Yanking wrong text

[Chris Moore]

[-- Attachment #1.1: Type: text/plain, Size: 994 bytes --]

On 6/23/07, Juri Linkov <juri@jurta.org> wrote:

> >> I wonder if we can tune it to detect that common case
> >> while not triggering in your case.
> >
> > I think the key is to check not just whether the two strings are equal,
> but
> > also whether the head of the kill-ring really comes from the currently
> > active region.  E.g. check that last-command is one that uses
> > copy-region-as-kill.
>
> Should we care only for situation of selecting and yanking with the mouse?
> In this case we could check last-command-event for mouse keys.


I don't think that's enough.  The selection may have been unchanged by the
last command.

For example, with a single window in the frame, and using
cua-selection-mode, select one word with the mouse, then select another
word, then type "C-x o" (there's only one window, so this does nothing) then
hit "C-y".  The selected word isn't edited.  Try the same but omit the "C-x
o" step and the selected word is replaced by the previously selected word.

[-- Attachment #1.2: Type: text/html, Size: 1386 bytes --]

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: Yanking wrong text

[Juri Linkov]

>> >> I wonder if we can tune it to detect that common case
>> >> while not triggering in your case.
>> >
>> > I think the key is to check not just whether the two strings are
>> > equal, but also whether the head of the kill-ring really comes from
>> > the currently active region.  E.g. check that last-command is one
>> > that uses copy-region-as-kill.
>>
>> Should we care only for situation of selecting and yanking with the mouse?
>> In this case we could check last-command-event for mouse keys.
>
> I don't think that's enough.  The selection may have been unchanged by the
> last command.

I've found that mouse.el already has the necessary function
`mouse-region-match' that checks if there's an active region
that was set with the mouse.  I fixed both cua-base.el and delsel.el
to use it.

-- 
Juri Linkov
http://www.jurta.org/emacs/