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

Vi-Mode Feature: Atomic unified commands for ChangeInside/DeleteInside #874

Merged

Conversation

deephbz
Copy link
Contributor

@deephbz deephbz commented Jan 18, 2025

Prelude

I enjoy so much using nushell. It has potential becoming the object-oriented standard shell on Unix! I depend on Vi a lot so I'm excited to contribute to continue Nushell's Vi mode!

Note this PR is on top of PR #867 so reviewers you could start review from the last commit .

Problems Solved

Change/Delete inside a pair (quotes, brackets, etc.) are not atomic action: The previous implementation does cut/delete by two actions: left contents to the cursor, then right. This doesn't play nice with redo/undo and they only put the right part to clip buffer.

Changes Made

Will state following sequence of control flow:

  1. Vi mode parser validates "action+pair" user input (e.g. ci(, di") once using bracket_pair_for.
  2. Once such "action+pair" input is detected, Command::DeleteInsidePair/ChangeInsidePair Vi commands are generated. Differentiating Change v.s. Delete is for mode change in Vi.
  3. Vi Command::Delete/ChangeInside are both translated to EditCommand::CutInside, a new atomic editor command.
  4. Inside the implementation of cut_inside it will handle nested brackets the same way as in standard Vi.
  5. Added thorough unittests for cut inside.

Misc

This is related to #849 but that's for change inside word while this PR focuses on change inside quotes/brackets.

@deephbz deephbz changed the title Feature/vi mode action inside pair implementation Vi-Mode Feature: Atomic unified commands for ChangeInside/DeleteInside Jan 18, 2025
@deephbz deephbz force-pushed the feature/ViMode-ActionInsidePair-implementation branch from dc73251 to 1490b42 Compare January 18, 2025 10:05
@deephbz deephbz mentioned this pull request Jan 18, 2025
2 tasks
@deephbz
Copy link
Contributor Author

deephbz commented Feb 9, 2025

@sholderbach please review this when you get a chance

Copy link

@132ikl 132ikl left a comment

Choose a reason for hiding this comment

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

thank you! this is a big improvement. I have some notes, and I also tried to simplify the find_matching_pair code. Let me know what you think 😄

src/edit_mode/vi/command.rs Outdated Show resolved Hide resolved
src/edit_mode/vi/parser.rs Show resolved Hide resolved
src/edit_mode/vi/command.rs Outdated Show resolved Hide resolved
src/enums.rs Outdated Show resolved Hide resolved
src/core_editor/editor.rs Outdated Show resolved Hide resolved
src/core_editor/line_buffer.rs Outdated Show resolved Hide resolved
@sholderbach
Copy link
Member

Ah I didn't see your comments here first @132ikl

@deephbz do you want to finish up the changes on this branch/PR or on #868

…e/YankInside(to be added):

- added thorough unittests to emulate standard Vi behavior.
- handle nested brackets but not nested quotes, alinged with Vi behavior
@deephbz deephbz force-pushed the feature/ViMode-ActionInsidePair-implementation branch from 1490b42 to 5172cea Compare February 11, 2025 02:18
@deephbz
Copy link
Contributor Author

deephbz commented Feb 11, 2025

Ah I didn't see your comments here first @132ikl

@deephbz do you want to finish up the changes on this branch/PR or on #868

As dependency #867 is merged, I've rebased this PR and addressed @132ikl 's suggestions. They are incredibly helpful.
I think we are good to merge this PR now. Please review:
image

Note let's keep this and #868 separate. After this PR gets merged, #868 will be left with only one commit addressing a different topic.

Copy link

@132ikl 132ikl left a comment

Choose a reason for hiding this comment

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

looks great, thank you!

@132ikl 132ikl merged commit 6cbdaa4 into nushell:main Feb 11, 2025
6 checks passed
@deephbz deephbz deleted the feature/ViMode-ActionInsidePair-implementation branch February 11, 2025 05:40
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.

3 participants