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

Previous Next

Full log

Message #146 received at 53894 <at> debbugs.gnu.org (full text, mbox):

From: Ignacio Casso <ignaciocasso <at> hotmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: luangruo <at> yahoo.com, larsi <at> gnus.org, 53894 <at> debbugs.gnu.org
Subject: Re: bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard
 copy does not push to kill ring
Date: Tue, 01 Mar 2022 16:29:36 +0100

[Message part 1 (text/plain, inline)]

> 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 (text/x-diff, attachment)]

[Message part 3 (text/plain, inline)]

> ---
>  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.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.