minad / consult

:mag: consult.el - Consulting completing-read
GNU General Public License v3.0
1.13k stars 99 forks source link

Allow consult-yank-pop to insert after point #221

Closed codygman closed 3 years ago

codygman commented 3 years ago

This is useful for evil users and inspired by the counsel equivalent in https://github.com/abo-abo/swiper/pull/1762

codygman commented 3 years ago

I think this could be done by modifying insert-for-yank-1 where the insert call happens but I'm not quite sure the appropriate way and if that's all that needs done.

minad commented 3 years ago

I am not exactly how the feature is supposed to work, since I am not an evil user myself. But I would be happy to merge a PR enhancing the command for evil users as long as the enhancement does not degrade the behavior for vanilla Emacs users.

oantolin commented 3 years ago

What does "insert after point" mean? Does it mean that after yanking the point is to the left of the yanked text? Like doing C-u C-y?

oantolin commented 3 years ago

And I assume you'd want the same interface for this functionality as with the built-in yank, namely using C-u, @codygman?

oantolin commented 3 years ago

Maybe we should follow the built-in command and only enable this meaning of C-u for consult-yank and not for consult-yank-pop or consult-yank-replace.

codygman commented 3 years ago

I'll try to make a gif demonstrating later as well as answering these other questions I'm not sure about currently.

codygman commented 3 years ago

Actually, no need to make a gif. Evil/vim pasting works like this:

Given:

Foo bar

In vim/evil if you yank the text bar and your mark is at the beginning of that line, it will result in:

Fbaroo

This seems to be different from my tests than:

Does it mean that after yanking the point is to the left of the yanked text? Like doing C-u C-y?

In fact, in vanilla emacs the only way I know how to get this behavior is to move mark on top of the first o in Foo bar.

And I assume you'd want the same interface for this functionality as with the built-in yank, namely using C-u, @codygman?

I'm not quite sure what you mean, but if you mean would I like for using C-u in the above example to result in barFoo then yes since it would be consistent with evil-paste-before.

Maybe we should follow the built-in command and only enable this meaning of C-u for consult-yank and not for consult-yank-pop or consult-yank-replace.

That sounds right, but I'll admit I don't fully understand consult-yank-pop or consult-yank-replace yet.

codygman commented 3 years ago

I wrote some code, but the preview is wrong since consult--yank-read will need to know the difference here as well.

(defcustom selectrum-yank-after-point nil "docs")

