From 9918b6cadea55feae131c9e4727bf7c55c7e75a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Marcos?= Date: Sat, 22 Mar 2025 06:06:13 -0300 Subject: [PATCH] Scroll to follow expanding part of `editor::SelectLargerSyntaxNode` (#27295) When the selection grows both ways, the new code prioritizes the top part instead of bottom one, this is usually more helpful considering that most programming language grammars tend to define tokens right before large delimited blocks, and rarely after (because humans and parsers read from top to bottom). Also, revert selection when convenient, so you have more control over what you're selecting, looking at the selection `head` is commonly more convenient than at the `tail`. Release Notes: - Improve scrolling of `editor::SelectLargerSyntaxNode` for better visibility. --- crates/editor/src/editor.rs | 158 +++++++++++++++++++++---- crates/editor/src/editor_tests.rs | 6 +- crates/editor/src/scroll.rs | 22 ++++ crates/editor/src/scroll/actions.rs | 105 +++------------- crates/editor/src/scroll/autoscroll.rs | 15 +++ 5 files changed, 193 insertions(+), 113 deletions(-) diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 9ba9a659e0e9cc10203065349048940fb83fc579..eb2575ed857d0689c56607c13603c05da51eb4c9 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -188,16 +188,12 @@ use ui::{ use util::{maybe, post_inc, RangeExt, ResultExt, TryFutureExt}; use workspace::{ item::{ItemHandle, PreviewTabsSettings}, - ItemId, RestoreOnStartupBehavior, SERIALIZATION_THROTTLE_TIME, -}; -use workspace::{ notifications::{DetachAndPromptErr, NotificationId, NotifyTaskExt}, - WorkspaceSettings, -}; -use workspace::{ - searchable::SearchEvent, ItemNavHistory, SplitDirection, ViewId, Workspace, WorkspaceId, + searchable::SearchEvent, + Item as WorkspaceItem, ItemId, ItemNavHistory, OpenInTerminal, OpenTerminal, + RestoreOnStartupBehavior, SplitDirection, TabBarSettings, Toast, ViewId, Workspace, + WorkspaceId, WorkspaceSettings, SERIALIZATION_THROTTLE_TIME, }; -use workspace::{Item as WorkspaceItem, OpenInTerminal, OpenTerminal, TabBarSettings, Toast}; use crate::hover_links::{find_url, find_url_from_range}; use crate::signature_help::{SignatureHelpHiddenBy, SignatureHelpState}; @@ -641,7 +637,7 @@ pub struct Editor { selection_history: SelectionHistory, autoclose_regions: Vec, snippet_stack: InvalidationStack, - select_larger_syntax_node_stack: Vec]>>, + select_syntax_node_history: SelectSyntaxNodeHistory, ime_transaction: Option, active_diagnostics: Option, show_inline_diagnostics: bool, @@ -1032,6 +1028,42 @@ pub struct ClipboardSelection { pub first_line_indent: u32, } +// selections, scroll behavior, was newest selection reversed +type SelectSyntaxNodeHistoryState = ( + Box<[Selection]>, + SelectSyntaxNodeScrollBehavior, + bool, +); + +#[derive(Default)] +struct SelectSyntaxNodeHistory { + stack: Vec, + // disable temporarily to allow changing selections without losing the stack + pub disable_clearing: bool, +} + +impl SelectSyntaxNodeHistory { + pub fn try_clear(&mut self) { + if !self.disable_clearing { + self.stack.clear(); + } + } + + pub fn push(&mut self, selection: SelectSyntaxNodeHistoryState) { + self.stack.push(selection); + } + + pub fn pop(&mut self) -> Option { + self.stack.pop() + } +} + +enum SelectSyntaxNodeScrollBehavior { + CursorTop, + CenterSelection, + CursorBottom, +} + #[derive(Debug)] pub(crate) struct NavigationData { cursor_anchor: Anchor, @@ -1374,7 +1406,7 @@ impl Editor { selection_history: Default::default(), autoclose_regions: Default::default(), snippet_stack: Default::default(), - select_larger_syntax_node_stack: Vec::new(), + select_syntax_node_history: SelectSyntaxNodeHistory::default(), ime_transaction: Default::default(), active_diagnostics: None, show_inline_diagnostics: ProjectSettings::get_global(cx).diagnostics.inline.enabled, @@ -2136,7 +2168,7 @@ impl Editor { self.add_selections_state = None; self.select_next_state = None; self.select_prev_state = None; - self.select_larger_syntax_node_stack.clear(); + self.select_syntax_node_history.try_clear(); self.invalidate_autoclose_regions(&self.selections.disjoint_anchors(), buffer); self.snippet_stack .invalidate(&self.selections.disjoint_anchors(), buffer); @@ -11819,13 +11851,19 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { + let Some(visible_row_count) = self.visible_row_count() else { + return; + }; + let old_selections: Box<[_]> = self.selections.all::(cx).into(); + if old_selections.is_empty() { + return; + } + let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); let buffer = self.buffer.read(cx).snapshot(cx); - let old_selections = self.selections.all::(cx).into_boxed_slice(); - let mut stack = mem::take(&mut self.select_larger_syntax_node_stack); let mut selected_larger_node = false; - let new_selections = old_selections + let mut new_selections = old_selections .iter() .map(|selection| { let old_range = selection.start..selection.end; @@ -11867,13 +11905,56 @@ impl Editor { }) .collect::>(); + if !selected_larger_node { + return; // don't put this call in the history + } + + // scroll based on transformation done to the last selection created by the user + let (last_old, last_new) = old_selections + .last() + .zip(new_selections.last().cloned()) + .expect("old_selections isn't empty"); + + // revert selection + let is_selection_reversed = { + let should_newest_selection_be_reversed = last_old.start != last_new.start; + new_selections.last_mut().expect("checked above").reversed = + should_newest_selection_be_reversed; + should_newest_selection_be_reversed + }; + if selected_larger_node { - stack.push(old_selections); - self.change_selections(Some(Autoscroll::fit()), window, cx, |s| { - s.select(new_selections); + self.select_syntax_node_history.disable_clearing = true; + self.change_selections(None, window, cx, |s| { + s.select(new_selections.clone()); }); - } - self.select_larger_syntax_node_stack = stack; + self.select_syntax_node_history.disable_clearing = false; + } + + let start_row = last_new.start.to_display_point(&display_map).row().0; + let end_row = last_new.end.to_display_point(&display_map).row().0; + let selection_height = end_row - start_row + 1; + let scroll_margin_rows = self.vertical_scroll_margin() as u32; + + // if fits on screen (considering margin), keep it in the middle, else, scroll to selection head + let scroll_behavior = if visible_row_count >= selection_height + scroll_margin_rows * 2 { + let middle_row = (end_row + start_row) / 2; + let selection_center = middle_row.saturating_sub(visible_row_count / 2); + self.set_scroll_top_row(DisplayRow(selection_center), window, cx); + SelectSyntaxNodeScrollBehavior::CenterSelection + } else if is_selection_reversed { + self.scroll_cursor_top(&Default::default(), window, cx); + SelectSyntaxNodeScrollBehavior::CursorTop + } else { + self.scroll_cursor_bottom(&Default::default(), window, cx); + SelectSyntaxNodeScrollBehavior::CursorBottom + }; + + self.select_syntax_node_history.push(( + old_selections, + scroll_behavior, + is_selection_reversed, + )); } pub fn select_smaller_syntax_node( @@ -11882,13 +11963,44 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { - let mut stack = mem::take(&mut self.select_larger_syntax_node_stack); - if let Some(selections) = stack.pop() { - self.change_selections(Some(Autoscroll::fit()), window, cx, |s| { + let Some(visible_row_count) = self.visible_row_count() else { + return; + }; + + if let Some((mut selections, scroll_behavior, is_selection_reversed)) = + self.select_syntax_node_history.pop() + { + let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx)); + + if let Some(selection) = selections.last_mut() { + selection.reversed = is_selection_reversed; + } + + self.select_syntax_node_history.disable_clearing = true; + self.change_selections(None, window, cx, |s| { s.select(selections.to_vec()); }); + self.select_syntax_node_history.disable_clearing = false; + + let newest = self.selections.newest::(cx); + let start_row = newest.start.to_display_point(&display_map).row().0; + let end_row = newest.end.to_display_point(&display_map).row().0; + + match scroll_behavior { + SelectSyntaxNodeScrollBehavior::CursorTop => { + self.scroll_cursor_top(&Default::default(), window, cx); + } + SelectSyntaxNodeScrollBehavior::CenterSelection => { + let middle_row = (end_row + start_row) / 2; + let selection_center = middle_row.saturating_sub(visible_row_count / 2); + // centralize the selection, not the cursor + self.set_scroll_top_row(DisplayRow(selection_center), window, cx); + } + SelectSyntaxNodeScrollBehavior::CursorBottom => { + self.scroll_cursor_bottom(&Default::default(), window, cx); + } + } } - self.select_larger_syntax_node_stack = stack; } fn refresh_runnables(&mut self, window: &mut Window, cx: &mut Context) -> Task<()> { diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index 868f53d72b920c37b00b165854f8667a554ab91c..6691ae0709a5b602a440c5f2e94d1b4a6de9c347 100644 --- a/crates/editor/src/editor_tests.rs +++ b/crates/editor/src/editor_tests.rs @@ -6033,7 +6033,7 @@ async fn test_select_larger_smaller_syntax_node(cx: &mut TestAppContext) { use mod1::mod2::{mod3, «mod4ˇ»}; fn fn_1«ˇ(param1: bool, param2: &str)» { - let var1 = "«textˇ»"; + let var1 = "«ˇtext»"; } "#}, cx, @@ -6101,7 +6101,7 @@ async fn test_select_larger_smaller_syntax_node(cx: &mut TestAppContext) { use mod1::mod2::{mod3, «mod4ˇ»}; fn fn_1«ˇ(param1: bool, param2: &str)» { - let var1 = "«textˇ»"; + let var1 = "«ˇtext»"; } "#}, cx, @@ -6170,7 +6170,7 @@ async fn test_select_larger_smaller_syntax_node(cx: &mut TestAppContext) { use mod1::mod2::«{mod3, mod4}ˇ»; fn fn_1«ˇ(param1: bool, param2: &str)» { - «let var1 = "text";ˇ» + «ˇlet var1 = "text";» } "#}, cx, diff --git a/crates/editor/src/scroll.rs b/crates/editor/src/scroll.rs index f463777b7e207a27d9b1589028b084a011a45e53..fed41e91137cea7c83ff4924c5bda96ef690fc5e 100644 --- a/crates/editor/src/scroll.rs +++ b/crates/editor/src/scroll.rs @@ -462,6 +462,28 @@ impl Editor { self.set_scroll_position_internal(scroll_position, true, false, window, cx); } + /// Scrolls so that `row` is at the top of the editor view. + pub fn set_scroll_top_row( + &mut self, + row: DisplayRow, + window: &mut Window, + cx: &mut Context, + ) { + let snapshot = self.snapshot(window, cx).display_snapshot; + let new_screen_top = DisplayPoint::new(row, 0); + let new_screen_top = new_screen_top.to_offset(&snapshot, Bias::Left); + let new_anchor = snapshot.buffer_snapshot.anchor_before(new_screen_top); + + self.set_scroll_anchor( + ScrollAnchor { + anchor: new_anchor, + offset: Default::default(), + }, + window, + cx, + ); + } + pub(crate) fn set_scroll_position_internal( &mut self, scroll_position: gpui::Point, diff --git a/crates/editor/src/scroll/actions.rs b/crates/editor/src/scroll/actions.rs index 9e829b196f1ead79e29515bd4679ff5ed3ebd06c..652670dd50030bb7bdf8347d9cdcdc0de46b4716 100644 --- a/crates/editor/src/scroll/actions.rs +++ b/crates/editor/src/scroll/actions.rs @@ -1,8 +1,8 @@ use super::Axis; use crate::{ - Autoscroll, Bias, Editor, EditorMode, NextScreen, NextScrollCursorCenterTopBottom, - ScrollAnchor, ScrollCursorBottom, ScrollCursorCenter, ScrollCursorCenterTopBottom, - ScrollCursorTop, SCROLL_CENTER_TOP_BOTTOM_DEBOUNCE_TIMEOUT, + display_map::DisplayRow, Autoscroll, Editor, EditorMode, NextScreen, + NextScrollCursorCenterTopBottom, ScrollCursorBottom, ScrollCursorCenter, + ScrollCursorCenterTopBottom, ScrollCursorTop, SCROLL_CENTER_TOP_BOTTOM_DEBOUNCE_TIMEOUT, }; use gpui::{Context, Point, Window}; @@ -40,41 +40,17 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { - let snapshot = self.snapshot(window, cx).display_snapshot; - let visible_rows = if let Some(visible_rows) = self.visible_line_count() { - visible_rows as u32 - } else { - return; - }; - - let scroll_margin_rows = self.vertical_scroll_margin() as u32; - let mut new_screen_top = self.selections.newest_display(cx).head(); - *new_screen_top.column_mut() = 0; match self.next_scroll_position { NextScrollCursorCenterTopBottom::Center => { - *new_screen_top.row_mut() = new_screen_top.row().0.saturating_sub(visible_rows / 2); + self.scroll_cursor_center(&Default::default(), window, cx); } NextScrollCursorCenterTopBottom::Top => { - *new_screen_top.row_mut() = - new_screen_top.row().0.saturating_sub(scroll_margin_rows); + self.scroll_cursor_top(&Default::default(), window, cx); } NextScrollCursorCenterTopBottom::Bottom => { - *new_screen_top.row_mut() = new_screen_top - .row() - .0 - .saturating_sub(visible_rows.saturating_sub(scroll_margin_rows)); + self.scroll_cursor_bottom(&Default::default(), window, cx); } } - self.set_scroll_anchor( - ScrollAnchor { - anchor: snapshot - .buffer_snapshot - .anchor_before(new_screen_top.to_offset(&snapshot, Bias::Left)), - offset: Default::default(), - }, - window, - cx, - ); self.next_scroll_position = self.next_scroll_position.next(); self._scroll_cursor_center_top_bottom_task = cx.spawn(async move |editor, cx| { @@ -95,23 +71,10 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { - let snapshot = self.snapshot(window, cx).display_snapshot; let scroll_margin_rows = self.vertical_scroll_margin() as u32; - - let mut new_screen_top = self.selections.newest_display(cx).head(); - *new_screen_top.row_mut() = new_screen_top.row().0.saturating_sub(scroll_margin_rows); - *new_screen_top.column_mut() = 0; - let new_screen_top = new_screen_top.to_offset(&snapshot, Bias::Left); - let new_anchor = snapshot.buffer_snapshot.anchor_before(new_screen_top); - - self.set_scroll_anchor( - ScrollAnchor { - anchor: new_anchor, - offset: Default::default(), - }, - window, - cx, - ) + let new_screen_top = self.selections.newest_display(cx).head().row().0; + let new_screen_top = new_screen_top.saturating_sub(scroll_margin_rows); + self.set_scroll_top_row(DisplayRow(new_screen_top), window, cx); } pub fn scroll_cursor_center( @@ -120,27 +83,12 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { - let snapshot = self.snapshot(window, cx).display_snapshot; - let visible_rows = if let Some(visible_rows) = self.visible_line_count() { - visible_rows as u32 - } else { + let Some(visible_rows) = self.visible_line_count().map(|count| count as u32) else { return; }; - - let mut new_screen_top = self.selections.newest_display(cx).head(); - *new_screen_top.row_mut() = new_screen_top.row().0.saturating_sub(visible_rows / 2); - *new_screen_top.column_mut() = 0; - let new_screen_top = new_screen_top.to_offset(&snapshot, Bias::Left); - let new_anchor = snapshot.buffer_snapshot.anchor_before(new_screen_top); - - self.set_scroll_anchor( - ScrollAnchor { - anchor: new_anchor, - offset: Default::default(), - }, - window, - cx, - ) + let new_screen_top = self.selections.newest_display(cx).head().row().0; + let new_screen_top = new_screen_top.saturating_sub(visible_rows / 2); + self.set_scroll_top_row(DisplayRow(new_screen_top), window, cx); } pub fn scroll_cursor_bottom( @@ -149,30 +97,13 @@ impl Editor { window: &mut Window, cx: &mut Context, ) { - let snapshot = self.snapshot(window, cx).display_snapshot; let scroll_margin_rows = self.vertical_scroll_margin() as u32; - let visible_rows = if let Some(visible_rows) = self.visible_line_count() { - visible_rows as u32 - } else { + let Some(visible_rows) = self.visible_line_count().map(|count| count as u32) else { return; }; - - let mut new_screen_top = self.selections.newest_display(cx).head(); - *new_screen_top.row_mut() = new_screen_top - .row() - .0 - .saturating_sub(visible_rows.saturating_sub(scroll_margin_rows)); - *new_screen_top.column_mut() = 0; - let new_screen_top = new_screen_top.to_offset(&snapshot, Bias::Left); - let new_anchor = snapshot.buffer_snapshot.anchor_before(new_screen_top); - - self.set_scroll_anchor( - ScrollAnchor { - anchor: new_anchor, - offset: Default::default(), - }, - window, - cx, - ) + let new_screen_top = self.selections.newest_display(cx).head().row().0; + let new_screen_top = + new_screen_top.saturating_sub(visible_rows.saturating_sub(scroll_margin_rows)); + self.set_scroll_top_row(DisplayRow(new_screen_top), window, cx); } } diff --git a/crates/editor/src/scroll/autoscroll.rs b/crates/editor/src/scroll/autoscroll.rs index bb42cec57f8b73901c51baa70da838851a29457f..f0cfb3a3c6b7a687a472c74fe1cb87639651554f 100644 --- a/crates/editor/src/scroll/autoscroll.rs +++ b/crates/editor/src/scroll/autoscroll.rs @@ -38,6 +38,16 @@ impl Autoscroll { Self::Strategy(AutoscrollStrategy::TopRelative(n)) } + /// Scrolls so that the newest cursor is at the top. + pub fn top() -> Self { + Self::Strategy(AutoscrollStrategy::Top) + } + + /// Scrolls so that the newest cursor is roughly an n-th line from the bottom. + pub fn bottom_relative(n: usize) -> Self { + Self::Strategy(AutoscrollStrategy::BottomRelative(n)) + } + /// Scrolls so that the newest cursor is at the bottom. pub fn bottom() -> Self { Self::Strategy(AutoscrollStrategy::Bottom) @@ -54,6 +64,7 @@ pub enum AutoscrollStrategy { Top, Bottom, TopRelative(usize), + BottomRelative(usize), } impl AutoscrollStrategy { @@ -213,6 +224,10 @@ impl Editor { scroll_position.y = target_top - lines as f32; self.set_scroll_position_internal(scroll_position, local, true, window, cx); } + AutoscrollStrategy::BottomRelative(lines) => { + scroll_position.y = target_bottom + lines as f32; + self.set_scroll_position_internal(scroll_position, local, true, window, cx); + } } self.scroll_manager.last_autoscroll = Some((