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

Implemented VI mode change inside and delete inside functionality #844

Merged
merged 8 commits into from
Oct 17, 2024

Conversation

ayax79
Copy link
Contributor

@ayax79 ayax79 commented Oct 11, 2024

This implements the change in (ci) and delete inside vi mode functionality:

  • ci" - Change in quotes
  • di" - Delete in quotes
  • ci[ - change in brackets

supports [ , ( , ", ', `, {

@ayax79 ayax79 changed the title Implemented change in and delete in functionality Implemented change inside and delete inside functionality Oct 11, 2024
@ayax79 ayax79 changed the title Implemented change inside and delete inside functionality Implemented VI mode change inside and delete inside functionality Oct 11, 2024
@ayax79 ayax79 added the A-ViKeybinding Area: Vi(m) keybinding support label Oct 11, 2024
Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Thanks, such a nice convenience of vim to have!

There are certainly some mismatches we will still have due to the clunky event translation (like not advancing to the text object if before or multiplier and redo things that may need some tweaking but that probably needs a rework outside the scope of one PR)

src/edit_mode/vi/command.rs Outdated Show resolved Hide resolved
src/edit_mode/vi/parser.rs Outdated Show resolved Hide resolved
@ayax79 ayax79 requested a review from sholderbach October 11, 2024 19:00
@blindFS
Copy link
Contributor

blindFS commented Oct 14, 2024

I was waiting for this feature for long. It made me curious about the future plans for nushell repl parser/highlighter, are we going to use treesitter here in reedline in near future? Since without AST info, it's hard to deal with nasty cases where brackets get nested like (foo(bar "(" ) baz)

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

Looking good to get that going, in principle the i{char} text object parsing could also move to the motion parsing part. The command logic should preempt to insert mode before it is interpreted as a traditional motion. But the gain from that refactor is not as big yet as we only have d and c accepting motions at the moment.

Comment on lines +190 to +196
Self::ChangeInside(right) if is_valid_change_inside_right(right) => {
let left = bracket_for(right);
vec![
ReedlineOption::Edit(EditCommand::CutLeftBefore(left)),
ReedlineOption::Edit(EditCommand::CutRightBefore(*right)),
]
}
Copy link
Member

Choose a reason for hiding this comment

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

I am thinking you could save yourself some duplication here by doing the conversion to the one bracket side when creating the Change/DeleteInisde(char) but then there would be also something to watch out for correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would still need to know if it is a right of left side bracket to get the ordering right.. It felt cleaner to have be a separate match statements. I did collapse the Delete and Change conditions together since they were inherently the same.

@ayax79 ayax79 merged commit 5dd7e0e into nushell:main Oct 17, 2024
6 checks passed
@ayax79 ayax79 deleted the vi_mode_change_delete_in branch October 17, 2024 00:30
@ayax79
Copy link
Contributor Author

ayax79 commented Oct 17, 2024

Another thing that should be addressed at some point.. Since this is basically supplying two actions, it requires two undo commands to get back to the previous state.

@bishopmatthew
Copy link

I was waiting for this feature for long. It made me curious about the future plans for nushell repl parser/highlighter, are we going to use treesitter here in reedline in near future? Since without AST info, it's hard to deal with nasty cases where brackets get nested like (foo(bar "(" ) baz)

+1 to that — I didn't realize someone was already working on this and had started looking into how one might implement the full set of vim text object operations...

The basic pattern is where:

Operation is something like d (delete), c (change), y (yank/copy), v (select)
Scope is either i (inside) or a (around)
Text object can be things like w (word), p (paragraph), ( (parentheses), " (quotes), } (curly braces)

Anyways — it seems like the way to be able to implement all of this stuff in a consistent way would be to use tree sitter.

  • reedline could be more useful to other shells since support for other languages
  • what @blindFS mentioned about handling complex nesting — actually it seems like maybe you've already started on improving https://github.com/nushell/tree-sitter-nu in support of this?

@blindFS
Copy link
Contributor

blindFS commented Nov 28, 2024

@bishopmatthew tree-sitter-nu's accuracy has been improved recently on its own. Feel free to report any parsing error or WA. As for the integration to reedline, I think it's not planned yet, and a general set of api to process ast/cst would be a better option on second thought. Using treesitter may not be the only option since it needs extra overhead compared to the native parser.

Actually there's a hacking way (in case you want the functionality right now) for limited textobj related ops with current reedline implementation: mapping to user defined command with internal ast and commandline for the dirty job.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ViKeybinding Area: Vi(m) keybinding support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants