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

[Ignacio Casso]

> Not just comment: we should document that every implementation of
> gui-get-selection should return either a valid timestamp (and say what
> is a valid timestamp) or nil, meaning 'CLIPBOARD 'TIMESTAMP is
> unsupported.

You are right that it is a dangerous assumption, and it's also hard to
test and probably unnecessary. So I have changed the code to check the
window-system variable instead and only use timestamps for X and haiku
(I don't actually know the list of systems that support it, I have
included haiku because it is also in the list of systems that support
reliably gui-backend-selection-owner-p, according to the code).

I also have defined the functions
gui--set-last-clipboard/primary-selection and
gui--clipboard/primary-selection-unchanged-p to just change the lines
(setq gui--last-selected-text-clipboard/primary text) and (equal text
gui--last-selected-text-clipboard/primary) with them. They behave
exactly the same for systems other than X and haiku, and are backwards
compatible with setting or reading directly the original
gui--last-selected-text-clipboard/primary variable in other places.

I have assumed this last thing (being backwards compatible) was the
reason why Po Lu suggested to use text properties instead of changing
the gui--last-selected-text-clipboard/primary variable, so I have taken
the liberty of moving away from that idea and using the new variables
gui--last-selection-timestamp-clipboard and
gui--last-selection-timestamp-primary instead, which simplify the code.

So here is the final, hopefully definitive patch, attached to the email
and commented below:

0001-same-fix-but-with-functions.patch
Description: Patch for bug#53894

> ---
>  lisp/menu-bar.el    |  2 +-
>  lisp/select.el      | 91 ++++++++++++++++++++++++++++++++++-----------
>  lisp/term/pc-win.el |  8 ++++
>  3 files changed, 78 insertions(+), 23 deletions(-)
> 
> diff --git a/lisp/menu-bar.el b/lisp/menu-bar.el
> index ab64928fe7..7ff5439f7c 100644
> --- a/lisp/menu-bar.el
> +++ b/lisp/menu-bar.el
> @@ -606,7 +606,7 @@ 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'.
> +        ;; Ensure that we defeat the DWIM logic in `gui-selection-value'.
>          (gui--last-selected-text-clipboard nil))
>      (yank)))

- DWIM login -> DWIM logic (typo)

- (let ((gui--last-selected-text-clipboard nil))) still achieves the
  same after the changes

> diff --git a/lisp/select.el b/lisp/select.el
> index 42b50c44e6..708034f6bd 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,9 +109,10 @@ 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.
>  

- Updated comment

> @@ -119,6 +121,50 @@ gui--last-selected-text-clipboard
>  (defvar gui--last-selected-text-primary nil
>    "The value of the PRIMARY selection last seen.")
>  
> +(defvar gui--last-selection-timestamp-clipboard nil
> +  "The timestamp of the CLIPBOARD selection last seen.")
> +(defvar gui--last-selection-timestamp-primary nil
> +  "The timestamp of the PRIMARY selection last seen.")
> +
> +(defun gui--set-last-clipboard-selection (text)
> +  "Save last clipboard selection, to be able to check later whether
> +it has changed. Save the selected text, and for window systems
> +that support it, the selection timestamp."
> +  (setq gui--last-selected-text-clipboard text)
> +  (when (memq window-system '(x haiku))
> +    (setq gui--last-selection-timestamp-clipboard
> +          (gui-get-selection 'CLIPBOARD 'TIMESTAMP))))
> +
> +(defun gui--set-last-primary-selection (text)
> +  "Save last primary selection, to be able to check later whether
> +it has changed. Save the selected text, and for window systems
> +that support it, the selection timestamp."
> +  (setq gui--last-selected-text-primary text)
> +  (when (memq window-system '(x haiku))
> +    (setq gui--last-selection-timestamp-primary
> +          (gui-get-selection 'PRIMARY 'TIMESTAMP))))
> +
> +(defun gui--clipboard-selection-unchanged-p (text)
> +  "Check whether the clipboard selection is the same as the last
> +time it was read, by comparing the selected text, and for window
> +systems that support it, the selection timestamp."
> +  (and
> +   (equal text gui--last-selected-text-clipboard)
> +   (or (not (memq window-system '(x haiku)))
> +       (eq gui--last-selection-timestamp-clipboard
> +           (gui-get-selection 'CLIPBOARD 'TIMESTAMP)))))
> +
> +(defun gui--primary-selection-unchanged-p (text)
> +  "Check whether the primary selection is the same as the last
> +time it was read, by comparing the selected text, and for window
> +systems that support it, the selection timestamp."
> +  (and
> +   (equal text gui--last-selected-text-primary)
> +   (or (not (memq window-system '(x haiku)))
> +       (eq gui--last-selection-timestamp-primary
> +           (gui-get-selection 'PRIMARY 'TIMESTAMP)))))
> +
> +
>  (defun gui-select-text (text)
>    "Select TEXT, a string, according to the window system.
>  if `select-enable-clipboard' is non-nil, copy TEXT to the system's clipboard.

