kill-new may replace the wrong item

kill-new may replace the wrong item

[Leo]

Hello All,

In the BODY of kill-new, it sets REPLACE t if the new entry is the same
as the head of the kill-ring.

When save-interprogram-paste-before-kill is non-nil, at the time of
replacing it may replace the entry from 'clipboard' instead of the one
it used to set REPLACE to t.

Is this a bug?

Leo

Re: kill-new may replace the wrong item

[Juri Linkov]

> In the BODY of kill-new, it sets REPLACE t if the new entry is the same
> as the head of the kill-ring.
>
> When save-interprogram-paste-before-kill is non-nil, at the time of
> replacing it may replace the entry from 'clipboard' instead of the one
> it used to set REPLACE to t.
>
> Is this a bug?

I have non-nil save-interprogram-paste-before-kill and non-nil
kill-do-not-save-duplicates, but I never noticed a problem with them.

Could you please provide step-by-step test case to reproduce this?

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

Re: kill-new may replace the wrong item

[Leo]

On 1 June 2010 20:58, Juri Linkov <juri@jurta.org> wrote:
>> In the BODY of kill-new, it sets REPLACE t if the new entry is the same
>> as the head of the kill-ring.
>>
>> When save-interprogram-paste-before-kill is non-nil, at the time of
>> replacing it may replace the entry from 'clipboard' instead of the one
>> it used to set REPLACE to t.
>>
>> Is this a bug?
>
> I have non-nil save-interprogram-paste-before-kill and non-nil
> kill-do-not-save-duplicates, but I never noticed a problem with them.
>
> Could you please provide step-by-step test case to reproduce this?

I was debugging another problem due to pasting images but it was not
the fault of kill-new.
The problem with kill-new is as commented here:

;;; 1. here REPLACE is set to T according to the CAR at this time.
(when (and kill-do-not-save-duplicates
           (equal string (car kill-ring)))
  (setq replace t))

(if (fboundp 'menu-bar-update-yank-menu)
    (menu-bar-update-yank-menu string (and replace (car kill-ring))))

;;; 2. things from clipboard or whatnot get pushed into kill-ring
(when save-interprogram-paste-before-kill
  (let ((interprogram-paste (and interprogram-paste-function
                                 (funcall interprogram-paste-function))))
    (when interprogram-paste
      (if (listp interprogram-paste)
          (dolist (s (nreverse interprogram-paste))
            (push s kill-ring))
        (push interprogram-paste kill-ring)))))

;;; 3. when REPLACE happens, CAR may have changed due to step 2
(if (and replace kill-ring)
    (setcar kill-ring string)
  (push string kill-ring)
  (if (> (length kill-ring) kill-ring-max)
      (setcdr (nthcdr (1- kill-ring-max) kill-ring) nil)))

So kill-new could end up replacing the item it is not supposed to replace.

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

Leo

Re: kill-new may replace the wrong item

[Leo]

On 2 June 2010 04:10, Leo <sdl.web@gmail.com> wrote:
> So kill-new could end up replacing the item it is not supposed to replace.

When this happens duplicate entries will end up in the kill ring. The
old CAR now get pushed down the kill-ring, and the NEW CAR, that was
originally from the clipboard but get replaced with the 'STRING' arg.

Leo

Re: kill-new may replace the wrong item

[Juri Linkov]

> When this happens duplicate entries will end up in the kill ring. The
> old CAR now get pushed down the kill-ring, and the NEW CAR, that was
> originally from the clipboard but get replaced with the 'STRING' arg.

Do you think `kill-do-not-save-duplicates' should take into account
clipboard strings too and don't allow duplicates from clipboard?
Maybe.  But should the same logic apply to the menu items of yank-menu?
Should `menu-bar-update-yank-menu' use the value of `(car kill-ring)'
that was *before* pushing clipboard into kill-ring, but the value of
`replace' that is *after* pushing clipboard into kill-ring and
comparing it with the new value of `(car kill-ring)'?

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

Re: kill-new may replace the wrong item

[Leo]

On 2 June 2010 20:53, Juri Linkov <juri@jurta.org> wrote:
> Do you think `kill-do-not-save-duplicates' should take into account
> clipboard strings too and don't allow duplicates from clipboard?
> Maybe.  But should the same logic apply to the menu items of yank-menu?
> Should `menu-bar-update-yank-menu' use the value of `(car kill-ring)'
> that was *before* pushing clipboard into kill-ring, but the value of
> `replace' that is *after* pushing clipboard into kill-ring and
> comparing it with the new value of `(car kill-ring)'?

All I am trying to point out is there's something incorrect there.
When it detects the CAR is the same as the 'STRING' arg, there's
enough information to do something about it already, i.e.:

   (when (and kill-do-not-save-duplicates
              (equal string (car kill-ring)))
-    (setq replace t))
+    (pop kill-ring))
   (if (fboundp 'menu-bar-update-yank-menu)
       (menu-bar-update-yank-menu string (and replace (car kill-ring))))
   (when save-interprogram-paste-before-kill

is the old way of setting REPLACE to T is for the convenience of
menu-bar-update-yank-menu?

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

Leo

Re: kill-new may replace the wrong item

[Juri Linkov]

> All I am trying to point out is there's something incorrect there.

The whole implementation of `kill-do-not-save-duplicates' is incorrect.
It should do nothing instead of setting `replace' to t and replacing the
same string with itself (that may override the value pushed from
`interprogram-paste' to `kill-ring').

I believe this patch will provide the consistent interaction between
`kill-do-not-save-duplicates' and `save-interprogram-paste-before-kill'.
Before pushing a string to kill-ring it compares the head of kill-ring with
the string from interprogram-paste and later with the input argument `string':

=== modified file 'lisp/simple.el'
--- lisp/simple.el	2010-05-19 19:17:29 +0000
+++ lisp/simple.el	2010-06-03 19:16:18 +0000
@@ -3009,24 +3009,25 @@ (defun kill-new (string &optional replac
     (if yank-handler
 	(signal 'args-out-of-range
 		(list string "yank-handler specified for empty string"))))
-  (when (and kill-do-not-save-duplicates
-             (equal string (car kill-ring)))
-    (setq replace t))
   (if (fboundp 'menu-bar-update-yank-menu)
       (menu-bar-update-yank-menu string (and replace (car kill-ring))))
   (when save-interprogram-paste-before-kill
     (let ((interprogram-paste (and interprogram-paste-function
                                    (funcall interprogram-paste-function))))
       (when interprogram-paste
-        (if (listp interprogram-paste)
-            (dolist (s (nreverse interprogram-paste))
-              (push s kill-ring))
-            (push interprogram-paste kill-ring)))))
-  (if (and replace kill-ring)
-      (setcar kill-ring string)
-    (push string kill-ring)
-    (if (> (length kill-ring) kill-ring-max)
-	(setcdr (nthcdr (1- kill-ring-max) kill-ring) nil)))
+        (dolist (s (if (listp interprogram-paste)
+		       (nreverse interprogram-paste)
+		     (list interprogram-paste)))
+	  (unless (and kill-do-not-save-duplicates
+		       (equal s (car kill-ring)))
+	    (push s kill-ring))))))
+  (unless (and kill-do-not-save-duplicates
+	       (equal string (car kill-ring)))
+    (if (and replace kill-ring)
+	(setcar kill-ring string)
+      (push string kill-ring)
+      (if (> (length kill-ring) kill-ring-max)
+	  (setcdr (nthcdr (1- kill-ring-max) kill-ring) nil))))
   (setq kill-ring-yank-pointer kill-ring)
   (if interprogram-cut-function
       (funcall interprogram-cut-function string (not replace))))

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

Re: kill-new may replace the wrong item

[Leo]

On 3 June 2010 20:17, Juri Linkov <juri@jurta.org> wrote:
>> All I am trying to point out is there's something incorrect there.
>
> The whole implementation of `kill-do-not-save-duplicates' is incorrect.
> It should do nothing instead of setting `replace' to t and replacing the
> same string with itself (that may override the value pushed from
> `interprogram-paste' to `kill-ring').
>
> I believe this patch will provide the consistent interaction between
> `kill-do-not-save-duplicates' and `save-interprogram-paste-before-kill'.
> Before pushing a string to kill-ring it compares the head of kill-ring with
> the string from interprogram-paste and later with the input argument `string':
[patch]

Thanks for fixing this.

Leo