smartparens icon indicating copy to clipboard operation
smartparens copied to clipboard

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

Open CSRaghunandan opened this issue 7 years ago • 9 comments

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

CSRaghunandan avatar Aug 14 '18 19:08 CSRaghunandan

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.

Fuco1 avatar Aug 14 '18 22:08 Fuco1

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

CSRaghunandan avatar Aug 15 '18 07:08 CSRaghunandan

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

Fuco1 avatar Aug 15 '18 07:08 Fuco1

Seems to be originated here #824

Fuco1 avatar Aug 15 '18 07:08 Fuco1

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.

nrxus avatar Jul 19 '20 17:07 nrxus

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.

nivekuil avatar Aug 02 '20 18:08 nivekuil

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.

Fuco1 avatar Oct 18 '20 12:10 Fuco1

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))))))

nivekuil avatar Oct 18 '20 20:10 nivekuil

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

Fuco1 avatar Oct 19 '20 20:10 Fuco1