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

Overhaul counsel-yank-pop #1356

Closed
wants to merge 4 commits into from
Closed

Conversation

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Dec 8, 2017

Changelog

Please see each commit message in order. The last and most significant commit in particular addresses this Emacs SE question and closes #1190.

Before merging

The function counsel--yank-pop-position is also useful in #1133. If that PR lands first, the function should be removed from this PR. Conversely, if this PR lands first, then #1133 should be rebased to avail of the function.

Question

Is completely ignoring blank kills really that useful? Thoughts on lifting this restriction or making it configurable?

basil-conto added 3 commits Dec 8, 2017
- Hoist common branch code.
- Heed carriage returns.
- Consolidate nested lets.
Do not unnecessarily modify kill-ring structure and elements.
@basil-conto basil-conto force-pushed the basil-conto:yank-pop branch 2 times, most recently Dec 8, 2017
counsel.el Outdated
(setq ivy-completion-end (point))
(ivy-read "kill-ring: " cands
:require-match t
:preselect (nth (or arg 1) kill-ring-yank-pointer)

This comment has been minimized.

@basil-conto

basil-conto Dec 8, 2017
Author Collaborator

Woops, this is wrong for negative ARGs; I'll fix it later today.

counsel.el Outdated
(ivy-read "kill-ring: " candidates
:action 'counsel-yank-pop-action
:caller 'counsel-yank-pop))))
(string-match "\\`[\n\r[:blank:]]*\\'" s))

This comment has been minimized.

@ambihelical

ambihelical Dec 8, 2017
Contributor

Potentially replaceable with string-blank-p

counsel.el Outdated
(delete-dups kill-ring))))
(let ((cands (delete-dups
(cl-mapcan (lambda (s)
(unless (string-match-p "\\`[\n\r[:blank:]]*\\'" s)

This comment has been minimized.

@ambihelical

ambihelical Dec 8, 2017
Contributor

potentially use string-blank-p here.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Dec 8, 2017

@ambihelical I would have used string-blank-p too, but the [:blank:] character class matches Unicode whitespace in addition to just tabs and spaces in Emacs 25+. Probably string-blank-p should be updated to use [:blank:].

I'm more concerned about whether we should unconditionally be removing all blank kills from the candidates offered.

@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Dec 8, 2017

While trying to fix the :preselect calculation, I realised that filtering kill-ring elements only for the sake of Ivy completion is Doing The Wrong Thing. In particular, it makes relative offsets into the kill-ring inconsistent and meaningless and does not integrate nicely with Emacs.

Nevertheless, I understand why users may want to ignore certain kill-ring elements. I can think of three approaches counsel.el can take in order to Do The Right Thing:

1. Leave the kill ring alone

(elisp) Kill Functions explains that Emacs offers mechanisms for filtering killed text which users can modify to suit their preferences, though I'm not sure whether these can omit blank strings altogether.

Pros:

  • Less to worry about.
  • Consistent and reliable candidate preselecting.
  • Consistency and integration with broader kill ring ecosystem.

Cons:

  • Less convenient for and friendly to users.
  • Potentially offers no automated way to ignore blank kills.

2. Modify the kill ring; long live the kill ring!

If counsel.el does not want to offer blank kills during completion, it should completely and destructively remove them from the kill ring. Perhaps a regexp/function user option can be provided for customising the filtering operation.

Pros:

  • Consistent and reliable candidate preselecting. Kind of; the user has to keep in mind that relative positions into the kill ring skip blank kills.
  • Consistency and integration with broader kill ring ecosystem.

Cons:

  • Some users (including myself) are opposed to / squeamish about modifying the kill ring.
  • Intrusive / non-Emacsy.

3. All of the above

Provide a toggle between leaving the kill ring untouched and destructively filtering it.

Pros:

  • Union of pros of approaches 1 and 2.

Cons:

  • Intersection of cons of approaches 1 and 2.

My personal favorite is 3, for which I'd be happy to provide the code. Thoughts?

@basil-conto basil-conto force-pushed the basil-conto:yank-pop branch Dec 8, 2017
@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Dec 8, 2017

I've fixed the :preselect calculation for now with a bit of a hack.

The command yank-pop does too much under the bonnet to ever hope to
emulate, so use it directly.  This respects and updates various
internals, most noticeably kill-ring-yank-pointer, thus allowing
consecutive calls to yank to work.
See https://emacs.stackexchange.com/q/37351/15748.

(counsel--yank-pop-position): New function.
(counsel-yank-pop-action): Use yank-pop.
(counsel-yank-pop): Do The Right Thing w.r.t. point and mark
regardless of whether last-command was a yank.  Adopt
interactive-spec of yank-pop and preselect candidate accordingly.
Abort on empty/blank kill-ring.

Fixes #1190
@basil-conto basil-conto force-pushed the basil-conto:yank-pop branch to c1941f0 Dec 8, 2017
@abo-abo abo-abo closed this in 50aa561 Dec 8, 2017
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 8, 2017

Thanks.

I realised that filtering kill-ring elements only for the sake of Ivy completion is Doing The Wrong Thing

You mean deleting blanks from the kill-ring? I have no problem with doing that. If it's an issue for you, PR with removing blanks non-destructively.

@basil-conto basil-conto deleted the basil-conto:yank-pop branch Dec 8, 2017
@basil-conto
Copy link
Collaborator Author

@basil-conto basil-conto commented Dec 8, 2017

You mean deleting blanks from the kill-ring?

Not exactly. What I think is The Wrong Thing is a) not giving users the choice to complete kill-ring contents without any filtering; and b) having differences between the contents of kill-ring and the completion collection which counsel-yank-pop offers.

Point (a) is simply about user preference. Point (b), on the other hand, is a more subtle problem because many kill/yank commands operate in terms of relative offsets of kill-ring-yank-pointer within kill-ring. When, for example, counsel-yank-pop only displays half the contents of kill-ring, then commands like rotate-yank-pointer, which operate on an unfiltered kill-ring, are likely to give unexpected results.

This is why I think it is important to accompany filtering of kill-ring with actual deletion of filtered elements, so that all kill/yank commands remain consistent and correct.

If that sounds good to you I'll submit another PR to allow toggling kill-ring filtering and accompanying filtering with actual deletion.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Dec 9, 2017

If that sounds good to you I'll submit another PR to allow toggling kill-ring filtering and accompanying filtering with actual deletion.

Thanks for the explanation. Please PR, with deletion on by default.

basil-conto added a commit to basil-conto/swiper that referenced this pull request Dec 10, 2017
(counsel-string-non-blank-p): New function.
(counsel-yank-pop-filter): New defcustom defaulting to it.
(counsel--yank-pop-kills): Use it to destructively filter and
uniquify kill-ring and kill-ring-yank-pointer elements.
(counsel-yank-pop-action-remove, counsel-yank-pop):
Consistently test equivalence with equal-including-properties.

Re: abo-abo#1356
abo-abo added a commit that referenced this pull request Dec 10, 2017
(counsel-string-non-blank-p): New function.
(counsel-yank-pop-filter): New defcustom defaulting to it.
(counsel--yank-pop-kills): Use it to destructively filter and
uniquify kill-ring and kill-ring-yank-pointer elements.
(counsel-yank-pop-action-remove, counsel-yank-pop):
Consistently test equivalence with equal-including-properties.

Re: #1356
Fixes #1367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants