Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve yank pop #1523

Merged
merged 2 commits into from Apr 9, 2018
Merged

Improve yank pop #1523

merged 2 commits into from Apr 9, 2018

Conversation

DamienCassou
Copy link
Contributor

@DamienCassou DamienCassou commented Apr 5, 2018

  • Allow counsel-yank-pop in read-only buffers
  • Give a chance to interprogram-paste-function to alter the kill-ring

/cc @basil-conto

counsel.el Outdated
@@ -3255,6 +3255,8 @@ All blank strings are deleted from `kill-ring' by default."
"Return list of kills for `counsel-yank-pop' to complete.
Returned elements satisfy `counsel-yank-pop-filter' and are
unique under `equal-including-properties'."
;; Refresh kill-ring in the presence of `interprogram-paste-function'
Copy link
Collaborator

@basil-conto basil-conto Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should quote kill-ring as you do interprogram-paste-function.

counsel.el Outdated
@@ -3255,6 +3255,8 @@ All blank strings are deleted from `kill-ring' by default."
"Return list of kills for `counsel-yank-pop' to complete.
Returned elements satisfy `counsel-yank-pop-filter' and are
unique under `equal-including-properties'."
;; Refresh kill-ring in the presence of `interprogram-paste-function'
(current-kill 0)
Copy link
Collaborator

@basil-conto basil-conto Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced it's a good idea to unconditionally allow kill-ring to be prepended to every time counsel--yank-pop-kills is called.

One of the problems is that current-kill throws an error if kill-ring is empty and interprogram-paste-function is or returns nil.

Another problem is that other callers, such as counsel-yank-pop-action-rotate, do not take this behaviour of counsel--yank-pop-kills into account.

Yet another problem is that, with vanilla yank/yank-pop, interprogram paste(s) are only prepended to kill-ring when:

  • yank, yank-pop, or rotate-yank-pointer is called with a zero prefix argument; or
  • save-interprogram-paste-before-kill is non-nil and a kill is performed.

So, before continuing, we must decide whether to:

  1. honour the vanilla Emacs behaviour of only prepending to kill-ring in certain cases, as described above; or
  2. always prepend to kill-ring before any counsel-yank-pop-related action.

I am in favour of (2), but I wanted to make sure that everyone is aware of and agrees with this divergence from vanilla Emacs behaviour.

counsel.el Outdated
@@ -3320,7 +3322,7 @@ can be controlled with `counsel-yank-pop-preselect-last', which
see. See also `counsel-yank-pop-filter' for how to filter
candidates.
Note: Duplicate elements of `kill-ring' are always deleted."
(interactive "*P")
(interactive "P")
Copy link
Collaborator

@basil-conto basil-conto Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the intention behind this? Browsing the kill-ring without inserting anything? If so, I like the idea, but counsel-yank-pop-action shouldn't be allowed to insert anything into a read-only buffer.

Copy link
Collaborator

@basil-conto basil-conto Apr 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose the following:

From fd32e4487ff3fcda54256609cf07c1813415c855 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Sun, 8 Apr 2018 18:58:58 +0100
Subject: Allow browsing kill-ring in read-only buffers

counsel.el (counsel-yank-pop):
Move barf-if-buffer-read-only from interactive spec here...
(counsel-yank-pop-action): ...to here.
---
 counsel.el | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/counsel.el b/counsel.el
index 986b488..d790996 100644
--- a/counsel.el
+++ b/counsel.el
@@ -3266,8 +3266,11 @@ counsel--yank-pop-kills
           kill-ring))
 
 (defun counsel-yank-pop-action (s)
-  "Like `yank-pop', but insert the kill corresponding to S."
+  "Like `yank-pop', but insert the kill corresponding to S.
+Signal a `buffer-read-only' error if called from a read-only
+buffer position."
   (with-ivy-window
+    (barf-if-buffer-read-only)
     (setq last-command 'yank)
     (setq yank-window-start (window-start))
     (yank-pop (counsel--yank-pop-position s))
@@ -3324,7 +3327,8 @@ counsel-yank-pop
 Note: Duplicates and elements not satisfying
 `counsel-yank-pop-filter' are destructively removed from
 `kill-ring' prior to completion."
-  (interactive "*P")
+  ;; Do not specify `*' to allow browsing `kill-ring' in read-only buffers
+  (interactive "P")
   (let ((ivy-format-function #'counsel--yank-pop-format-function)
         (ivy-height counsel-yank-pop-height)
         (kills (counsel--yank-pop-kills)))
-- 
2.16.3

Copy link
Owner

@abo-abo abo-abo Apr 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with applying the patch by @basil-conto, please open a new PR.
@DamienCassou does this patch also address your concerns?

counsel.el (counsel-yank-pop):
Move barf-if-buffer-read-only call from here...
(counsel-yank-pop-action): ...to here.
* counsel.el (counsel--yank-pop-kills): Refresh kill-ring before
getting the list of kills.
@DamienCassou
Copy link
Contributor Author

@DamienCassou DamienCassou commented Apr 9, 2018

@basil-conto I took care of all your actionable comments and put your patch instead of mine.

Because I'm using Exwm and the kill-ring outside Emacs buffers (e.g., Firefox), I need counsel-yank-pop to update the kill-ring when called. I then agree with you regarding choosing solution 2.

What should I do then?

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Apr 9, 2018

@DamienCassou

I took care of all your actionable comments and put your patch instead of mine.

Thanks.

What should I do then?

In looking into this, I have managed to go down a rabbit hole of Emacs bugs and lacking documentation. I have already reported one minor bug (bug#31097) and am preparing to submit another tonight, where current-kill causes interprogram-paste-function to be called (N + 1) times, where N is the number of strings returned by interprogram-paste-function.

My point is that it will take me at least another day or two to come up with a robust alternative to current-kill for counsel-yank-pop, unless someone else suggests something first.

If you're in a hurry, @abo-abo agrees, and neither of you mind the quite contrived/rare edge-case issues outlined above, then this PR has my blessing and I will try to follow-up ASAP with a new PR.

@abo-abo abo-abo merged commit 9c3c382 into abo-abo:master Apr 9, 2018
1 of 2 checks passed
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Apr 9, 2018

Thanks all.

follow-up ASAP with a new PR.

Thanks in advance.

basil-conto added a commit to basil-conto/swiper that referenced this issue Apr 25, 2018
(counsel--yank-pop-kills): Clarify docstring.  Don't fail if
kill-ring and result of interprogram-paste-function are both nil.
Remove obsolete ivy-cleanup-string filtering.
(counsel-yank-pop-action): Avoid unexpected additions to kill-ring.
(counsel-yank-pop-action-remove):
Remove redundant call to counsel--yank-pop-kills.
(counsel-yank-pop-action-rotate): Allow forced update of window
system selection even when yank pointer rotation is a no-op.
Remove unnecessary ivy-state-collection update.
Avoid unexpected additions to kill-ring.
(counsel-yank-pop): Remove redundant varbinds.

Re: abo-abo#1523
@DamienCassou DamienCassou deleted the improve-yank-pop branch Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants