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

[Ignacio Casso <ignaciocasso@hotmail.com>]

>> Can't you put a `timestamp' text property on strings stored inside the
>> old gui--last-selected-text-clipboard variable instead?
>
>
> Good idea. I'll implement it and send the new patch.

I have implemented this, but I must admit I have never worked with text
properties before so I might have failed to consider unexpected effects
that these changes could have in other parts of the code. I also think
that using symbol properties for
gui--last-selected-text-clipboard/primary would be better, since a)
using text properties implies some checks that the variable values are
actually strings, and b) those text properties could be modified
elsewhere, since we don't know with which other elisp code those string
are shared. In particular, it could be the case that
gui--last-selected-text-clipboard and gui--last-selected-text-primary
shared the string (although if that was a problem it could easily be
solved by using different timestamp property names for clipboard and
primary). I've never worked with either text or symbol properties
before, so I'm not sure. What do you think?

I attach the new patch anyway and comment each section below:

>  lisp/select.el | 57 ++++++++++++++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/lisp/select.el b/lisp/select.el
> index 42b50c44e6..e4d3fc8bd0 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,

- Same as previous patch

> @@ -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, same as previous patch

> @@ -127,6 +129,8 @@ gui-select-text
>  MS-Windows does not have a \"primary\" selection."
>    (when select-enable-primary
>      (gui-set-selection 'PRIMARY text)
> +    (when (stringp text)
> +      (propertize text 'timestamp (gui-get-selection 'PRIMARY 'TIMESTAMP)))
>      (setq gui--last-selected-text-primary text))
>    (when select-enable-clipboard
>      ;; When cutting, the selection is cleared and PRIMARY

- Put timestamp property in text. Suggestions for the property name are
  welcome.

- Previously, calling (gui-select-text value) with a value that was not
  a string did not produce an error, so I make the stringp check to
  preserve that behavior although that situation should never happen.

> @@ -134,6 +138,8 @@ gui-select-text
>      ;; should not be reset by cut (Bug#16382).
>      (setq saved-region-selection text)
>      (gui-set-selection 'CLIPBOARD text)
> +    (when (stringp text)
> +      (propertize text 'timestamp (gui-get-selection 'CLIPBOARD 'TIMESTAMP)))
>      (setq gui--last-selected-text-clipboard text)))
>  (define-obsolete-function-alias 'x-select-text 'gui-select-text "25.1")

Same as above

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

Same as previous patch

> @@ -194,32 +201,41 @@ 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))
> +                 (last-ts (when gui--last-selected-text-clipboard
> +                            (get-text-property 0 'timestamp gui--last-selected-text-clipboard))))

- getting new and old timestamp

- Is that the right way to get the property?

>               (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, same as previous patch

>               (prog1
> -                 (unless (equal text gui--last-selected-text-clipboard)
> +                 (unless (and (equal text gui--last-selected-text-clipboard)
> +                              (eq timestamp last-timestamp))
>                     text)
> +               (when text (propertize text 'timestamp timestamp))
>                 (setq gui--last-selected-text-clipboard text)))))

check text and timestamp equality, and update timestamp property

>          (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))
> +                 (last-timestamp (when gui--last-selected-text-primary
> +                            (get-text-property 0 'timestamp gui--last-selected-text-primary))))
>               (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 (and (equal text gui--last-selected-text-primary)
> +                              (eq timestamp last-timestamp))
>                     text)
> +               (when text (put-text-property 0 1 'timestamp timestamp text))
>                 (setq gui--last-selected-text-primary text))))))
>  
>      ;; As we have done one selection, clear this now.

Same changes but for PRIMARY instead of CLIPBOARD

> @@ -239,7 +255,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)
>      ))

Same as previous patch