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

Setting registers is more in line with VIM #1049

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

nnicandro
Copy link
Contributor

@nnicandro nnicandro commented May 10, 2018

This fixes the odd behavior of the yank register (0) and delete registers (1-9) described in #864 and makes their behavior identical to Vim. The following changes were made:

  • Add a new variable, evil-delete-kill-ring, which stores the text for the delete registers.
  • Remove evil-was-yanked-without-register
  • Add a new variable evil-is-yank-and-delete which distinguishes between commands that yank
    and delete text vs commands that just yank text.
  • Add the function evil-set-register-on-yank which replaces the register setting logic in the yank
    functions.
  • Set the yank register when setting the unnamed register (") for behavior more in line with Vim.

I tried to figure out how I could keep the evil-was-yanked-without-register variable without changing its meaning since I know removing it will break other packages, but it couldn't be done. It was also being used in a confusing way, mainly only to prevent the setting of the yank register in evil-delete.

@nnicandro nnicandro changed the title Setting registers is more in line with VIM, fixes #864 Setting registers is more in line with VIM May 10, 2018
evil-common.el Outdated
;; Handle 1-9 registers when deleting
;; http://vimdoc.sourceforge.net/htmldoc/change.html#registers
(when (and evil-is-yank-and-delete (not (eq register ?_)))
(let ((special-delete-motions
Copy link
Member

Choose a reason for hiding this comment

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

This should be put into a customizable variable, so that other packages can add their own things to it.

@wasamasa
Copy link
Member

This looks pretty good, thanks! Regarding breakage, I've done a quick search and it seems that other than people vendoring their packages (which is totally fine), the rest seems to have copy-pasted the yank commands. They'll have to update them to make use of your code. Nothing much we can do about that.

@nnicandro
Copy link
Contributor Author

nnicandro commented Sep 25, 2018

An update on this, mainly I've tried to get the behavior as close to Vim's as possible. I think it is essentially identical now.

  • I've renamed evil-set-register-on-yank to evil-kill-new. Essentially this function was kill-new, but also handled the behavior of registers.

  • Handle the small delete register ?- in evil-kill-new instead of evil-delete, also make its behavior more in line with Vim's. That is, do not set register ?1 if register ?- is being set. Set both registers if the motion is one of evil-special-delete-motions.

  • Add documentation describing the behavior of registers in evil-kill-new. Mention evil-kill-new in the documentation of evil-yank and evil-delete.

  • Give an initial value to evil-delete-kill-ring and fix behavior when setting one of the delete registers. The previous behavior would set the wrong register if evil-delete-kill-ring was not at its maximum size.

  • Removed the variable evil-is-yank-and-delete in favor of using this-command set to evil-delete to distinguish between a yank command or a yank followed by a delete, i.e. a delete command. We can make this distinction by simply assuming that the text passed to evil-kill-new is due to a yank command if this-command is not evil-delete and if this-command is evil-delete then the text is due to a delete command.

    • Actually it probably makes more sense to make the default be that the text is due to a delete command and make the check for evil-yank since killing text in Emacs is essentially the same as deleting text in Vim.

@nnicandro
Copy link
Contributor Author

Force pushed to squash all of the WIP commits.

@syl20bnr
Copy link
Member

@wasamasa do you think we can merge this ? This is a common issue with new Evil users coming from Vim.

@syl20bnr
Copy link
Member

@DZoP Thank you for such a great PR. There is a core dump with Emacs 26.1 in Travis. Which version of Emacs are you using ?

@TheBB
Copy link
Member

TheBB commented Jan 15, 2019

I think @wasamasa has withdrawn from active participation. I'll see if I can devote some time to this, and other PRs.

@nnicandro
Copy link
Contributor Author

I'm using

GNU Emacs 26.1 (build 2, x86_64-apple-darwin18.0.0, Carbon Version 158 AppKit 1671) of 2018-12-27

but I think the issue is related to #945, running make test works fine. I triggered another Travis build to see if it goes through this time.

@nnicandro
Copy link
Contributor Author

Friendly bump.

I've been using this change without issue for a long time now, but I think before it gets merged I should address the removal of the evil-was-yanked-without-register variable which breaks packages like lispyville and evil-collection.

Maybe it would be better to keep that variable around for compataility...

@leungbk
Copy link
Contributor

leungbk commented Jun 18, 2020

@DZoP @TheBB What's the status on this? It would be nice to see this get merged at some point.

Friendly bump.

I've been using this change without issue for a long time now, but I think before it gets merged I should address the removal of the evil-was-yanked-without-register variable which breaks packages like lispyville and evil-collection.

Maybe it would be better to keep that variable around for compataility...

evil-collection uses it once, in reference to evil-mc, which has evil-was-yanked-without-register as part of a list of variables of interest, but does not otherwise record or make use of it.

The author of lispyville mentioned this change as something he would keep an eye on. I wouldn't mind making a PR to lispyville once this gets merged.

evil-common.el Outdated Show resolved Hide resolved
Copy link
Collaborator

@axelf4 axelf4 left a comment

Choose a reason for hiding this comment

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

I think these are good changes, and it would be nice to get it merged. (It is unfortunate that text deleted by delete or change commands cannot use the regular kill-ring instead of evil-delete-kill-ring, with "" being the special register. After all, it is kill-ring and not copy-ring, and you would then remain able to access text yanked from Emacs commands using the registers "1 through "9. But I understand that would complicate all paste commands.)

The docstrings could also perhaps be made a bit more understandable.

evil-common.el Outdated Show resolved Hide resolved
evil-common.el Outdated Show resolved Hide resolved
Bring it back.  But don't use it.  Should be removed in a future commit.
This adheres to the Vim documentation which states

    Writing to the "" register writes to register "0.

See `:help registers`.
The setting of registers is now handled by the eventual call to
`evil-kill-new` when calling `evil-yank`.
Some of the tests assumed that the delete registers 1-9 store yanked
text, this is no longer the case.  Those registers now store only
deleted text.
@nnicandro
Copy link
Contributor Author

I've re-worked the documentation and addressed the issues that you mentioned. Please give this another look.

I did have to fix some tests that relied on the current behavior of the 1-9 registers. I'm thinking that people may already be used to how those registers worked before this change in that they stored yanked as well as deleted text. Perhaps there should be an option to revert back to the old behavior of those registers before this change? This would also address your concern about using the 1-9 registers to access text yanked by other Emacs commands.

@nnicandro nnicandro requested a review from axelf4 May 17, 2024 02:53
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.

6 participants