- New variables gui--last-selection-timestamp-clipboard and
  gui--last-selection-timestamp-primary.

- New functions gui--set-last-clipboard-selection and
  gui--set-last-primary-selection.
  
- New functions gui--clipboard-selection-unchanged-p and
  gui--primary-selection-unchanged-p.

> @@ -127,14 +173,14 @@ 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))
> +    (gui--set-last-primary-selection text))
>    (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)))
> +    (gui--set-last-clipboard-selection text)))
>  (define-obsolete-function-alias 'x-select-text 'gui-select-text "25.1")
>  
>  (defcustom x-select-request-type nil

Using new functions instead

> @@ -175,6 +221,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.


> @@ -197,19 +244,18 @@ gui-selection-value
>             (let ((text (gui--selection-value-internal 'CLIPBOARD)))
>               (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.

Updated comment

>               (prog1
> -                 (unless (equal text gui--last-selected-text-clipboard)
> +                 (unless (gui--clipboard-selection-unchanged-p text)
>                     text)
> -               (setq gui--last-selected-text-clipboard text)))))
> +               (gui--set-last-clipboard-selection text)))))
>          (primary-text
>           (when select-enable-primary
>             (let ((text (gui--selection-value-internal 'PRIMARY)))

Using new functions instead

> @@ -218,9 +264,9 @@ gui-selection-value
>               ;; 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 (gui--primary-selection-unchanged-p text)
>                     text)
> -               (setq gui--last-selected-text-primary text))))))
> +               (gui--set-last-primary-selection text))))))
>  
>      ;; As we have done one selection, clear this now.
>      (setq next-selection-coding-system nil)

Using new functions instead

> @@ -239,7 +285,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 (for most
> +    ;; window systems), 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..289a1414c6 100644
> --- a/lisp/term/pc-win.el
> +++ b/lisp/term/pc-win.el

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.


> @@ -246,6 +246,14 @@ w16-selection-owner-p
>          ;; if it does not exist, or exists and compares
>          ;; equal with the last text we've put into the
>          ;; Windows clipboard.
> +        ;; EDIT: that variable is actually the last text any program
> +        ;; (not just Emacs) has put into the windows clipboard (up
> +        ;; until the last time Emacs read or set the clipboard), so
> +        ;; it's not suitable for checking actual selection
> +        ;; ownership. This does not result in a bug for any of the
> +        ;; current uses of gui-backend-selection-owner however, since
> +        ;; they don't actually care about selection ownership, but
> +        ;; about the selected text having changed.
>          (cond
>           ((not text) t)
>           ((equal text gui--last-selected-text-clipboard) text)
> -- 

A comment on the only other use of gui--last-selected-text-clipboard I
found, since it is and was incorrect. It behaves exactly the same after
the new changes though.