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

vim: Add support for temporary normal mode (ctrl-o) within insert mode #19454

Merged
merged 10 commits into from
Nov 13, 2024

Conversation

axelcarl
Copy link
Contributor

@axelcarl axelcarl commented Oct 19, 2024

Support has been added for the ctrl-o command within insert mode. Ctrl-o is used to partially enter normal mode for 1 motion to then return back into insert mode.

Release Notes:

  • vim: Added support for ctrl-o in insert mode to enter temporary normal mode

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 19, 2024
@ConradIrwin ConradIrwin self-assigned this Oct 23, 2024
@ConradIrwin
Copy link
Member

Thanks for this! I am amazed how little code gets such a large % of this feature right.

If you'd like to pair with me on getting this over the line feel free to book time here: https://calendly.com/conradirwin/pairing. I did some looking into how vim works, thoughts below:

We can't rely on any EditorEvent leaving the mode, as these can happen for other reasons (e.g. if a collaborator moves a cursor; a process on your machine edits the current file, of even if your own cursor moves as part of a ctrl-o cs([) and doesn't happen in some cases we should handle (e.g. ctrl-o cmd-s to save).

We could possibly narrow it down to make it more robust, but I think we should instead try and keep the detection a little more explicit.

It looks like there are roughly four main categories of things that might happen after ctrl-o:

  1. ctrl-o <motion> (e.g. ctrl-o w)
  2. ctrl-o v <do-visual-stuff>
  3. ctrl-o p / other actions that mutate the buffer (either from vim or zed)
  4. ctrl-o : command or ctrl-o / search (though I think we could descope this from v1)

We can detect each differently:

  1. We can exit at the end of normal_motion
  2. We can exit whenever we switch_mode to Normal (we already need to clear it when the mode switches to Insert/Replace)
  3. Is a bit trickier. For vim actions: . already relies on a very similar notion of "done". In stop_recording we currently set a flag to stop recording that is handled in observe_actions; I think we should additionally exit temporary mode if this is set. For non-vim actions, they cannot chain, so in observe_actions if we see any non-vim action, we should probably also exit temporary mode.
  4. Search we can probably handle by putting Insert into prior_mode. Not sure about Command yet... perhaps descope; otherwise I think we need a way to know when the command palette is closed and restore insert mode after the selection action was dispatched.

Edge cases in the current implementation I think we should fix and add tests for:

  • i ctrl-o escape does not return to insert mode
  • i ctrl-o p puts the cursor one character before the end of the pasted content instead of after
  • i ctrl-o / a enter leaves you in normal mode

There are a few other feature interactions to think about:

  • <count> i <ctrl-o> causes the count to be ignored. We could copy this limitation, or not (as our insert mode recording currently just records the actions anyway).
  • . in vim mode is reset by ctrl-o, so i ctrl-o ~ x escape . will insert a second x. In the current implementation we capitalize the X. We should either copy vim here, or use the fact that we already record the full action sequence and have . capitalize the X and add another x.
  • vim help also calls out normal mode mappings that switch in and out of insert mode as another issue: ideally we copy vim's behavior here, but also happy to descope for now.

The final thing to do is to update the mode indicator to show "(insert)" before the mode name :D.

@axelcarl
Copy link
Contributor Author

Thanks for the lengthy response!

Schedueled a pair-session the 5th, sadly couldn't find a time that fit me sooner but on the other hand it will give me more time to look into the current edge cases as well as getting more familiar with the codebase.

Will iterate on the current implementation in the meanwhile, thanks again :)

@axelcarl axelcarl force-pushed the vim-insert-ctrl-o branch 2 times, most recently from 8a3750d to 3bde3c6 Compare November 3, 2024 19:51
This commit adds the action as well as the temp_mode bool to the Vim struct.
Temporary normal mode will now be exited after one normal motion.
Temporary normal will now exit when the mode is switched (unless its a
visual mode).
A prefix will now be prepended to the mode indicator when temporary
normal mode is activated.
@axelcarl axelcarl force-pushed the vim-insert-ctrl-o branch 2 times, most recently from 81afa62 to 72f736e Compare November 6, 2024 09:27
@axelcarl
Copy link
Contributor Author

axelcarl commented Nov 7, 2024

@ConradIrwin Added a visual test (which was good because it doesn't work after yesterdays changes). Will look at it later and see if I can figure out what's wrong but my best guess is that the second ctrl-o is somehow cancelled. (When testing I had to run ctrl-o twice to get back into temporary normal mode).

@axelcarl
Copy link
Contributor Author

axelcarl commented Nov 9, 2024

@ConradIrwin Was able to fix both the visual test (that I added) and the failing replay test (it was due to blocking repeating replays/repeats).

All tests are passing now.

@ConradIrwin ConradIrwin merged commit b1cd9e4 into zed-industries:main Nov 13, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants