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

Interoperate better with kill-ring #1133

Closed
wants to merge 1 commit into from
Closed

Conversation

ambihelical
Copy link
Contributor

@ambihelical ambihelical commented Aug 2, 2017

Make counsel-yank-pop inter-operate better with the
existing variable kill-ring-yank-pointer

  • Don't let delete leave kill-ring-yank-pointer dangling
  • Add action to set kill-ring-yank-pointer to selection

Make counsel-yank-pop inter-operate better with the
existing variable kill-ring-yank-pointer

* Don't let delete leave kill-ring-yank-pointer dangling
* Add action to set kill-ring-yank-pointer to selection
(setq kill-ring (delete s kill-ring)))
(let ((next-ptr (cdr (member s kill-ring))))
(setq kill-ring (delete s kill-ring))
(setq kill-ring-yank-pointer (or next-ptr kill-ring))))
Copy link
Collaborator

@basil-conto basil-conto Dec 8, 2017

Choose a reason for hiding this comment

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

I think it's sufficient to write

(setq kill-ring (delete s kill-ring))
(setq kill-ring-yank-pointer (delete s kill-ring-yank-pointer))

The docstring should also read "Remove all occurences of S from the kill ring."


(defun counsel-yank-pop-action-position (s)
"Set yank position to S."
(setq kill-ring-yank-pointer (member s kill-ring)))
Copy link
Collaborator

@basil-conto basil-conto Dec 8, 2017

Choose a reason for hiding this comment

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

In Emacs this is called "rotating" the yank pointer, so I think we should also say "rotate" instead of "position".

Modifying the kill ring and its pointer directly is not usually kosher, as killing and yanking are very central, configurable and flexible features of Emacs. We should, therefore, delegate as much as possible to internal functions:

(defun counsel--yank-pop-position (s)
  "Return position of S in `kill-ring' relative to last yank.
S must exist in `kill-ring'."
  (or (cl-position s kill-ring-yank-pointer :test #'equal-including-properties)
      (+ (cl-position s kill-ring :test #'equal-including-properties)
         (- (length kill-ring-yank-pointer)
            (length kill-ring)))))

(defun counsel-yank-pop-action-rotate (s)
  "Rotate the yanking point to S in the kill ring.
See `current-kill' for how this interacts with the window system
selection."
  (current-kill (counsel--yank-pop-position s)))

Adding counsel--yank-pop-position would make this PR a borderline non-trivial contribution (right @abo-abo ?), so you may need to assign your copyright to the FSF first. Otherwise you can hope that and wait until my next PR lands which needs to define counsel--yank-pop-position anyway. :)

Copy link
Collaborator

@basil-conto basil-conto Dec 8, 2017

Choose a reason for hiding this comment

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

The promised PR now lives at #1356. If that PR lands first, then this one should be rebased to avail of counsel--yank-pop-position. Conversely, if this PR lands first, #1356 should be rebased to have the function removed.

Copy link
Contributor Author

@ambihelical ambihelical Dec 8, 2017

Choose a reason for hiding this comment

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

I don't have any great desire to do that paperwork, otherwise I'd probably contribute more to this project, in fact I cut this PR down to make it fit under the total limit. So, anything you can do first I'm OK with. I'll examine your suggestions later, I'm preoccupied atm. But you are probably right that it could be simpler. I'm fairly new to writing [e]lisp code.

@@ -2814,7 +2820,8 @@ A is the left hand side, B the right hand side."

(ivy-set-actions
'counsel-yank-pop
'(("d" counsel-yank-pop-action-remove "delete")))
'(("d" counsel-yank-pop-action-remove "delete")
("p" counsel-yank-pop-action-position "position")))
Copy link
Collaborator

@basil-conto basil-conto Dec 8, 2017

Choose a reason for hiding this comment

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

Ditto re: saying "rotate" instead of "position". It would then follow that a more intuitive key should be "r" instead of "p".

Copy link
Contributor Author

@ambihelical ambihelical Dec 8, 2017

Choose a reason for hiding this comment

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

Rotate makes sense.

@basil-conto
Copy link
Collaborator

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

@abo-abo Both of the counsel-yank-pop actions in this PR have the potential to modify kill-ring and thus the list of candidates. What happens if someone invokes one of these actions without exiting completion? Is it possible to update the candidate list after invoking a non-exiting action? If not, is it possible to ensure these actions exit completion when invoked? Would mentioning these limitations in their docstrings suffice?

@abo-abo
Copy link
Owner

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

Is it possible to update the candidate list after invoking a non-exiting action?

Possible, but a bit hacky, see M-o k for ivy-switch-buffer. Would be nice to provide a better API for that. Perhaps C-M-o should refresh the candidate list after the action by default.

@abo-abo
Copy link
Owner

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

@ambihelical Thanks.
@basil-conto Looks like you want to use counsel--yank-pop-position somewhere, please PR.

@basil-conto
Copy link
Collaborator

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

Possible, but a bit hacky

Is it just a matter of updating and refreshing the ivy-last state? If so, should I try to do that for the aforementioned two counsel-yank-pop actions?

Perhaps C-M-o should refresh the candidate list after the action by default

How would it do that for static collections?

basil-conto added a commit to basil-conto/swiper that referenced this issue Dec 8, 2017
(counsel-yank-pop-action-remove): Simplify logic.
(counsel-yank-pop-action-rotate): Use current-kill.

Re: abo-abo#1133
@basil-conto
Copy link
Collaborator

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

Looks like you want to use counsel--yank-pop-position somewhere, please PR

Done: #1359

basil-conto added a commit to basil-conto/swiper that referenced this issue Dec 8, 2017
(counsel--yank-pop-kills): New function.
(counsel-yank-pop): Use it.
(counsel-yank-pop-action-remove, counsel-yank-pop-action-rotate):
Use it.  Update collection and preselect for next ivy-call.

Re: abo-abo#1133
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