loadingalias and Kirill Bulatov created
Closes #43329
## Summary
This fixes `path:line:column` navigation for files containing non-ASCII
text.
Before this change, open path flows were passing the external column
directly into `go_to_singleton_buffer_point`. That happened to work for
ASCII, but it was wrong for Unicode because external columns are
user-visible character positions while the editor buffer stores columns
as UTF-8 byte offsets.
This PR adds a shared text layer conversion for external row/column
coordinates and uses it in the affected open-path flows:
- file finder navigation
- recent project remote connection navigation
- recent project remote server navigation
It also adds regression coverage for the Unicode case that originally
failed.
As a small - necessary - prerequisite, this also adds
`remote_connection` test support to `file_finder`'s dev-deps so the
local regression test can build and run on this branch. That follows the
same feature mismatch pattern previously fixed in #48280. I wasn't able
to locally verify the tests/etc. w/o the addition... so I've rolled it
into this PR. Tests are green.
The earlier attempt in #47093 was headed in the right direction, but it
did not land and did not include the final regression coverage requested
in review.
## Verification
- `cargo fmt --all -- --check`
- `./script/clippy -p text`
- `./script/clippy -p file_finder`
- `./script/clippy -p recent_projects`
- `cargo test -p file_finder --lib --no-run`
- `cargo test -p file_finder
file_finder_tests::test_row_column_numbers_query_inside_file -- --exact`
- `cargo test -p file_finder
file_finder_tests::test_row_column_numbers_query_inside_unicode_file --
--exact`
- `cargo test -p text
tests::test_point_for_row_and_column_from_external_source -- --exact`
## Manual
I reproduced locally on my machine (macOS) w/ a stateless launch using a
Unicode file (Cyrillic)
Before:
- `:1:5` landed too far left
- `:1:10` landed around the 4th visible Cyrillic character
After:
- `:1:5` lands after 4 visible characters / before the 5th
- `:1:10` lands after 9 visible characters / before the 10th
Release Notes:
- Fixed `path:line:column` navigation so non-ASCII columns land on the
correct character.
---------
Co-authored-by: Kirill Bulatov <kirill@zed.dev>Cargo.lock | 2
crates/file_finder/Cargo.toml | 1
crates/file_finder/src/file_finder.rs | 8 +
crates/file_finder/src/file_finder_tests.rs | 85 ++++++++++++++++++
crates/go_to_line/Cargo.toml | 1
crates/go_to_line/src/go_to_line.rs | 36 ++-----
crates/multi_buffer/src/multi_buffer.rs | 2
crates/multi_buffer/src/multi_buffer_tests.rs | 24 +++++
crates/recent_projects/src/remote_connections.rs | 8 +
crates/recent_projects/src/remote_servers.rs | 15 +-
crates/text/src/tests.rs | 18 +++
crates/text/src/text.rs | 31 ++++++
12 files changed, 192 insertions(+), 39 deletions(-)
@@ -6296,7 +6296,6 @@ dependencies = [
"serde",
"serde_json",
"settings",
- "text",
"theme",
"ui",
"util",
@@ -7521,6 +7520,7 @@ dependencies = [
"indoc",
"language",
"menu",
+ "multi_buffer",
"project",
"rope",
"serde",
@@ -28,7 +28,6 @@ picker.workspace = true
project.workspace = true
settings.workspace = true
serde.workspace = true
-text.workspace = true
theme.workspace = true
ui.workspace = true
util.workspace = true
@@ -35,7 +35,6 @@ use std::{
atomic::{self, AtomicBool},
},
};
-use text::Point;
use ui::{
ButtonLike, ContextMenu, HighlightedLabel, Indicator, KeyBinding, ListItem, ListItemSpacing,
PopoverMenu, PopoverMenuHandle, TintColor, Tooltip, prelude::*,
@@ -1700,7 +1699,12 @@ impl PickerDelegate for FileFinderDelegate {
active_editor
.downgrade()
.update_in(cx, |editor, window, cx| {
- editor.go_to_singleton_buffer_point(Point::new(row, col), window, cx);
+ let Some(buffer) = editor.buffer().read(cx).as_singleton() else {
+ return;
+ };
+ let buffer_snapshot = buffer.read(cx).snapshot();
+ let point = buffer_snapshot.point_from_external_input(row, col);
+ editor.go_to_singleton_buffer_point(point, window, cx);
})
.log_err();
}
@@ -521,6 +521,91 @@ async fn test_row_column_numbers_query_inside_file(cx: &mut TestAppContext) {
});
}
+#[gpui::test]
+async fn test_row_column_numbers_query_inside_unicode_file(cx: &mut TestAppContext) {
+ let app_state = init_test(cx);
+
+ let first_file_name = "first.rs";
+ let first_file_contents = "aéøbcdef";
+ app_state
+ .fs
+ .as_fake()
+ .insert_tree(
+ path!("/src"),
+ json!({
+ "test": {
+ first_file_name: first_file_contents,
+ "second.rs": "// Second Rust file",
+ }
+ }),
+ )
+ .await;
+
+ let project = Project::test(app_state.fs.clone(), [path!("/src").as_ref()], cx).await;
+
+ let (picker, workspace, cx) = build_find_picker(project, cx);
+
+ let file_query = &first_file_name[..3];
+ let file_row = 1;
+ let file_column = 5;
+ let query_inside_file = format!("{file_query}:{file_row}:{file_column}");
+ picker
+ .update_in(cx, |finder, window, cx| {
+ finder
+ .delegate
+ .update_matches(query_inside_file.to_string(), window, cx)
+ })
+ .await;
+ picker.update(cx, |finder, _| {
+ assert_match_at_position(finder, 1, &query_inside_file.to_string());
+ let finder = &finder.delegate;
+ assert_eq!(finder.matches.len(), 2);
+ let latest_search_query = finder
+ .latest_search_query
+ .as_ref()
+ .expect("Finder should have a query after the update_matches call");
+ assert_eq!(latest_search_query.raw_query, query_inside_file);
+ assert_eq!(latest_search_query.file_query_end, Some(file_query.len()));
+ assert_eq!(latest_search_query.path_position.row, Some(file_row));
+ assert_eq!(latest_search_query.path_position.column, Some(file_column));
+ });
+
+ cx.dispatch_action(Confirm);
+
+ let editor = cx.update(|_, cx| workspace.read(cx).active_item_as::<Editor>(cx).unwrap());
+ cx.executor().advance_clock(Duration::from_secs(2));
+
+ let expected_column = first_file_contents
+ .chars()
+ .take(file_column as usize - 1)
+ .map(|character| character.len_utf8())
+ .sum::<usize>();
+
+ editor.update(cx, |editor, cx| {
+ let all_selections = editor.selections.all_adjusted(&editor.display_snapshot(cx));
+ assert_eq!(
+ all_selections.len(),
+ 1,
+ "Expected to have 1 selection (caret) after file finder confirm, but got: {all_selections:?}"
+ );
+ let caret_selection = all_selections.into_iter().next().unwrap();
+ assert_eq!(
+ caret_selection.start, caret_selection.end,
+ "Caret selection should have its start and end at the same position"
+ );
+ assert_eq!(
+ file_row,
+ caret_selection.start.row + 1,
+ "Query inside file should get caret with the same focus row"
+ );
+ assert_eq!(
+ expected_column,
+ caret_selection.start.column as usize,
+ "Query inside file should map user-visible columns to byte offsets for Unicode text"
+ );
+ });
+}
+
#[gpui::test]
async fn test_row_column_numbers_query_outside_file(cx: &mut TestAppContext) {
let app_state = init_test(cx);
@@ -17,6 +17,7 @@ editor.workspace = true
gpui.workspace = true
language.workspace = true
menu.workspace = true
+multi_buffer.workspace = true
serde.workspace = true
settings.workspace = true
text.workspace = true
@@ -2,7 +2,7 @@ pub mod cursor_position;
use cursor_position::UserCaretPosition;
use editor::{
- Anchor, Editor, MultiBufferSnapshot, RowHighlightOptions, SelectionEffects, ToOffset, ToPoint,
+ Anchor, Editor, MultiBufferSnapshot, RowHighlightOptions, SelectionEffects, ToPoint,
actions::Tab,
scroll::{Autoscroll, ScrollOffset},
};
@@ -11,6 +11,7 @@ use gpui::{
Subscription, div, prelude::*,
};
use language::Buffer;
+use multi_buffer::MultiBufferRow;
use text::{Bias, Point};
use theme::ActiveTheme;
use ui::prelude::*;
@@ -228,31 +229,14 @@ impl GoToLine {
let row = query_row.saturating_sub(1);
let character = query_char.unwrap_or(0).saturating_sub(1);
- let start_offset = Point::new(row, 0).to_offset(snapshot);
- const MAX_BYTES_IN_UTF_8: u32 = 4;
- let max_end_offset = snapshot
- .clip_point(
- Point::new(row, character * MAX_BYTES_IN_UTF_8 + 1),
- Bias::Right,
- )
- .to_offset(snapshot);
-
- let mut chars_to_iterate = character;
- let mut end_offset = start_offset;
- 'outer: for text_chunk in snapshot.text_for_range(start_offset..max_end_offset) {
- let mut offset_increment = 0;
- for c in text_chunk.chars() {
- if chars_to_iterate == 0 {
- end_offset += offset_increment;
- break 'outer;
- } else {
- chars_to_iterate -= 1;
- offset_increment += c.len_utf8();
- }
- }
- end_offset += offset_increment;
- }
- Some(snapshot.anchor_before(snapshot.clip_offset(end_offset, Bias::Left)))
+ let target_multi_buffer_row = MultiBufferRow(row);
+ let (buffer_snapshot, target_in_buffer, _) = snapshot.point_to_buffer_point(Point::new(
+ target_multi_buffer_row.min(snapshot.max_row()).0,
+ 0,
+ ))?;
+ let target_point =
+ buffer_snapshot.point_from_external_input(target_in_buffer.row, character);
+ Some(snapshot.anchor_before(target_point))
}
fn relative_line_from_query(&self, cx: &App) -> Option<i32> {
@@ -2141,7 +2141,7 @@ impl MultiBuffer {
if point < start {
found = Some((start, excerpt_id));
}
- if point > end {
+ if point >= end {
found = Some((end, excerpt_id));
}
}
@@ -72,6 +72,30 @@ fn test_singleton(cx: &mut App) {
assert_consistent_line_numbers(&snapshot);
}
+#[gpui::test]
+fn test_buffer_point_to_anchor_at_end_of_singleton_buffer(cx: &mut App) {
+ let buffer = cx.new(|cx| Buffer::local("abc", cx));
+ let multibuffer = cx.new(|cx| MultiBuffer::singleton(buffer.clone(), cx));
+
+ let excerpt_id = multibuffer
+ .read(cx)
+ .excerpt_ids()
+ .into_iter()
+ .next()
+ .unwrap();
+ let anchor = multibuffer
+ .read(cx)
+ .buffer_point_to_anchor(&buffer, Point::new(0, 3), cx);
+
+ assert_eq!(
+ anchor,
+ Some(Anchor::in_buffer(
+ excerpt_id,
+ buffer.read(cx).snapshot().anchor_after(Point::new(0, 3)),
+ ))
+ );
+}
+
#[gpui::test]
fn test_remote(cx: &mut App) {
let host_buffer = cx.new(|cx| Buffer::local("a", cx));
@@ -10,7 +10,6 @@ use extension_host::ExtensionStore;
use futures::{FutureExt as _, channel::oneshot, select};
use gpui::{AppContext, AsyncApp, PromptLevel, WindowHandle};
-use language::Point;
use project::trusted_worktrees;
use remote::{
DockerConnectionOptions, Interactive, RemoteConnection, RemoteConnectionOptions,
@@ -458,7 +457,12 @@ pub fn navigate_to_positions(
active_editor.update(cx, |editor, cx| {
let row = row.saturating_sub(1);
let col = path.column.unwrap_or(0).saturating_sub(1);
- editor.go_to_singleton_buffer_point(Point::new(row, col), window, cx);
+ let Some(buffer) = editor.buffer().read(cx).as_singleton() else {
+ return;
+ };
+ let buffer_snapshot = buffer.read(cx).snapshot();
+ let point = buffer_snapshot.point_from_external_input(row, col);
+ editor.go_to_singleton_buffer_point(point, window, cx);
});
})
.ok();
@@ -17,7 +17,6 @@ use gpui::{
EventEmitter, FocusHandle, Focusable, PromptLevel, ScrollHandle, Subscription, Task,
WeakEntity, Window, canvas,
};
-use language::Point;
use log::{debug, info};
use open_path_prompt::OpenPathDelegate;
use paths::{global_ssh_config_file, user_ssh_config_file};
@@ -519,11 +518,15 @@ impl ProjectPicker {
active_editor.update(cx, |editor, cx| {
let row = row.saturating_sub(1);
let col = path.column.unwrap_or(0).saturating_sub(1);
- editor.go_to_singleton_buffer_point(
- Point::new(row, col),
- window,
- cx,
- );
+ let Some(buffer) =
+ editor.buffer().read(cx).as_singleton()
+ else {
+ return;
+ };
+ let buffer_snapshot = buffer.read(cx).snapshot();
+ let point =
+ buffer_snapshot.point_from_external_input(row, col);
+ editor.go_to_singleton_buffer_point(point, window, cx);
});
})
.ok();
@@ -30,6 +30,24 @@ fn test_edit() {
assert_eq!(buffer.text(), "ghiamnoef");
}
+#[test]
+fn test_point_for_row_and_column_from_external_source() {
+ let buffer = Buffer::new(
+ ReplicaId::LOCAL,
+ BufferId::new(1).unwrap(),
+ "aéøbcdef\nsecond",
+ );
+ let snapshot = buffer.snapshot();
+
+ assert_eq!(snapshot.point_from_external_input(0, 0), Point::new(0, 0));
+ assert_eq!(snapshot.point_from_external_input(0, 4), Point::new(0, 6));
+ assert_eq!(
+ snapshot.point_from_external_input(0, 100),
+ Point::new(0, 10)
+ );
+ assert_eq!(snapshot.point_from_external_input(1, 3), Point::new(1, 3));
+}
+
#[gpui::test(iterations = 100)]
fn test_random_edits(mut rng: StdRng) {
let operations = env::var("OPERATIONS")
@@ -2254,6 +2254,37 @@ impl BufferSnapshot {
(row_end_offset - row_start_offset) as u32
}
+ /// A function to convert character offsets from e.g. user's `go.mod:22:33` input into byte-offset Point columns.
+ pub fn point_from_external_input(&self, row: u32, characters: u32) -> Point {
+ const MAX_BYTES_IN_UTF_8: u32 = 4;
+
+ let row = row.min(self.max_point().row);
+ let start = Point::new(row, 0);
+ let end = self.clip_point(
+ Point::new(
+ row,
+ characters
+ .saturating_mul(MAX_BYTES_IN_UTF_8)
+ .saturating_add(1),
+ ),
+ Bias::Right,
+ );
+ let range = start..end;
+ let mut point = range.start;
+ let mut remaining_columns = characters;
+
+ for chunk in self.text_for_range(range) {
+ for character in chunk.chars() {
+ if remaining_columns == 0 {
+ return point;
+ }
+ remaining_columns -= 1;
+ point.column += character.len_utf8() as u32;
+ }
+ }
+ point
+ }
+
pub fn line_indents_in_row_range(
&self,
row_range: Range<u32>,