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

Improve Visual Mode Selection and Command Consistency #867

Merged
merged 6 commits into from
Feb 10, 2025

Conversation

deephbz
Copy link
Contributor

@deephbz deephbz commented Dec 31, 2024

Fix Vi Mode Visual Selection Behavior

This PR addresses several inconsistencies in Reedline's Vi mode implementation, particularly around visual mode and cut operations, making the behavior more aligned with standard Vim.

Changes

1. Visual Mode Selection Clearing

  • Added proper selection clearing when pressing Esc
  • Fixed by adding explicit cursor movements to reset selection state
  • Ensures visual feedback matches mode state

2. Cut Operations Consistency

  • Fixed x, s, d, and c commands to behave consistently with Vim standards
  • Made x properly cut selection in visual mode instead of just one character
  • Fixed c to enter insert mode after cutting selection
  • Corrected selection range calculation to be inclusive (include the last character)

3. Mode Transitions

  • Properly restore normal mode after cut operations in visual mode
  • Added mode transitions for DeleteChar and DeleteToEnd commands
  • Fixed mode switching timing to happen before command execution

Technical Details

  • Modified get_selection() to include the last character in selection range
  • Updated command parsing to handle mode transitions correctly
  • Added proper mode restoration for all cut-related commands

Testing

The changes have been tested with the following scenarios:

  • Visual mode selection and Esc behavior
  • Cut operations in both normal and visual modes
  • Mode transitions after various commands
  • Selection range calculations

Fixes #865 .
I've done cargo fmt --all, cargo clippy, and cargo test for each of these three commits.

@deephbz deephbz mentioned this pull request Dec 31, 2024
@deephbz deephbz changed the title Bugfix/visual mode Improve Visual Mode Selection and Command Consistency Dec 31, 2024
@deephbz deephbz mentioned this pull request Dec 31, 2024
2 tasks
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 for tackling the VI mode, very much lacking and deserving attention!

@deephbz deephbz force-pushed the bugfix/visual-mode branch 2 times, most recently from 49357f3 to f033574 Compare January 1, 2025 01:57
@deephbz deephbz force-pushed the bugfix/visual-mode branch from f033574 to 57c2128 Compare January 1, 2025 02:22
@deephbz
Copy link
Contributor Author

deephbz commented Jan 1, 2025

Minor note: Github incorrectly introduced another "participant" because I messed up my .gitconfig "name" setting. It's fixed now

@deephbz deephbz requested a review from sholderbach January 4, 2025 01:46
@sholderbach
Copy link
Member

sholderbach commented Jan 19, 2025

Trying out the visual selection while the v-characterwise mode seems to behave as I would expect for the deletion itself, the highlight through the background lags one character behind in my testing of a478473 while the foreground changes.

  1. insert blurb
  2. move to l
  3. press l
    grafik

Are we running into an unfortunate situation regarding the styling? (setting background for the selection and inverting it again for the cursor?)

@132ikl
Copy link

132ikl commented Feb 8, 2025

hey @deephbz would you be able to finish this one up? it looks like we should be good if the styling issue mentioned by @sholderbach is resolved?

@deephbz
Copy link
Contributor Author

deephbz commented Feb 9, 2025

Trying out the visual selection while the v-characterwise mode seems to behave as I would expect for the deletion itself, the highlight through the background lags one character behind in my testing of a478473 while the foreground changes.

  1. insert blurb
  2. move to l
  3. press l
    grafik

Are we running into an unfortunate situation regarding the styling? (setting background for the selection and inverting it again for the cursor?)

I cannot reproduce this. Below is my recording:
out

Even if there's small rendering/styling issue, I think we could address that in future PRs given current PR functions well in a self-contained way and it's blocking others

@deephbz
Copy link
Contributor Author

deephbz commented Feb 9, 2025

hey @deephbz would you be able to finish this one up? it looks like we should be good if the styling issue mentioned by @sholderbach is resolved?

See my above reply

@132ikl
Copy link

132ikl commented Feb 10, 2025

hmm, I also can't reproduce it. I also tried using the demo example with --vi like Stefan did and I don't seem to experience the issue. @sholderbach could you see if you can still reproduce it?

@sholderbach
Copy link
Member

sholderbach commented Feb 10, 2025

(style issue is still there for me)
Possibly there is an interaction with the way Windows Terminal renders the ANSI background coloring in my case. The char under cursor is still somewhat recognizable due to the missing syntax highlighting there.

In the interest of moving things along let's merge this and revisit the cursor styling in due time.

@sholderbach sholderbach merged commit 853cde5 into nushell:main Feb 10, 2025
6 checks passed
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.

Vi mode: 's' key not working in visual mode
5 participants