[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy doe
From: |
Ignacio Casso |
Subject: |
bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring |
Date: |
Mon, 28 Feb 2022 13:12:44 +0100 |
User-agent: |
mu4e 1.6.10; emacs 29.0.50 |
>> 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.
0001-fixed-bug.patch
Description: Patch for bug#53894
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, (continued)
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/10
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/11
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/27
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/27
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring,
Ignacio Casso <=
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Eli Zaretskii, 2022/02/28
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/28
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/28
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/28
- Message not available
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/28
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Po Lu, 2022/02/28
- Message not available
- Message not available
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/28
- Message not available
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/11
- Message not available
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Ignacio Casso, 2022/02/10
- bug#53894: 27.2; Clipboard copy -> C-y -> M-y -> Same clipboard copy does not push to kill ring, Lars Ingebrigtsen, 2022/02/11