(defun consult-yank ()
  "Select text from the kill ring and insert it."
  (interactive) 
  (let ((text (consult--yank-read)))
    (if selectrum-yank-after-point
    (if (string-match-p "\n" text)
        (next-line)
      (forward-char 1)))
    (setq yank-window-start (window-start))
    (push-mark)
    (insert-for-yank text)
    (setq this-command 'yank)
    nil))

Actually I think that the check for a newline will need to be done when the selection is changed in that function.

Hopefully communicating the above semantics in code makes the desired behavior of:

I think that second piece is the vim heuristic but as I write it I'm wondering if it's not more complicated than that.

codygman commented 3 years ago

I wrote some code, but the preview is wrong since consult--yank-read will need to know the difference here as well.


Actually I think that the check for a newline will need to be done when the selection is changed in that function.

So I think ov's point (pt here) needs to be modified according to the same heuristic but using cand inside of the lambda that I'm betting is acting as a callback in the consult--yank-read definition:

(defun consult--yank-read ()
  "Open kill ring menu and return selected text."
  (consult--read
   "Yank text: "
   (consult--remove-dups kill-ring)
   :history t ;; disable history
   :sort nil
   :category 'kill-ring
   :require-match t
   :preview
   ;; If previous command is yank, hide previously yanked text
   (let* ((ov) (pt (point)) (mk (or (and (eq last-command 'yank) (mark t)) pt)))
     (lambda (cand restore)
       (if restore
           (when ov (delete-overlay ov))
         (unless ov (setq ov (consult--overlay (min pt mk) (max pt mk) 'invisible t)))
         ;; Use `add-face-text-property' on a copy of "cand in order to merge face properties
         (setq cand (copy-sequence cand))
         (add-face-text-property 0 (length cand) 'consult-preview-yank t cand)
         ;; Use the `before-string' property since the overlay might be empty.
         (overlay-put ov 'before-string cand))))))
codygman commented 3 years ago

I think I'd need to translate my next-line and forward-char calls into point modifications using this method and I'm not sure if that's easily possible for next-line.

I'm willing to bet there's a better approach here I don't know since I'm not super experienced with elisp so I'll leave this for now and wait for feedback.

minad commented 3 years ago

I think I'd need to translate my next-line and forward-char calls into point modifications using this method and I'm not sure if that's easily possible for next-line.

You can probably use the exact same method you used above to move the point and then place the overlay at the desired position. If the overlay position should be changed during the preview there is move-overlay.

codygman commented 3 years ago

I tried using move-overlay naievely after the put-overlay to test that idea and consult seemed to run slowly, not show any preview, nor any error.

I also noticed that if I try to redefine consult--yank-read in my config then ov is out of scope and I'm not sure why.

Thanks for the help though! I'll try to read more about overlays and better understand whats going on here tomorrow or this weekend if I get time.

minad commented 3 years ago

I also noticed that if I try to redefine consult--yank-read in my config then ov is out of scope and I'm not sure why.

If this happen you evaluated the function without lexical scoping. Ensure that this is enabled. I don't understand why Emacs is still defaulting to this horrible dynamical scoping by default.

oantolin commented 3 years ago

At least the scratch buffer uses lexical scope by default now.

minad commented 3 years ago

At least the scratch buffer uses lexical scope by default now.

I often start with something in the scratch buffer then save it somewhere and reevaluate the new buffer, losing lexical scoping on the way. I hope the default is lexical everywhere in 28.

oantolin commented 3 years ago

I hope the default is lexical everywhere in 28.

Me too! It seems a little doubtful, though: I haven't tried very recent 28 snapshots because the Debian repo I use to install Emacs seems to have stopped updating, but it isn't the default in the build I'm using.

minad commented 3 years ago

I am so glad that Emacs 27.1 is now available in buster-backports :tada: However this reduces the probability for me to move to Emacs 28, since I am now back from using my self compiled Emacs 27 to the distribution version.

codygman commented 3 years ago

Alright, I finally got each edge case working and one I discovered along the way. Those cases are:

The only remaining wish I have is having evil's overwrite behavior on paste if a region is highlighted, but this gets all the basics seemingly reliably.

The implementation probably isn't pretty and definitely has duplication, but it's working:

  (defun consult-yank ()
    "Select text from the kill ring and insert it."
    (interactive)
    (let ((text (consult--yank-read)))
      (if (string-match-p "\n" text)
          (progn (next-line) (beginning-of-visual-line))
        (forward-char 1))
      (setq yank-window-start (window-start))
      (push-mark)
      (insert-for-yank text)
      (setq this-command 'yank)
      (if (string-match-p "\n" text) (previous-line) nil)))

(defun consult--yank-read ()
  "Open kill ring menu and return selected text."
  (consult--read
   "Yank text: "
   (consult--remove-dups kill-ring)
   :history t ;; disable history
   :sort nil
   :category 'kill-ring
   :require-match t
   :preview

   (let* ((ov) (pt (point)) (mk (or (and (eq last-command 'yank) (mark t)) pt)))
     (lambda (cand restore)
       (if restore
           (when ov (delete-overlay ov))
         (unless ov (setq ov (consult--overlay (min pt mk) (max pt mk) 'invisible t)))

         (if (string-match-p "\n" cand)
             (move-overlay ov (save-mark-and-excursion (progn (next-line) (beginning-of-visual-line)) (point)) (save-mark-and-excursion (progn (next-line) (beginning-of-visual-line)) (point)))
           (move-overlay ov (+ 1 (point)) (+ 1 (point))))

         ;; Use `add-face-text-property' on a copy of "cand in order to merge face properties
         (setq cand (copy-sequence cand))
         (add-face-text-property 0 (length cand) 'consult-preview-yank t cand)
         ;; Use the `before-string' property since the overlay might be empty.
         (overlay-put ov 'before-string cand)

         (if (string-match-p "\n" cand)
             (move-overlay ov (save-mark-and-excursion ((progn (next-line) (beginning-of-visual-line))) (point))
                           (save-mark-and-excursion (next-line) (point)))
           (move-overlay ov (+ 1 (point)) (+ 1 (point)))))))))

I'm not sure if it's acceptable to just put this behind an evil (heh) feature flag as is or if some more cleanup or discussion needs to be had. I can probably put up a PR this weekend if I continue actually having internet for a change :laughing:

Edit: It looks like I'm getting an error now and I'm not sure I understand it:

(fn X)" run-hook-with-args-until-success consult--completion-candidate-hook] 8]): (invalid-function (progn (next-line) (beginning-of-visual-line)))

Confusing since this works fine in the scratch buffer:

(progn (next-line) (beginning-of-visual-line))

I do notice that the single next-line call worked and things only went awry when I used progn for some reason.

okamsn commented 3 years ago

The only remaining wish I have is having evil's overwrite behavior on paste if a region is highlighted, but this gets all the basics seemingly reliably.

Try looking at the built-in delete-selection-mode and delsel.el. I think it puts properties on existing commands.

minad commented 3 years ago

@codygman Is there still interest in this feature? Do you want to submit a PR?

codygman commented 3 years ago

@minad I currently don't have interest since I'm using vanilla Emacs now. I'll close it for now but could come back to it later.

minad commented 3 years ago

I'm using vanilla Emacs now.

Even better then ;) How did it come to that?

codygman commented 3 years ago

I'm using vanilla Emacs now.

Even better then ;) How did it come to that?

Curiosity and frustration with integration issues like this? Lol

I already pay enough for being an exwm user ;)