-
Notifications
You must be signed in to change notification settings - Fork 197
vi-mode: add gg/G motions and fix ^ motion behavior #953
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
base: main
Are you sure you want to change the base?
Conversation
0bcc73d to
eedb81a
Compare
ysthakur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have very little experience with both Reedline keybindings and Vim, so pardon my ignorance.
Unlike Vim, these motions do not keep the current column
I just tried them out in Vim, and both of them seem to go to the first column rather than keeping the current column. I checked my ~/.vimrc and I do have custom keybindings for some reason, but none that would affect gg/G.
their interaction with commands like c, d, and y differs because reedline does not support linewise operations
Could you give an example of where behavior would differ from Vim? For cgg, dgg, and ygg, at least, it seems to be working fine, entire lines get cut/deleted/yanked.
For G, though, I'd like to verify that Vim doesn't go to the first column. Because if it does, the implementation in this PR doesn't match.
In the future, I'd recommend splitting up PRs that do multiple things (here, adding gg and G is one feature, and fixing ^ is a related but separate thing). Not a big deal, especially since it's such a small change here.
I will be honest, I didn't test against vim but rather neovim. And I assumed the behaviour would be the same. But you are right, in vim, the cursor always goes to the first character of the line.
If we have this and perform this set of motion It is because, if you copy something linewise in vim, it will also paste linewise i.e. it won't paste it between two characters of a line.
Yes, this is where I intentionally make it different. In a commandline context, needing to go at the absolute end of the buffer is more frequent. You can go the the first character of the line using
Understood. If you ask me, I can try to split this PR. I already made separate commits for them. |
1c275de to
9bc868b
Compare
Juhan280
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated to code to use linewise copy
73e850d to
81526b0
Compare
Great! Thanks for the explanation before the edit.
Makes sense
Nah, you're good. This is a fairly small PR anyway. |
There is a bit of divergence in behaviour of `G` motion of reedline and vim. In vim, `G` takes the cursor to the first character of the last line. In reedline, it tales to the very last character of the buffer instead. Also when using these motion with a command, e.g. `c`, `d` and `y`, there are some inconsistency in handling the new line character. Added test case for `dgg`, `dG`, `cgg` and `cG` command.
Previously the `^` motion was behaving like the `0` motion which is to move the cursor to the start of the line. But in vim, the `^` motion is different from `0` motion. In vim `^` moved the cursor to the first non-blank character of the line. (See `:h ^`) Updated test case for `d^` and `c^` command since it has different behaviour now.
|
When you make changes, could you please upload the changes as separate commits, rather than rebasing and force pushing? When you force push, I can't tell what changed since the last time I reviewed the PR. However, if you make a new commit, GitHub will show me the new changes (or I can look at the new commits myself). If you're maintaining the change as 2 separate commits to keep the commit history clean, you don't need to worry about that. We always squash and merge PRs, so no matter how many commits you make, it'll all turn into a single commit when merging anyway. |
Okay
Yeah, I was trying to keep commit history clean |
| EditCommand::CutFromStart => write!(f, "CutFromStart"), | ||
| EditCommand::CutFromStartLinewise => write!(f, "CutFromStartLinewise"), | ||
| EditCommand::CutFromStartLinewise { .. } => { | ||
| write!(f, "CutFromStartLinewise leave_blank_line: <bool>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really sure how the display for variants with fields supposed to look like. And what determines a field is optional. MoveLeft has one field select and it says optional in the display. And then MoveRightBefore has two fields but only one is mentioned in the display and that's required.
This PR adds the
ggandGmotions and adjusts the^motion invi-mode. Theggmotion moves the cursor to the first character of the buffer andGmoves it to the last. Unlike Vim,Gtakes the cursor to the end of the buffer instead of the first character of the last line. Tests are included fordgg,dG,cgg, andcG.The
^motion now moves the cursor to the first non-blank character of the line instead of the line start. Test cases ford^andc^are updated accordingly.Also, the name
NonBlankStartcould be better. Please tell me if you come up with something better.This is the first pr I made in rust, if I did any mistake please point it out. Thanks