Fuco1 / smartparens

Minor mode for Emacs that deals with parens pairs and tries to be smart about it.
GNU General Public License v3.0
1.82k stars 195 forks source link

sp-kill-whole-line adds extra new line character to the kill-ring when killing the whole line #920

Open CSRaghunandan opened 6 years ago

CSRaghunandan commented 6 years ago

This has been annoying me for some while now. This behavior only happens when sp-kill-whole-line kill the whole line of text.

After killing a line with sp-kill-whole-line, a new line character is added to the kill-ring. So, when I run yank-pop, instead of yanking the last killed text, a new line is inserted. So, as a workaround, I've to run yank-pop with C-2 as a prefix argument to get the last killed text.

To reproduce in emacs -Q:

  1. run package-initiazlie
  2. run (require 'smartparens)
  3. kill a whole line using (sp-kill-whole-line)
  4. run yank-pop
Fuco1 commented 6 years ago

Do we want to have the newline attached to the killed line or discard it alltogether? I'm not using this setting so I'm not sure what is expected.

In either case what you are describing is definitely a bug, it should create only one entry.

CSRaghunandan commented 6 years ago

Do we want to have the newline attached to the killed line or discard it alltogether? I'm not using this setting so I'm not sure what is expected.

I'm not sure what the community would prefer. Some might like to have the new line appended to the king ring whilst others might not. Personally, I'd like to have the new line appended to the end

Fuco1 commented 6 years ago

Maybe we should do what kill-whole-line does as this is pretty much a copy of that functionality.

Fuco1 commented 6 years ago

Seems to be originated here #824

nrxus commented 4 years ago

I know this is an old bug but I also just came across this surprising behavior. Given that kill-whole-line appends the new line into the kill-ring, then sp-kill-whole-line should have the same behavior.

nivekuil commented 4 years ago

This is all this function really needs to do:

(defun sp-kill-whole-line ()
  (interactive)
  (beginning-of-line)
  (sp-kill-hybrid-sexp nil))

The extra kill ring entry is from the extra kill step at the end which supposedly handles some edge case at (eobp), but afaict not only is the logic buggy but the whole thing is unnecessary.

Fuco1 commented 3 years ago

The code is there to preserve the behaviour of built-on kill-whole-line where it removes the newline.

Instead of calling kill-whole-line again, we need to use thing-at-point to get the line boundaries, then delete-and-extract-region the text and kill-append it to the current kill.

nivekuil commented 3 years ago

It's been a while so I don't remember what the specific issue was here, but I have these two functions and they've been good replicas of whole-line-or-region.el's functionality:

(defun sp-kill-whole-line-or-region ()
  (interactive)
  (if mark-active
      (sp-kill-region (region-beginning) (region-end))
    (beginning-of-line)
    (sp-kill-hybrid-sexp nil)
    (when (= (char-after) ?\n)
      (append-next-kill)
      (kill-whole-line))))

(defun sp-whole-line-or-region-kill-ring-save ()
  (interactive)
  (if mark-active
      (copy-region-as-kill (region-beginning) (region-end))
    (save-excursion
      (beginning-of-line)
      (let ((sexp (sp-get-hybrid-sexp)))
        (kill-ring-save (plist-get sexp :beg) (plist-get sexp :end))))))
Fuco1 commented 3 years ago

Oh, it seems just adding append-next-kill might solve the issue. I didn't know such a function existed.