-
Notifications
You must be signed in to change notification settings - Fork 162
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
base: main
Are you sure you want to change the base?
Conversation
202d51e
to
8b3bdfe
Compare
8b3bdfe
to
b2566f4
Compare
b2566f4
to
ff8a9da
Compare
There was a problem hiding this 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!
src/core_editor/editor.rs
Outdated
(selection_anchor, self.insertion_point()) | ||
( | ||
selection_anchor, | ||
(self.insertion_point() + 1).min(buffer_len), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
byte index + 1 will run into the issue that this may advance into part of a multibyte UTF-8 character, producing nonsense and a panic downstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think the min(buffer_len)
can help here?
If not would you kindly suggest alternative way to approach the wrong current selection range issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of (selection_anchor, self.insertion_point()
you could use (selection_anchor, self.line_buffer.grapheme_right_index())
.
For the else
branch i am not sure. You could change the grapheme_right_index
method to allow passing a 'cursor position' and then call it with selection_anchor
. Along the lines of:
pub fn grapheme_right_index_from_pos(&self, pos: usize) -> usize {
self.lines[pos..]
.grapheme_indices(true)
.nth(1)
.map(|(i, _)| pos + i)
.unwrap_or_else(|| self.lines.len())
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, something along those lines sounds right. We advance the cursor always a full grapheme (e.g. so you don't get stuck between an accent and a vowel or the components of an emoji if they are composite and rendered together)
Here it is also worth checking if changing the semantics of the selection for Vi visual mode doesn't introduce a regression for our selection stuff in the emacs mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goot catch! Thank you Johannes for the helpful example. I've made the change as you suggest, tested it and added unittests for the new grapheme_right_index_from_pos
method.
It looks we've agreed on all points of this PR. Please help review and feel free to throw qs / edit / merge.
src/edit_mode/vi/parser.rs
Outdated
@@ -108,13 +108,19 @@ impl ParsedViSequence { | |||
| (Some(Command::RewriteCurrentLine), ParseResult::Incomplete) | |||
| (Some(Command::SubstituteCharWithInsert), ParseResult::Incomplete) | |||
| (Some(Command::HistorySearch), ParseResult::Incomplete) | |||
| (Some(Command::Change), ParseResult::Valid(_)) => Some(ViMode::Insert), | |||
| (Some(Command::Change), ParseResult::Valid(_)) | |||
| (Some(Command::Change), ParseResult::Incomplete) => Some(ViMode::Insert), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the mode change should occur for the Change
/Incomplete
combination already? At least in normal mode we should remain there. Different story in Visual mode, so maybe that is what you are trying to achieve.
We just may want to defend against it becoming an issue in normal mode.
Same for the delete part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree the mode change depends on current mode. Fixed this in the 5th commit of this PR (if you'd like to squash it works together with the 1st commit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, at first I was wondering about the special delete versions you had in the previous versoins but they are unreachable in visual mode due to all of them starting with d
... Oh do we have to handle an x
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe x
should be handled, to be consistent with most Vim editors (including the classic vi
): Under visual mode user is able to select a range and delete with either x
or d
.
49357f3
to
f033574
Compare
…t::ResetSelection
…d yield mode change from visual to insert/normal
f033574
to
57c2128
Compare
Minor note: Github incorrectly introduced another "participant" because I messed up my |
9066490
to
a478473
Compare
Trying out the visual selection while the Are we running into an unfortunate situation regarding the styling? (setting background for the selection and inverting it again for the cursor?) |
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
2. Cut Operations Consistency
x
,s
,d
, andc
commands to behave consistently with Vim standardsx
properly cut selection in visual mode instead of just one characterc
to enter insert mode after cutting selection3. Mode Transitions
DeleteChar
andDeleteToEnd
commandsTechnical Details
get_selection()
to include the last character in selection rangeTesting
The changes have been tested with the following scenarios:
Fixes #865 .
I've done
cargo fmt --all
,cargo clippy
, andcargo test
for each of these three commits.