From f0c7e62adc5f69866eb6434b78d552780adf47d2 Mon Sep 17 00:00:00 2001 From: lord Date: Mon, 18 Nov 2024 18:32:43 -0500 Subject: [PATCH] Leave goal_x unchanged when moving by rows past the start or end of the document (#20705) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Perhaps this was intentional behavior, but if not, I've attempted to write this hacky fix — I noticed using the vertical arrow keys to move past the document start/end would reset the goal_x to either zero (for moving upwards) or the line width (for moving downwards). This change makes Zed match most native text fields (at least on macOS) which leave goal_x unchanged, even when hitting the end of the document. I tested this change manually. Would be happy to add automatic tests for it too, but couldn't find any existing cursor movement tests. Release Notes: - Behavior when moving vertically past the start or end of a document now matches native text fields; it no longer resets the selection goal --- crates/editor/src/editor_tests.rs | 28 ++++++++++++++++++++++++++++ crates/editor/src/movement.rs | 18 ++++++++---------- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 2e4edf98bc..01507c4e31 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -1398,6 +1398,15 @@ fn test_move_cursor_different_line_lengths(cx: &mut TestAppContext) { view.change_selections(None, cx, |s| { s.select_display_ranges([empty_range(0, "ⓐⓑⓒⓓⓔ".len())]); }); + + // moving above start of document should move selection to start of document, + // but the next move down should still be at the original goal_x + view.move_up(&MoveUp, cx); + assert_eq!( + view.selections.display_ranges(cx), + &[empty_range(0, "".len())] + ); + view.move_down(&MoveDown, cx); assert_eq!( view.selections.display_ranges(cx), @@ -1422,6 +1431,25 @@ fn test_move_cursor_different_line_lengths(cx: &mut TestAppContext) { &[empty_range(4, "ⓐⓑⓒⓓⓔ".len())] ); + // moving past end of document should not change goal_x + view.move_down(&MoveDown, cx); + assert_eq!( + view.selections.display_ranges(cx), + &[empty_range(5, "".len())] + ); + + view.move_down(&MoveDown, cx); + assert_eq!( + view.selections.display_ranges(cx), + &[empty_range(5, "".len())] + ); + + view.move_up(&MoveUp, cx); + assert_eq!( + view.selections.display_ranges(cx), + &[empty_range(4, "ⓐⓑⓒⓓⓔ".len())] + ); + view.move_up(&MoveUp, cx); assert_eq!( view.selections.display_ranges(cx), diff --git a/crates/editor/src/movement.rs b/crates/editor/src/movement.rs index 19ba147e16..52bedde2e3 100644 --- a/crates/editor/src/movement.rs +++ b/crates/editor/src/movement.rs @@ -3,7 +3,7 @@ use super::{Bias, DisplayPoint, DisplaySnapshot, SelectionGoal, ToDisplayPoint}; use crate::{scroll::ScrollAnchor, CharKind, DisplayRow, EditorStyle, RowExt, ToOffset, ToPoint}; -use gpui::{px, Pixels, WindowTextSystem}; +use gpui::{Pixels, WindowTextSystem}; use language::Point; use multi_buffer::{MultiBufferRow, MultiBufferSnapshot}; use serde::Deserialize; @@ -120,7 +120,7 @@ pub(crate) fn up_by_rows( preserve_column_at_start: bool, text_layout_details: &TextLayoutDetails, ) -> (DisplayPoint, SelectionGoal) { - let mut goal_x = match goal { + let goal_x = match goal { SelectionGoal::HorizontalPosition(x) => x.into(), SelectionGoal::WrappedHorizontalPosition((_, x)) => x.into(), SelectionGoal::HorizontalRange { end, .. } => end.into(), @@ -138,7 +138,6 @@ pub(crate) fn up_by_rows( return (start, goal); } else { point = DisplayPoint::new(DisplayRow(0), 0); - goal_x = px(0.); } let mut clipped_point = map.clip_point(point, Bias::Left); @@ -159,7 +158,7 @@ pub(crate) fn down_by_rows( preserve_column_at_end: bool, text_layout_details: &TextLayoutDetails, ) -> (DisplayPoint, SelectionGoal) { - let mut goal_x = match goal { + let goal_x = match goal { SelectionGoal::HorizontalPosition(x) => x.into(), SelectionGoal::WrappedHorizontalPosition((_, x)) => x.into(), SelectionGoal::HorizontalRange { end, .. } => end.into(), @@ -174,7 +173,6 @@ pub(crate) fn down_by_rows( return (start, goal); } else { point = map.max_point(); - goal_x = map.x_for_display_point(point, text_layout_details) } let mut clipped_point = map.clip_point(point, Bias::Right); @@ -610,7 +608,7 @@ mod tests { test::{editor_test_context::EditorTestContext, marked_display_snapshot}, Buffer, DisplayMap, DisplayRow, ExcerptRange, FoldPlaceholder, InlayId, MultiBuffer, }; - use gpui::{font, Context as _}; + use gpui::{font, px, Context as _}; use language::Capability; use project::Project; use settings::SettingsStore; @@ -977,7 +975,7 @@ mod tests { ), ( DisplayPoint::new(DisplayRow(2), 0), - SelectionGoal::HorizontalPosition(0.0) + SelectionGoal::HorizontalPosition(col_2_x.0), ), ); assert_eq!( @@ -990,7 +988,7 @@ mod tests { ), ( DisplayPoint::new(DisplayRow(2), 0), - SelectionGoal::HorizontalPosition(0.0) + SelectionGoal::HorizontalPosition(0.0), ), ); @@ -1059,7 +1057,7 @@ mod tests { let max_point_x = snapshot .x_for_display_point(DisplayPoint::new(DisplayRow(7), 2), &text_layout_details); - // Can't move down off the end + // Can't move down off the end, and attempting to do so leaves the selection goal unchanged assert_eq!( down( &snapshot, @@ -1070,7 +1068,7 @@ mod tests { ), ( DisplayPoint::new(DisplayRow(7), 2), - SelectionGoal::HorizontalPosition(max_point_x.0) + SelectionGoal::HorizontalPosition(0.0) ), ); assert_eq!(