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

Off-by-one in counsel-yank-pop selection #1371

Closed
ambihelical opened this issue Dec 11, 2017 · 10 comments

Comments

@ambihelical
Copy link
Contributor

commented Dec 11, 2017

If I understand the recent changes correctly, the current kill-ring-yank-pointer position should be preselected. This seems to always be one off. If I have three lines which I've killed in order:

one
two
three

After killing "one", the counsel-yank-pop shows "one" and it is preselected.
After killing "two":

    two <- should be selected
    one <- selected

After killing "three":

    three  <- should be selected
    two  <- selected
    one

When I use the "rotate" command to change the current kill-ring-yank-pointer, it never sets the selection to the item selected, but is always one off. If I select "three", and use rotate (M-o r) to set the pointer, the next counsel-yank-pop shows "two" selected. If I select "two" and rotate, the next counsel-yank-pop shows "one" selected. If I select "one", the next invocation shows "three" selected.

For this issue I was using commit 5c096a2.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 11, 2017

@basil-conto Can you have a look? Off-by-one results from the change in interactive spec - arg is usually 1 when it's expected to be zero.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

TL;DR

Preselecting the next-to-last kill is intended.

Rationale

The recent changes to counsel-yank-pop try to mirror the default yank, yank-pop, rotate-yank-pointer, etc. functionality as close as possible, whilst providing convenient realtime visualisation/completion and custom actions via the Ivy/Counsel interface.

As (emacs) Yanking explains in further detail, the general idea is that yank retrieves the last kill, whereas yank-pop is for accessing earlier kills. This is also summarised in the docstring of yank-pop:

yank-pop is an interactive compiled Lisp function in ‘simple.el’.

It is bound to M-y.

(yank-pop &optional ARG)

Replace just-yanked stretch of killed text with a different stretch.
This command is allowed only immediately after a ‘yank’ or a ‘yank-pop’.
At such a time, the region contains a stretch of reinserted
previously-killed text.  ‘yank-pop’ deletes that text and inserts in its
place a different stretch of killed text.

With no argument, the previous kill is inserted.
With argument N, insert the Nth previous kill.
If N is negative, this is a more recent kill.

The sequence of kills wraps around, so that after the oldest one
comes the newest one.

This command honors the ‘yank-handled-properties’ and
‘yank-excluded-properties’ variables, and the ‘yank-handler’ text
property, in the way that ‘yank’ does.

The optional prefix argument defaults to 1, meaning yank the next-to-last kill. If you want to yank the last kill, you should pass it a prefix argument of zero.

The current implementation of counsel-yank-pop mirrors this by preselecting what vanilla yank-pop would have yanked. So it doesn't "select" the "wrong" candidate, it "preselects" the "default" candidate.

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 11, 2017

@basil-conto Thanks for the explanation. It makes sense in a way.

Yet in another way it doesn't. Using M-y RET instead of C-y seems like a nice use case. Of course, if you did M-w seconds ago, you know what's on the top of the kill ring and you can do C-y directly. But if you're just fairly sure that the thing you want is on the top of the kill ring, M-y RET gives you a preview, an option to back out with C-g, and an option to select something else.

Currently, the above use case was moved to the M-y C-p RET binding. I'm not sure it's worth the consistency with the defaults.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

It makes sense in a way. Yet in another way it doesn't.

If you'll pardon the facetious retort, it doesn't make sense to discuss whether this makes sense. :)

Here's my understanding of why that is. Please to confirm/comment.

  • Emacs provides intricate, central and well-documented idioms and interfaces around killing and yanking, with defaults and conventions which support a particular workflow.
  • Counsel can extend the user-friendliness and flexibility of many such systems via Ivy filtering, actions, etc.
  • The command counsel-yank-pop previously did no preselecting, meaning the first completion candidate was preselected by default.
  • The command's recent reimplementation was intended to reuse the existing Emacs system but make it more Counsel-y. Part of using the existing system is respecting the yank-pop idiom of doing what's in the name and popping the kill-ring. This was not just to satisfy the author's compulsive pedantry, but also to satisfy a user request (#1190).
  • Now, users (and owner :) seem (because no-one has stated it clearly) to be requesting that Counsel should return to and/or also support preselecting the last kill.

My conclusion is that, as is always the case, different people have different preferences, so we should simply do the Emacs-y and Counsel-y thing and add another user toggle which defaults to the old, non-Emacs behaviour, right?

@dieggsy

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2017

@basil-conto A user toggle/custom var seems like a good solution here

@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 11, 2017

@basil-conto You're right. counsel-yank-pop does say

"Ivy replacement for `yank-pop'.

Could you add a defcustom for this, with the current behavior as the default?

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

@abo-abo

Could you add a defcustom for this

I can't promise I can, but I can promise to try. :)

with the current behavior as the default

Are you sure you don't prefer the old behaviour as default?

basil-conto added a commit to basil-conto/swiper that referenced this issue Dec 11, 2017
counsel.el (counsel-yank-pop): Add preselect toggle
(counsel-yank-pop-preselect-last): New defcustom.
(counsel-yank-pop): Use it.

Fixes abo-abo#1371
@abo-abo

This comment has been minimized.

Copy link
Owner

commented Dec 12, 2017

Are you sure you don't prefer the old behaviour as default?

I was planning to customize it in my config, in the name of counsel-yank-pop staying true to its name.

We can change it later if needed.

@articuluxe

This comment has been minimized.

Copy link

commented Dec 15, 2017

@basil-conto I just wanted to say, thanks for this change. It makes this feature usable for me and it is a big improvement IMO to make the functionality consistent with standard emacs usage.

@basil-conto

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2017

@articuluxe Glad it's useful for you; thanks are due to others, though. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.