bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring

[Ignacio Casso <ignaciocasso@hotmail.com>]

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

>> I volunteer to do both if you agree that it would be an improvement.
>
> I do agree, any patches would be greatly welcome.  Thanks in advance.

I have installed the development version of Emacs and made a first patch
attempt, preserving the structure of the program as much as possible. I
have not tested thoroughly, but it works for all the cases I have
tried. I have attached it to this mail, and comment each change
below. Let me know what you think.

>  lisp/menu-bar.el           |  4 +--
>  lisp/obsolete/mouse-sel.el |  2 +-
>  lisp/select.el             | 70 +++++++++++++++++++++++---------------
>  lisp/term/pc-win.el        |  2 +-
>  5 files changed, 47 insertions(+), 33 deletions(-)
> 
> diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
> index ab64928fe7..01683f5c9f 100644
> --- a/lisp/menu-bar.el
> +++ b/lisp/menu-bar.el
> @@ -606,8 +606,8 @@ clipboard-yank
>    "Insert the clipboard contents, or the last stretch of killed text."
>    (interactive "*")
>    (let ((select-enable-clipboard t)
> -        ;; Ensure that we defeat the DWIM login in `gui-selection-value'.
> -        (gui--last-selected-text-clipboard nil))
> +        ;; Ensure that we defeat the DWIM logic in `gui-selection-value'.
> +        (gui--last-clipboard-selection-fingerprint nil))
>      (yank)))
>  
>  (defun clipboard-kill-ring-save (beg end &optional region)

- DWIM login -> DWIM logic (typo)

- replaced gui--last-selected-text-clipboard variable with the new one I
  have introduced (explained below)

- I have tested that clipboard-yank works for the following cases:
  - (setq select-enable-clipboard nil) -> copy in another program -> clipboard-yank
  another program.
  - copy in another program -> C-y -> M-y -> clipboard-yank

> diff --git a/lisp/obsolete/mouse-sel.el b/lisp/obsolete/mouse-sel.el
> index a9d6bfee60..fc91cc9fc1 100644
> --- a/lisp/obsolete/mouse-sel.el
> +++ b/lisp/obsolete/mouse-sel.el
> @@ -304,7 +304,7 @@ mouse-sel-get-selection-function
>      (if (eq selection 'PRIMARY)
>  	(or (gui-selection-value)
>  	    (bound-and-true-p x-last-selected-text-primary)
> -            gui--last-selected-text-primary)
> +            gui--last-selected-text-primary) ;; this variable no longer exists. Does code in lisp/obsolete/ need to be mantained?
>        (gui-get-selection selection)))
>    "Function to call to get the selection.
>  Called with one argument:

Here I have not replaced the variable, since I have assumed that code
under the obsolete/ directory does not need to be maintained, and C-h f
tells me that function is not loaded by default. However there is a
warning when compiling. Should I fix it?


> diff --git a/lisp/select.el b/lisp/select.el
> index 42b50c44e6..55c409d347 100644
> --- a/lisp/select.el
> +++ b/lisp/select.el
> @@ -25,9 +25,10 @@
>  ;; Based partially on earlier release by Lucid.
>  
>  ;; The functionality here is divided in two parts:
> -;; - Low-level: gui-get-selection, gui-set-selection, gui-selection-owner-p,
> -;;   gui-selection-exists-p are the backend-dependent functions meant to access
> -;;   various kinds of selections (CLIPBOARD, PRIMARY, SECONDARY).
> +;; - Low-level: gui-backend-get-selection, gui-backend-set-selection,
> +;;   gui-backend-selection-owner-p, gui-backend-selection-exists-p are
> +;;   the backend-dependent functions meant to access various kinds of
> +;;   selections (CLIPBOARD, PRIMARY, SECONDARY).
>  ;; - Higher-level: gui-select-text and gui-selection-value go together to
>  ;;   access the general notion of "GUI selection" for interoperation with other
>  ;;   applications.  This can use either the clipboard or the primary selection,

- gui-selection-owner-p -> gui-backend-selection-owner-p (the former does
not exist)

- gui-selection-exists-p -> gui-backend-selection-exists-p (the
  former does not exist)

- gui-get-selection -> gui-backend-get-selection (the former
  does exist, but the later is lower lever, and I assumed that if the
  previous needed to be updated this one probably too)

- gui-set-selection -> gui-backend-set-selection (same as above)

> @@ -108,16 +109,24 @@ select-enable-primary
>    :group 'killing
>    :version "25.1")
>  
> -;; We keep track of the last text selected here, so we can check the
> -;; current selection against it, and avoid passing back our own text
> -;; from gui-selection-value.  We track both
> +;; We keep track of the last selection here, so we can check the
> +;; current selection against it, and avoid passing back with
> +;; gui-selection-value the same text we previously killed or
> +;; yanked. We track both
>  ;; separately in case another X application only sets one of them
>  ;; we aren't fooled by the PRIMARY or CLIPBOARD selection staying the same.
> +;;
> +;; TODO: add selection owner to fingerprint, since timestamp is not
> +;; always relieable? Probably not worth it, since right now we can't
> +;; get the owner with the low-level functions out of the box, and text
> +;; plus timestamp is probably a strong enough fingerprint already.
> +(defvar gui--last-clipboard-selection-fingerprint nil
> +  "The fingerprint of the CLIPBOARD selection last seen, which is a
> +list of value and timestamp.")
> +(defvar gui--last-primary-selection-fingerprint nil
> +  "The fingerprint of the PRIMARY selection last seen, which is a
> +list of value and timestamp.")
>  
> -(defvar gui--last-selected-text-clipboard nil
> -  "The value of the CLIPBOARD selection last seen.")
> -(defvar gui--last-selected-text-primary nil
> -  "The value of the PRIMARY selection last seen.")
>  
>  (defun gui-select-text (text)
>    "Select TEXT, a string, according to the window system.

- Using new variables gui--last-clipboard-selection-fingerprint and
  gui--last-primary-selection-fingerprint instead of
  gui--last-selected-text-clipboard and
  gui--last-selected-text-primary. They have the same purpose but hold
  the text and the timestamp instead of just the text, and I have
  updated the code everywhere they were referenced to use the
  text-timestamp pair instead of just the text . Better ideas for the
  names are welcome.

- Updated previous comment

- Just threw the idea that a better fingerprint would include the owner
  of the selection, if timestamps are sometimes not updated precisely
  when the owner changes, as Po Lu said.


> @@ -127,14 +136,16 @@ gui-select-text
>  MS-Windows does not have a \"primary\" selection."
>    (when select-enable-primary
>      (gui-set-selection 'PRIMARY text)
> -    (setq gui--last-selected-text-primary text))
> +    (setq gui--last-primary-selection-fingerprint
> +          (list text (gui-get-selection 'PRIMARY 'TIMESTAMP))))
>    (when select-enable-clipboard
>      ;; When cutting, the selection is cleared and PRIMARY
>      ;; set to the empty string.  Prevent that, PRIMARY
>      ;; should not be reset by cut (Bug#16382).
>      (setq saved-region-selection text)
>      (gui-set-selection 'CLIPBOARD text)
> -    (setq gui--last-selected-text-clipboard text)))
> +    (setq gui--last-clipboard-selection-fingerprint
> +          (list text (gui-get-selection 'CLIPBOARD 'TIMESTAMP)))))
>  (define-obsolete-function-alias 'x-select-text 'gui-select-text "25.1")
>  
>  (defcustom x-select-request-type nil

Saving the text-timestamp pair instead of just text.

> @@ -175,6 +186,7 @@ gui--selection-value-internal
>             ;; some other window systems.
>             (memq window-system '(x haiku))
>             (eq type 'CLIPBOARD)
> +           ;; Should we unify this with the DWIM logic?
>             (gui-backend-selection-owner-p type))
>      (let ((request-type (if (memq window-system '(x pgtk haiku))
>                              (or x-select-request-type

I consider that check to be conceptually part of the same DWIM logic,
and feel that maybe both checks should go together in the code. However
I decided to preserve the code structure for now, since I wanted to make
as little changes as possible, having the ownership check there can save
unnecessary computations, and the check is mostly redundant now
otherwise.


> @@ -194,33 +206,34 @@ gui--selection-value-internal
>  (defun gui-selection-value ()
>    (let ((clip-text
>           (when select-enable-clipboard
> -           (let ((text (gui--selection-value-internal 'CLIPBOARD)))
> +           (let ((text (gui--selection-value-internal 'CLIPBOARD))
> +                 (timestamp (gui-get-selection 'CLIPBOARD 'TIMESTAMP)))
>               (when (string= text "")
>                 (setq text nil))
> -             ;; When `select-enable-clipboard' is non-nil,
> -             ;; killing/copying text (with, say, `C-w') will push the
> -             ;; text to the clipboard (and store it in
> -             ;; `gui--last-selected-text-clipboard').  We check
> -             ;; whether the text on the clipboard is identical to this
> -             ;; text, and if so, we report that the clipboard is
> -             ;; empty.  See (bug#27442) for further discussion about
> -             ;; this DWIM action, and possible ways to make this check
> -             ;; less fragile, if so desired.
> +             ;; Check the CLIPBOARD selection for 'newness', i.e.,
> +             ;; whether it is different from the last time we did a
> +             ;; yank operation or whether it was set by Emacs itself
> +             ;; with a kill operation, since in both cases the text
> +             ;; will already be in the kill ring. See (bug#27442) and
> +             ;; (bug#53894) for further discussion about this DWIM
> +             ;; action, and possible ways to make this check less
> +             ;; fragile, if so desired.
>               (prog1
> -                 (unless (equal text gui--last-selected-text-clipboard)
> +                 (unless (equal (list text timestamp)  gui--last-clipboard-selection-fingerprint)
>                     text)
> -               (setq gui--last-selected-text-clipboard text)))))
> +               (setq gui--last-clipboard-selection-fingerprint (list text timestamp))))))
>          (primary-text
>           (when select-enable-primary
> -           (let ((text (gui--selection-value-internal 'PRIMARY)))
> +           (let ((text (gui--selection-value-internal 'PRIMARY))
> +                 (timestamp (gui-get-selection 'PRIMARY 'TIMESTAMP)))
>               (if (string= text "") (setq text nil))
>               ;; Check the PRIMARY selection for 'newness', is it different
>               ;; from what we remembered them to be last time we did a
>               ;; cut/paste operation.
>               (prog1
> -                 (unless (equal text gui--last-selected-text-primary)
> +                 (unless (equal (list text timestamp)  gui--last-primary-selection-fingerprint)
>                     text)
> -               (setq gui--last-selected-text-primary text))))))
> +               (setq gui--last-primary-selection-fingerprint (list text timestamp)))))))
>  
>      ;; As we have done one selection, clear this now.
>      (setq next-selection-coding-system nil)

- Using the timestamp-text pair instead of just text

- Updated comments

- I have tested the following cases:

  - Copy in another program -> C-y

  - Copy in other program -> C-y -> C-y -> M-y (the copied text is not
    duplicated in the kill ring, i.e., gui-selection-value returns nil
    for the second C-y)

  - C-k -> C-y -> M-y (the killed text is not duplicated in the kill
    ring, i.e., gui-selection-value returns nil)

  - Copy something in other program -> C-y -> M-y -> Copy the same thing
    in other program -> C-y (works as expected, i.e., the bug I reported
    is fixed)


> @@ -239,7 +252,8 @@ gui-selection-value
>      ;; timestamps there is no way to know what the 'correct' value to
>      ;; return is.  The nice thing to do would be to tell the user we
>      ;; saw multiple possible selections and ask the user which was the
> -    ;; one they wanted.
> +    ;; one they wanted. EDIT: We do have timestamps now, so we can
> +    ;; return the newer.
>      (or clip-text primary-text)
>      ))
>  

An unrelated issue which could be solved with timestamps, but the comment
must be old and says we don't have them. Just updated the comment saying
we do have them now.

> diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
> index 327d51f275..2ae3cbd8b2 100644
> --- a/lisp/term/pc-win.el
> +++ b/lisp/term/pc-win.el
> @@ -248,7 +248,7 @@ w16-selection-owner-p
>          ;; Windows clipboard.
>          (cond
>           ((not text) t)
> -         ((equal text gui--last-selected-text-clipboard) text)
> +         ((equal text (car gui--last-clipbaord-selection-fingerprint)) t)
>           (t nil)))))
>  

- Replaced the check

- Returned t instead of text

- Have not tested since I don't use Windows, but it's semantically
  equivalent as before.



[-- Attachment #2: Patch for bug#53894 --]
[-- Type: text/x-diff, Size: 9973 bytes --]

From a81da62327c58c8351c366e8530bf6250a4ffaa2 Mon Sep 17 00:00:00 2001
From: Ignacio <ignacio.decasso@imdea.org>
Date: Mon, 28 Feb 2022 12:13:52 +0100
Subject: [PATCH] fixed bug

---
 etc/DEVEL.HUMOR            |  2 +-
 lisp/menu-bar.el           |  4 +--
 lisp/obsolete/mouse-sel.el |  2 +-
 lisp/select.el             | 70 +++++++++++++++++++++++---------------
 lisp/term/pc-win.el        |  2 +-
 5 files changed, 47 insertions(+), 33 deletions(-)

diff --git a/etc/DEVEL.HUMOR b/etc/DEVEL.HUMOR
index bd51845cb1..e93631c331 100644
--- a/etc/DEVEL.HUMOR
+++ b/etc/DEVEL.HUMOR
@@ -203,4 +203,4 @@ it's still working the same way, and then whether it's still relevant
 due to changes elsewhere, and then finally whether it can be improved
 without breaking odd edge cases on obscure systems you don't have
 access to.  🙃"
-                                -- Ignacio Casso and Lars Ingebrigtsen
+                                -- Ignacio C. and Lars Ingebrigtsen
diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
index ab64928fe7..01683f5c9f 100644
--- a/lisp/menu-bar.el
+++ b/lisp/menu-bar.el
@@ -606,8 +606,8 @@ clipboard-yank
   "Insert the clipboard contents, or the last stretch of killed text."
   (interactive "*")
   (let ((select-enable-clipboard t)
-        ;; Ensure that we defeat the DWIM login in `gui-selection-value'.
-        (gui--last-selected-text-clipboard nil))
+        ;; Ensure that we defeat the DWIM logic in `gui-selection-value'.
+        (gui--last-clipboard-selection-fingerprint nil))
     (yank)))
 
 (defun clipboard-kill-ring-save (beg end &optional region)
diff --git a/lisp/obsolete/mouse-sel.el b/lisp/obsolete/mouse-sel.el
index a9d6bfee60..fc91cc9fc1 100644
--- a/lisp/obsolete/mouse-sel.el
+++ b/lisp/obsolete/mouse-sel.el
@@ -304,7 +304,7 @@ mouse-sel-get-selection-function
     (if (eq selection 'PRIMARY)
 	(or (gui-selection-value)
 	    (bound-and-true-p x-last-selected-text-primary)
-            gui--last-selected-text-primary)
+            gui--last-selected-text-primary) ;; this variable no longer exists. Does code in lisp/obsolete/ need to be mantained?
       (gui-get-selection selection)))
   "Function to call to get the selection.
 Called with one argument:
diff --git a/lisp/select.el b/lisp/select.el
index 42b50c44e6..55c409d347 100644
--- a/lisp/select.el
+++ b/lisp/select.el
@@ -25,9 +25,10 @@
 ;; Based partially on earlier release by Lucid.
 
 ;; The functionality here is divided in two parts:
-;; - Low-level: gui-get-selection, gui-set-selection, gui-selection-owner-p,
-;;   gui-selection-exists-p are the backend-dependent functions meant to access
-;;   various kinds of selections (CLIPBOARD, PRIMARY, SECONDARY).
+;; - Low-level: gui-backend-get-selection, gui-backend-set-selection,
+;;   gui-backend-selection-owner-p, gui-backend-selection-exists-p are
+;;   the backend-dependent functions meant to access various kinds of
+;;   selections (CLIPBOARD, PRIMARY, SECONDARY).
 ;; - Higher-level: gui-select-text and gui-selection-value go together to
 ;;   access the general notion of "GUI selection" for interoperation with other
 ;;   applications.  This can use either the clipboard or the primary selection,
@@ -108,16 +109,24 @@ select-enable-primary
   :group 'killing
   :version "25.1")
 
-;; We keep track of the last text selected here, so we can check the
-;; current selection against it, and avoid passing back our own text
-;; from gui-selection-value.  We track both
+;; We keep track of the last selection here, so we can check the
+;; current selection against it, and avoid passing back with
+;; gui-selection-value the same text we previously killed or
+;; yanked. We track both
 ;; separately in case another X application only sets one of them
 ;; we aren't fooled by the PRIMARY or CLIPBOARD selection staying the same.
+;;
+;; TODO: add selection owner to fingerprint, since timestamp is not
+;; always relieable? Probably not worth it, since right now we can't
+;; get the owner with the low-level functions out of the box, and text
+;; plus timestamp is probably a strong enough fingerprint already.
+(defvar gui--last-clipboard-selection-fingerprint nil
+  "The fingerprint of the CLIPBOARD selection last seen, which is a
+list of value and timestamp.")
+(defvar gui--last-primary-selection-fingerprint nil
+  "The fingerprint of the PRIMARY selection last seen, which is a
+list of value and timestamp.")
 
-(defvar gui--last-selected-text-clipboard nil
-  "The value of the CLIPBOARD selection last seen.")
-(defvar gui--last-selected-text-primary nil
-  "The value of the PRIMARY selection last seen.")
 
 (defun gui-select-text (text)
   "Select TEXT, a string, according to the window system.
@@ -127,14 +136,16 @@ gui-select-text
 MS-Windows does not have a \"primary\" selection."
   (when select-enable-primary
     (gui-set-selection 'PRIMARY text)
-    (setq gui--last-selected-text-primary text))
+    (setq gui--last-primary-selection-fingerprint
+          (list text (gui-get-selection 'PRIMARY 'TIMESTAMP))))
   (when select-enable-clipboard
     ;; When cutting, the selection is cleared and PRIMARY
     ;; set to the empty string.  Prevent that, PRIMARY
     ;; should not be reset by cut (Bug#16382).
     (setq saved-region-selection text)
     (gui-set-selection 'CLIPBOARD text)
-    (setq gui--last-selected-text-clipboard text)))
+    (setq gui--last-clipboard-selection-fingerprint
+          (list text (gui-get-selection 'CLIPBOARD 'TIMESTAMP)))))
 (define-obsolete-function-alias 'x-select-text 'gui-select-text "25.1")
 
 (defcustom x-select-request-type nil
@@ -175,6 +186,7 @@ gui--selection-value-internal
            ;; some other window systems.
            (memq window-system '(x haiku))
            (eq type 'CLIPBOARD)
+           ;; Should we unify this with the DWIM logic?
            (gui-backend-selection-owner-p type))
     (let ((request-type (if (memq window-system '(x pgtk haiku))
                             (or x-select-request-type
@@ -194,33 +206,34 @@ gui--selection-value-internal
 (defun gui-selection-value ()
   (let ((clip-text
          (when select-enable-clipboard
-           (let ((text (gui--selection-value-internal 'CLIPBOARD)))
+           (let ((text (gui--selection-value-internal 'CLIPBOARD))
+                 (timestamp (gui-get-selection 'CLIPBOARD 'TIMESTAMP)))
              (when (string= text "")
                (setq text nil))
-             ;; When `select-enable-clipboard' is non-nil,
-             ;; killing/copying text (with, say, `C-w') will push the
-             ;; text to the clipboard (and store it in
-             ;; `gui--last-selected-text-clipboard').  We check
-             ;; whether the text on the clipboard is identical to this
-             ;; text, and if so, we report that the clipboard is
-             ;; empty.  See (bug#27442) for further discussion about
-             ;; this DWIM action, and possible ways to make this check
-             ;; less fragile, if so desired.
+             ;; Check the CLIPBOARD selection for 'newness', i.e.,
+             ;; whether it is different from the last time we did a
+             ;; yank operation or whether it was set by Emacs itself
+             ;; with a kill operation, since in both cases the text
+             ;; will already be in the kill ring. See (bug#27442) and
+             ;; (bug#53894) for further discussion about this DWIM
+             ;; action, and possible ways to make this check less
+             ;; fragile, if so desired.
              (prog1
-                 (unless (equal text gui--last-selected-text-clipboard)
+                 (unless (equal (list text timestamp)  gui--last-clipboard-selection-fingerprint)
                    text)
-               (setq gui--last-selected-text-clipboard text)))))
+               (setq gui--last-clipboard-selection-fingerprint (list text timestamp))))))
         (primary-text
          (when select-enable-primary
-           (let ((text (gui--selection-value-internal 'PRIMARY)))
+           (let ((text (gui--selection-value-internal 'PRIMARY))
+                 (timestamp (gui-get-selection 'PRIMARY 'TIMESTAMP)))
              (if (string= text "") (setq text nil))
              ;; Check the PRIMARY selection for 'newness', is it different
              ;; from what we remembered them to be last time we did a
              ;; cut/paste operation.
              (prog1
-                 (unless (equal text gui--last-selected-text-primary)
+                 (unless (equal (list text timestamp)  gui--last-primary-selection-fingerprint)
                    text)
-               (setq gui--last-selected-text-primary text))))))
+               (setq gui--last-primary-selection-fingerprint (list text timestamp)))))))
 
     ;; As we have done one selection, clear this now.
     (setq next-selection-coding-system nil)
@@ -239,7 +252,8 @@ gui-selection-value
     ;; timestamps there is no way to know what the 'correct' value to
     ;; return is.  The nice thing to do would be to tell the user we
     ;; saw multiple possible selections and ask the user which was the
-    ;; one they wanted.
+    ;; one they wanted. EDIT: We do have timestamps now, so we can
+    ;; return the newer.
     (or clip-text primary-text)
     ))
 
diff --git a/lisp/term/pc-win.el b/lisp/term/pc-win.el
index 327d51f275..2ae3cbd8b2 100644
--- a/lisp/term/pc-win.el
+++ b/lisp/term/pc-win.el
@@ -248,7 +248,7 @@ w16-selection-owner-p
         ;; Windows clipboard.
         (cond
          ((not text) t)
-         ((equal text gui--last-selected-text-clipboard) text)
+         ((equal text (car gui--last-clipbaord-selection-fingerprint)) t)
          (t nil)))))
 
 ;; gui-set-selection is used in gui-set-selection.
-- 
2.25.1