vim: Fix incorrect bracket matching for symmetric quotes in cs operator (#52321)
## Context Closes #46698 This PR fixes a bug where the `cs` (change surrounds) operator fails on symmetric delimiters like quotes. In a sequence like `c s ' "`, Zed previously performed two independent searches. For a line like `I'm 'goˇod'`, the first step (`cs'`) correctly moves the cursor to the second quote: `I'm ˇ'good'`. However, when the replacement character `"` is pressed, `change_surrounds` would perform another scan from the new cursor position. Because quotes are symmetric, this second search would incorrectly match `'m '` as the target pair, leading to a broken result like `I"m "good'`. I've refactored the workflow to ensure the search happens only once. `prepare_and_move_to_valid_bracket_pair` now computes and stores the `Anchor` positions of the detected pair directly into the operator state. `change_surrounds` then simply reuses these anchors instead of re-executing the search. This ensures correctness for quotes while remaining consistent with Vim/Neovim cursor behavior. While this slightly increases coupling between these two functions, it is an intentional trade-off since they exclusively serve the `cs` operation. ## How to Review The main changes are in `crates/vim/src/surrounds.rs`. I renamed `check_and_move_to_valid_bracket_pair` to `prepare_and_move_to_valid_bracket_pair` and updated it to return the detected bracket anchors. In `change_surrounds`, I removed the redundant search logic and updated it to perform the replacement using the provided anchors. You can also see the updated `Operator::ChangeSurrounds` enum variant in `crates/vim/src/state.rs` which now carries the anchor data. ## Self-Review Checklist <!-- Check before requesting review: --> - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed an issue where the `cs` Vim operator incorrectly identified symmetric quotes in certain contexts. --------- Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>
This commit is contained in:
Xin ZhaoandConrad Irwin authored and GitHub committed
1 parent
a5e78b02de
commit
adfcb45abe
4 files changed
+133
-126
No files matched your search
@@ -563,10 +563,13 @@ impl Vim {
|
||||
waiting_operator = Some(Operator::DeleteSurrounds);
|
||||
}
|
||||
Some(Operator::ChangeSurrounds { target: None, .. }) => {
|
||||
if self.check_and_move_to_valid_bracket_pair(object, window, cx) {
|
||||
let bracket_anchors =
|
||||
self.prepare_and_move_to_valid_bracket_pair(object, window, cx);
|
||||
if !bracket_anchors.is_empty() {
|
||||
waiting_operator = Some(Operator::ChangeSurrounds {
|
||||
target: Some(object),
|
||||
opening,
|
||||
bracket_anchors,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
@@ -112,6 +112,8 @@ pub enum Operator {
|
||||
/// Represents whether the opening bracket was used for the target
|
||||
/// object.
|
||||
opening: bool,
|
||||
/// Computed anchors for the opening and closing bracket characters,
|
||||
bracket_anchors: Vec<Option<(Anchor, Anchor)>>,
|
||||
},
|
||||
DeleteSurrounds,
|
||||
Mark,
|
||||
|
||||
+120
-123
@@ -4,7 +4,7 @@ use crate::{
|
||||
object::{Object, surrounding_markers},
|
||||
state::Mode,
|
||||
};
|
||||
use editor::{Bias, MultiBufferOffset, movement};
|
||||
use editor::{Anchor, Bias, MultiBufferOffset, ToOffset, movement};
|
||||
use gpui::{Context, Window};
|
||||
use language::BracketPair;
|
||||
|
||||
@@ -275,6 +275,7 @@ impl Vim {
|
||||
text: Arc<str>,
|
||||
target: Object,
|
||||
opening: bool,
|
||||
bracket_anchors: Vec<Option<(Anchor, Anchor)>>,
|
||||
window: &mut Window,
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
@@ -301,96 +302,68 @@ impl Vim {
|
||||
will_replace_pair.start == will_replace_pair.end || !opening;
|
||||
|
||||
let display_map = editor.display_snapshot(cx);
|
||||
let selections = editor.selections.all_adjusted_display(&display_map);
|
||||
let mut edits = Vec::new();
|
||||
let mut anchors = Vec::new();
|
||||
|
||||
for selection in &selections {
|
||||
let start = selection.start.to_offset(&display_map, Bias::Left);
|
||||
if let Some(range) =
|
||||
target.range(&display_map, selection.clone(), true, None)
|
||||
{
|
||||
if !target.is_multiline() {
|
||||
let is_same_row = selection.start.row() == range.start.row()
|
||||
&& selection.end.row() == range.end.row();
|
||||
if !is_same_row {
|
||||
anchors.push(start..start);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
|
||||
// Keeps track of the length of the string that is
|
||||
// going to be edited on the start so we can ensure
|
||||
// that the end replacement string does not exceed
|
||||
// this value. Helpful when dealing with newlines.
|
||||
let mut edit_len = 0;
|
||||
let mut open_range_end = MultiBufferOffset(0);
|
||||
let mut chars_and_offset = display_map
|
||||
.buffer_chars_at(range.start.to_offset(&display_map, Bias::Left))
|
||||
.peekable();
|
||||
|
||||
while let Some((ch, offset)) = chars_and_offset.next() {
|
||||
if ch.to_string() == will_replace_pair.start {
|
||||
let mut open_str = pair.start.clone();
|
||||
let start = offset;
|
||||
open_range_end = start + 1usize;
|
||||
while let Some((next_ch, _)) = chars_and_offset.next()
|
||||
&& next_ch == ' '
|
||||
{
|
||||
open_range_end += 1;
|
||||
|
||||
if preserve_space {
|
||||
open_str.push(next_ch);
|
||||
}
|
||||
}
|
||||
|
||||
if add_space {
|
||||
open_str.push(' ');
|
||||
};
|
||||
|
||||
edit_len = open_range_end - start;
|
||||
edits.push((start..open_range_end, open_str));
|
||||
anchors.push(start..start);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
let mut reverse_chars_and_offsets = display_map
|
||||
.reverse_buffer_chars_at(
|
||||
range.end.to_offset(&display_map, Bias::Left),
|
||||
)
|
||||
.peekable();
|
||||
while let Some((ch, offset)) = reverse_chars_and_offsets.next() {
|
||||
if ch.to_string() == will_replace_pair.end {
|
||||
let mut close_str = String::new();
|
||||
let mut start = offset;
|
||||
let end = start + 1usize;
|
||||
while let Some((next_ch, _)) = reverse_chars_and_offsets.next()
|
||||
&& next_ch == ' '
|
||||
&& close_str.len() < edit_len - 1
|
||||
&& start > open_range_end
|
||||
{
|
||||
start -= 1;
|
||||
|
||||
if preserve_space {
|
||||
close_str.push(next_ch);
|
||||
}
|
||||
}
|
||||
|
||||
if add_space {
|
||||
close_str.push(' ');
|
||||
};
|
||||
|
||||
close_str.push_str(&pair.end);
|
||||
edits.push((start..end, close_str));
|
||||
break;
|
||||
}
|
||||
}
|
||||
} else {
|
||||
anchors.push(start..start);
|
||||
// Collect (open_offset, close_offset) pairs to replace from the
|
||||
// pre-computed anchors stored during check_and_move_to_valid_bracket_pair.
|
||||
let mut pairs_to_replace: Vec<(MultiBufferOffset, MultiBufferOffset)> =
|
||||
Vec::new();
|
||||
let snapshot = display_map.buffer_snapshot();
|
||||
for anchors in &bracket_anchors {
|
||||
let Some((open_anchor, close_anchor)) = anchors else {
|
||||
continue;
|
||||
};
|
||||
let pair = (
|
||||
open_anchor.to_offset(&snapshot),
|
||||
close_anchor.to_offset(&snapshot),
|
||||
);
|
||||
if !pairs_to_replace.contains(&pair) {
|
||||
pairs_to_replace.push(pair);
|
||||
}
|
||||
}
|
||||
|
||||
for (open_offset, close_offset) in pairs_to_replace {
|
||||
let mut open_str = pair.start.clone();
|
||||
let mut chars_and_offset =
|
||||
display_map.buffer_chars_at(open_offset).peekable();
|
||||
chars_and_offset.next(); // skip the bracket itself
|
||||
let mut open_range_end = open_offset + 1usize;
|
||||
while let Some((next_ch, _)) = chars_and_offset.next()
|
||||
&& next_ch == ' '
|
||||
{
|
||||
open_range_end += 1;
|
||||
if preserve_space {
|
||||
open_str.push(next_ch);
|
||||
}
|
||||
}
|
||||
if add_space {
|
||||
open_str.push(' ');
|
||||
}
|
||||
let edit_len = open_range_end - open_offset;
|
||||
edits.push((open_offset..open_range_end, open_str));
|
||||
|
||||
let mut close_str = String::new();
|
||||
let close_end = close_offset + 1usize;
|
||||
let mut close_start = close_offset;
|
||||
for (next_ch, _) in display_map.reverse_buffer_chars_at(close_offset) {
|
||||
if next_ch != ' '
|
||||
|| close_str.len() >= edit_len - 1
|
||||
|| close_start <= open_range_end
|
||||
{
|
||||
break;
|
||||
}
|
||||
close_start -= 1;
|
||||
if preserve_space {
|
||||
close_str.push(next_ch);
|
||||
}
|
||||
}
|
||||
if add_space {
|
||||
close_str.push(' ');
|
||||
}
|
||||
close_str.push_str(&pair.end);
|
||||
edits.push((close_start..close_end, close_str));
|
||||
}
|
||||
|
||||
let stable_anchors = editor
|
||||
.selections
|
||||
.disjoint_anchors_arc()
|
||||
@@ -411,67 +384,83 @@ impl Vim {
|
||||
}
|
||||
}
|
||||
|
||||
/// Checks if any of the current cursors are surrounded by a valid pair of brackets.
|
||||
/// **Only intended for use by the `cs` (change surrounds) operator.**
|
||||
///
|
||||
/// This method supports multiple cursors and checks each cursor for a valid pair of brackets.
|
||||
/// A pair of brackets is considered valid if it is well-formed and properly closed.
|
||||
/// For each cursor, checks whether it is surrounded by a valid bracket pair for the given
|
||||
/// object. Moves each cursor to the opening bracket of its found pair, and returns a
|
||||
/// `Vec<Option<(Anchor, Anchor)>>` with one entry per selection containing the pre-computed
|
||||
/// open and close bracket positions.
|
||||
///
|
||||
/// If a valid pair of brackets is found, the method returns `true` and the cursor is automatically moved to the start of the bracket pair.
|
||||
/// If no valid pair of brackets is found for any cursor, the method returns `false`.
|
||||
pub fn check_and_move_to_valid_bracket_pair(
|
||||
/// Storing these anchors avoids re-running the bracket search from the moved cursor position,
|
||||
/// which can misidentify the opening bracket for symmetric quote characters when the same
|
||||
/// character appears earlier on the line (e.g. `I'm 'good'`).
|
||||
///
|
||||
/// Returns an empty `Vec` if no valid pair was found for any cursor.
|
||||
pub fn prepare_and_move_to_valid_bracket_pair(
|
||||
&mut self,
|
||||
object: Object,
|
||||
window: &mut Window,
|
||||
cx: &mut Context<Self>,
|
||||
) -> bool {
|
||||
let mut valid = false;
|
||||
) -> Vec<Option<(Anchor, Anchor)>> {
|
||||
let mut matched_pair_anchors: Vec<Option<(Anchor, Anchor)>> = Vec::new();
|
||||
if let Some(pair) = self.object_to_bracket_pair(object, cx) {
|
||||
self.update_editor(cx, |_, editor, cx| {
|
||||
editor.transact(window, cx, |editor, window, cx| {
|
||||
editor.set_clip_at_line_ends(false, cx);
|
||||
let display_map = editor.display_snapshot(cx);
|
||||
let selections = editor.selections.all_adjusted_display(&display_map);
|
||||
let mut anchors = Vec::new();
|
||||
let mut updated_cursor_ranges = Vec::new();
|
||||
|
||||
for selection in &selections {
|
||||
let start = selection.start.to_offset(&display_map, Bias::Left);
|
||||
if let Some(range) =
|
||||
object.range(&display_map, selection.clone(), true, None)
|
||||
{
|
||||
// If the current parenthesis object is single-line,
|
||||
// then we need to filter whether it is the current line or not
|
||||
if object.is_multiline()
|
||||
|| (!object.is_multiline()
|
||||
&& selection.start.row() == range.start.row()
|
||||
&& selection.end.row() == range.end.row())
|
||||
{
|
||||
valid = true;
|
||||
let chars_and_offset = display_map
|
||||
.buffer_chars_at(
|
||||
range.start.to_offset(&display_map, Bias::Left),
|
||||
)
|
||||
.peekable();
|
||||
for (ch, offset) in chars_and_offset {
|
||||
if ch.to_string() == pair.start {
|
||||
anchors.push(offset..offset);
|
||||
break;
|
||||
}
|
||||
}
|
||||
} else {
|
||||
anchors.push(start..start)
|
||||
}
|
||||
let in_range = object
|
||||
.range(&display_map, selection.clone(), true, None)
|
||||
.filter(|range| {
|
||||
object.is_multiline()
|
||||
|| (selection.start.row() == range.start.row()
|
||||
&& selection.end.row() == range.end.row())
|
||||
});
|
||||
let Some(range) = in_range else {
|
||||
updated_cursor_ranges.push(start..start);
|
||||
matched_pair_anchors.push(None);
|
||||
continue;
|
||||
};
|
||||
|
||||
let range_start = range.start.to_offset(&display_map, Bias::Left);
|
||||
let range_end = range.end.to_offset(&display_map, Bias::Left);
|
||||
let open_offset = display_map
|
||||
.buffer_chars_at(range_start)
|
||||
.find(|(ch, _)| ch.to_string() == pair.start)
|
||||
.map(|(_, offset)| offset);
|
||||
let close_offset = display_map
|
||||
.reverse_buffer_chars_at(range_end)
|
||||
.find(|(ch, _)| ch.to_string() == pair.end)
|
||||
.map(|(_, offset)| offset);
|
||||
|
||||
if let (Some(open), Some(close)) = (open_offset, close_offset) {
|
||||
let snapshot = &display_map.buffer_snapshot();
|
||||
updated_cursor_ranges.push(open..open);
|
||||
matched_pair_anchors.push(Some((
|
||||
snapshot.anchor_before(open),
|
||||
snapshot.anchor_before(close),
|
||||
)));
|
||||
} else {
|
||||
anchors.push(start..start)
|
||||
updated_cursor_ranges.push(start..start);
|
||||
matched_pair_anchors.push(None);
|
||||
}
|
||||
}
|
||||
editor.change_selections(Default::default(), window, cx, |s| {
|
||||
s.select_ranges(anchors);
|
||||
s.select_ranges(updated_cursor_ranges);
|
||||
});
|
||||
editor.set_clip_at_line_ends(true, cx);
|
||||
|
||||
if !matched_pair_anchors.iter().any(|a| a.is_some()) {
|
||||
matched_pair_anchors.clear();
|
||||
}
|
||||
});
|
||||
});
|
||||
}
|
||||
valid
|
||||
matched_pair_anchors
|
||||
}
|
||||
|
||||
fn object_to_bracket_pair(
|
||||
@@ -1285,6 +1274,14 @@ mod test {
|
||||
cx.set_state(indoc! {"'ˇfoobar'"}, Mode::Normal);
|
||||
cx.simulate_keystrokes("c s ' }");
|
||||
cx.assert_state(indoc! {"ˇ{foobar}"}, Mode::Normal);
|
||||
|
||||
cx.set_state(indoc! {"I'm 'goˇod'"}, Mode::Normal);
|
||||
cx.simulate_keystrokes("c s ' \"");
|
||||
cx.assert_state(indoc! {"I'm ˇ\"good\""}, Mode::Normal);
|
||||
|
||||
cx.set_state(indoc! {"I'm 'goˇod'"}, Mode::Normal);
|
||||
cx.simulate_keystrokes("c s ' {");
|
||||
cx.assert_state(indoc! {"I'm ˇ{ good }"}, Mode::Normal);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
|
||||
@@ -770,6 +770,7 @@ impl Vim {
|
||||
Operator::ChangeSurrounds {
|
||||
target: action.target,
|
||||
opening: false,
|
||||
bracket_anchors: Vec::new(),
|
||||
},
|
||||
window,
|
||||
cx,
|
||||
@@ -1980,10 +1981,14 @@ impl Vim {
|
||||
}
|
||||
_ => self.clear_operator(window, cx),
|
||||
},
|
||||
Some(Operator::ChangeSurrounds { target, opening }) => match self.mode {
|
||||
Some(Operator::ChangeSurrounds {
|
||||
target,
|
||||
opening,
|
||||
bracket_anchors,
|
||||
}) => match self.mode {
|
||||
Mode::Normal => {
|
||||
if let Some(target) = target {
|
||||
self.change_surrounds(text, target, opening, window, cx);
|
||||
self.change_surrounds(text, target, opening, bracket_anchors, window, cx);
|
||||
self.clear_operator(window, cx);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user