emacs-lsp / lsp-ui

UI integrations for lsp-mode
https://emacs-lsp.github.io/lsp-ui
GNU General Public License v3.0
1.04k stars 139 forks source link

Don't use interactive kill-* commands in doc cleanup #512

Closed russell closed 3 years ago

russell commented 3 years ago

While trying to copy a symbol from a buffer text was inconveniently pre-pended and in some cases appended depending on if the lsp-ui doc cleanup happens before or after killing interactively in my buffer.

Some examples of entries from the kill-ring variable

e.g. Me trying to copy some text

"

---
git2::Error"

e.g. an example of a weird entry found in my kill ring, there are many more like this, some a just huge strings of empty lines.

"

---
"

This is happening because the interactive kill-* commands all put data onto the kill ring. If the previous command was kill-region, then the next kill command will append to the previous kill ring entry.

sebastiencs commented 3 years ago

Thanks @russell !

What do you think of just binding to nil all the variables those kill-{whole-}line read/modify ? instead of reimplementing those kill functions.

(let (kill-ring buffer-undo-list last-command this-command deactivate-mark
                      kill-ring-yank-pointer interprogram-cut-function save-interprogram-paste-before-kill)
        (kill-whole-line))
russell commented 3 years ago

Your proposed solution works, but it feels like a weird solution still, I mean your talking about using an interactive function to perform background modification of text in a hidden buffer.

I don't like that idea for a few of reasons,

  1. these are interactive user facing functions, so using them in this way isn't how they are intended to be used, trying to stub all their local variables is a pretty good indication that we are miss-using that API. I would worry that this means that we couldn't rely on consistent behavior from these functions overtime
  2. because they are user facing interactive functions, they are also likely to have advice put on them, as people like to modify the behavior of Emacs. So you could end up with bug reports relating to weird behavior due to users advising these interactive functions, which is a perfectly reasonable thing for them to do. e.g. https://stackoverflow.com/questions/16915712/implementation-of-a-kill-word-or-line-function-in-emacs
  3. this is a bit petty, but effectively we will still be allocating values to the kill-ring, all the rubbish we are deleting from the buffer, only to cause it to be garbage collected moments later, so it's also more in-efficient
russell commented 3 years ago

I see that tests are currently disabled in CI, but either way, they are there for when you get around to enabling them :)

sebastiencs commented 3 years ago

Thank you @russell, I will enable tests in ci