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

Add Vi o command to swap anchor and cursor #877

Conversation

thomasschafer
Copy link
Contributor

@thomasschafer thomasschafer commented Jan 31, 2025

This PR adds a keybinding to o in Vi visual mode, which swaps the cursor and the anchor - see here for Vim docs explaining this behaviour.

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.

I never stop learning new things about vim :) Didn't know about this Visual mode feature.

The feature part looks good to me. So the only thing that needs rework is the part extending the selection by a "character", which should now be covered by the changes in #867. Then we should be good to land.

Thanks for the tests as well!

@thomasschafer thomasschafer force-pushed the tschafer-add-command-to-swap-anchor-and-cursor branch from 7a5937a to 1df24d5 Compare February 12, 2025 19:53
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.

Looks good and works for me!

Also thanks for adding tests here.

Comment on lines +116 to +119
Some('o') => {
let _ = input.next();
Some(Command::SwapCursorAndAnchor)
}
Copy link
Member

@sholderbach sholderbach Feb 13, 2025

Choose a reason for hiding this comment

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

I think in theory this dispatch from the parser is insufficient as it would compete with the normal mode o (open insert mode in the line below) and we don't discriminate between the modes here.

But as we don't have that o implemented yet (as far as i can tell) this should be fine.

@sholderbach sholderbach merged commit 6133ec0 into nushell:main Feb 13, 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.

2 participants