From e5bb2c67902aab64d5d284a9b05cf1836fae91c6 Mon Sep 17 00:00:00 2001 From: loadingalias <138315197+loadingalias@users.noreply.github.com> Date: Mon, 16 Mar 2026 11:35:59 -0400 Subject: [PATCH] Fix non-ASCII path:line:column navigation (#51238) 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 --- 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 ++++++ .../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(-) diff --git a/Cargo.lock b/Cargo.lock index 467463433f2bb7c4bb90894095ef6216ef7c98a4..1c69b903c10345a4a78b4cab813baf8c899b7aad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -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", diff --git a/crates/file_finder/Cargo.toml b/crates/file_finder/Cargo.toml index 0876d95a7b044d2a4ce5bf8be964c4057725f827..80e466ac4c571ede217aa734a7862becd08e72ba 100644 --- a/crates/file_finder/Cargo.toml +++ b/crates/file_finder/Cargo.toml @@ -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 diff --git a/crates/file_finder/src/file_finder.rs b/crates/file_finder/src/file_finder.rs index 3dcd052c34acc3a28650c58079c82499b7e94c85..4302669ddc11c94f7df128534217d00c27ef083a 100644 --- a/crates/file_finder/src/file_finder.rs +++ b/crates/file_finder/src/file_finder.rs @@ -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(); } diff --git a/crates/file_finder/src/file_finder_tests.rs b/crates/file_finder/src/file_finder_tests.rs index cd9f22ef9e9c09a828ceced449ebafb9c3c2e12b..3f9d579b03c9aa2abeb408bdf6b77cf5e69de003 100644 --- a/crates/file_finder/src/file_finder_tests.rs +++ b/crates/file_finder/src/file_finder_tests.rs @@ -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::(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::(); + + 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); diff --git a/crates/go_to_line/Cargo.toml b/crates/go_to_line/Cargo.toml index 58c58dc389e37210063efb55337fc385cc0ad435..c07656985380c93355a4c8429dcf1135acf93d56 100644 --- a/crates/go_to_line/Cargo.toml +++ b/crates/go_to_line/Cargo.toml @@ -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 diff --git a/crates/go_to_line/src/go_to_line.rs b/crates/go_to_line/src/go_to_line.rs index 79c4e54700ccec7575c825ecae6a1bb05419b6fb..a5332e96c731a29027ea6a69288d7d9556cb2da0 100644 --- a/crates/go_to_line/src/go_to_line.rs +++ b/crates/go_to_line/src/go_to_line.rs @@ -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 { diff --git a/crates/multi_buffer/src/multi_buffer.rs b/crates/multi_buffer/src/multi_buffer.rs index 2b4428b36a8c8f3b91f53425981bfe27480f7e64..e3b578c9d5ff274f35ddced24a49f4b31d819bf1 100644 --- a/crates/multi_buffer/src/multi_buffer.rs +++ b/crates/multi_buffer/src/multi_buffer.rs @@ -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)); } } diff --git a/crates/multi_buffer/src/multi_buffer_tests.rs b/crates/multi_buffer/src/multi_buffer_tests.rs index c169297e2d5e170cc6cd7d85838c36c3e6bcf71e..8b708968f21b103ee3c7882c01cd1edf6884af03 100644 --- a/crates/multi_buffer/src/multi_buffer_tests.rs +++ b/crates/multi_buffer/src/multi_buffer_tests.rs @@ -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)); diff --git a/crates/recent_projects/src/remote_connections.rs b/crates/recent_projects/src/remote_connections.rs index b5af1a110a5b0ebae6cb8e6e035791b564e15527..5275cdaa1526a670e817ff3b229d7e92b94bb309 100644 --- a/crates/recent_projects/src/remote_connections.rs +++ b/crates/recent_projects/src/remote_connections.rs @@ -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(); diff --git a/crates/recent_projects/src/remote_servers.rs b/crates/recent_projects/src/remote_servers.rs index d4cfb6520e6f73592ede5abcacb558967d10dbc7..4569492d4c73b6e8087cf8363db805a645e5314e 100644 --- a/crates/recent_projects/src/remote_servers.rs +++ b/crates/recent_projects/src/remote_servers.rs @@ -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(); diff --git a/crates/text/src/tests.rs b/crates/text/src/tests.rs index 194ac2a40d5ac96a39177eedd35b991ded30de38..d5d3facb9b97d09e4724369bd17df639e2b6ac42 100644 --- a/crates/text/src/tests.rs +++ b/crates/text/src/tests.rs @@ -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") diff --git a/crates/text/src/text.rs b/crates/text/src/text.rs index a991a72df40c502a90aa0b82191b37c54b3f8de2..c054a4caacd34904090397612474be55c48ffbfd 100644 --- a/crates/text/src/text.rs +++ b/crates/text/src/text.rs @@ -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,