Reduce number of snapshots and notifies during editor scrolling (#34228)

Finn Evers and Michael Sloan created

We not do not create new snapshots anymore when autoscrolling
horizontally and also do not notify any longer should the new scroll
position match the old one.

Release Notes:

- N/A

---------

Co-authored-by: Michael Sloan <mgsloan@gmail.com>

Change summary

crates/editor/src/element.rs           | 59 ++++++++++++-----------
crates/editor/src/scroll.rs            | 29 +++++++----
crates/editor/src/scroll/autoscroll.rs | 68 ++++++++++++++++-----------
3 files changed, 89 insertions(+), 67 deletions(-)

Detailed changes

crates/editor/src/element.rs 🔗

@@ -8035,23 +8035,25 @@ impl Element for EditorElement {
                         }
                     };
 
-                    // TODO: Autoscrolling for both axes
-                    let mut autoscroll_request = None;
-                    let mut autoscroll_containing_element = false;
-                    let mut autoscroll_horizontally = false;
-                    self.editor.update(cx, |editor, cx| {
-                        autoscroll_request = editor.autoscroll_request();
-                        autoscroll_containing_element =
+                    let (
+                        autoscroll_request,
+                        autoscroll_containing_element,
+                        needs_horizontal_autoscroll,
+                    ) = self.editor.update(cx, |editor, cx| {
+                        let autoscroll_request = editor.autoscroll_request();
+                        let autoscroll_containing_element =
                             autoscroll_request.is_some() || editor.has_pending_selection();
-                        // TODO: Is this horizontal or vertical?!
-                        autoscroll_horizontally = editor.autoscroll_vertically(
-                            bounds,
-                            line_height,
-                            max_scroll_top,
-                            window,
-                            cx,
-                        );
-                        snapshot = editor.snapshot(window, cx);
+
+                        let (needs_horizontal_autoscroll, was_scrolled) = editor
+                            .autoscroll_vertically(bounds, line_height, max_scroll_top, window, cx);
+                        if was_scrolled.0 {
+                            snapshot = editor.snapshot(window, cx);
+                        }
+                        (
+                            autoscroll_request,
+                            autoscroll_containing_element,
+                            needs_horizontal_autoscroll,
+                        )
                     });
 
                     let mut scroll_position = snapshot.scroll_position();
@@ -8460,10 +8462,12 @@ impl Element for EditorElement {
                     );
 
                     self.editor.update(cx, |editor, cx| {
-                        let clamped = editor.scroll_manager.clamp_scroll_left(scroll_max.x);
+                        if editor.scroll_manager.clamp_scroll_left(scroll_max.x) {
+                            scroll_position.x = scroll_position.x.min(scroll_max.x);
+                        }
 
-                        let autoscrolled = if autoscroll_horizontally {
-                            editor.autoscroll_horizontally(
+                        if needs_horizontal_autoscroll.0
+                            && let Some(new_scroll_position) = editor.autoscroll_horizontally(
                                 start_row,
                                 editor_content_width,
                                 scroll_width,
@@ -8472,13 +8476,8 @@ impl Element for EditorElement {
                                 window,
                                 cx,
                             )
-                        } else {
-                            false
-                        };
-
-                        if clamped || autoscrolled {
-                            snapshot = editor.snapshot(window, cx);
-                            scroll_position = snapshot.scroll_position();
+                        {
+                            scroll_position = new_scroll_position;
                         }
                     });
 
@@ -8593,7 +8592,9 @@ impl Element for EditorElement {
                                 }
                             } else {
                                 log::error!(
-                                    "bug: line_ix {} is out of bounds - row_infos.len(): {}, line_layouts.len(): {}, crease_trailers.len(): {}",
+                                    "bug: line_ix {} is out of bounds - row_infos.len(): {}, \
+                                    line_layouts.len(): {}, \
+                                    crease_trailers.len(): {}",
                                     line_ix,
                                     row_infos.len(),
                                     line_layouts.len(),
@@ -8839,7 +8840,7 @@ impl Element for EditorElement {
                             underline: None,
                             strikethrough: None,
                         }],
-                        None
+                        None,
                     );
                     let space_invisible = window.text_system().shape_line(
                         "•".into(),
@@ -8852,7 +8853,7 @@ impl Element for EditorElement {
                             underline: None,
                             strikethrough: None,
                         }],
-                        None
+                        None,
                     );
 
                     let mode = snapshot.mode.clone();

crates/editor/src/scroll.rs 🔗

@@ -27,6 +27,8 @@ use workspace::{ItemId, WorkspaceId};
 pub const SCROLL_EVENT_SEPARATION: Duration = Duration::from_millis(28);
 const SCROLLBAR_SHOW_INTERVAL: Duration = Duration::from_secs(1);
 
+pub struct WasScrolled(pub(crate) bool);
+
 #[derive(Default)]
 pub struct ScrollbarAutoHide(pub bool);
 
@@ -215,7 +217,7 @@ impl ScrollManager {
         workspace_id: Option<WorkspaceId>,
         window: &mut Window,
         cx: &mut Context<Editor>,
-    ) {
+    ) -> WasScrolled {
         let scroll_top = scroll_position.y.max(0.);
         let scroll_top = match EditorSettings::get_global(cx).scroll_beyond_last_line {
             ScrollBeyondLastLine::OnePage => scroll_top,
@@ -264,7 +266,7 @@ impl ScrollManager {
             workspace_id,
             window,
             cx,
-        );
+        )
     }
 
     fn set_anchor(
@@ -276,7 +278,7 @@ impl ScrollManager {
         workspace_id: Option<WorkspaceId>,
         window: &mut Window,
         cx: &mut Context<Editor>,
-    ) {
+    ) -> WasScrolled {
         let adjusted_anchor = if self.forbid_vertical_scroll {
             ScrollAnchor {
                 offset: gpui::Point::new(anchor.offset.x, self.anchor.offset.y),
@@ -286,10 +288,14 @@ impl ScrollManager {
             anchor
         };
 
+        self.autoscroll_request.take();
+        if self.anchor == adjusted_anchor {
+            return WasScrolled(false);
+        }
+
         self.anchor = adjusted_anchor;
         cx.emit(EditorEvent::ScrollPositionChanged { local, autoscroll });
         self.show_scrollbars(window, cx);
-        self.autoscroll_request.take();
         if let Some(workspace_id) = workspace_id {
             let item_id = cx.entity().entity_id().as_u64() as ItemId;
 
@@ -311,6 +317,8 @@ impl ScrollManager {
                 .detach()
         }
         cx.notify();
+
+        WasScrolled(true)
     }
 
     pub fn show_scrollbars(&mut self, window: &mut Window, cx: &mut Context<Editor>) {
@@ -521,13 +529,13 @@ impl Editor {
         scroll_position: gpui::Point<f32>,
         window: &mut Window,
         cx: &mut Context<Self>,
-    ) {
+    ) -> WasScrolled {
         let mut position = scroll_position;
         if self.scroll_manager.forbid_vertical_scroll {
             let current_position = self.scroll_position(cx);
             position.y = current_position.y;
         }
-        self.set_scroll_position_internal(position, true, false, window, cx);
+        self.set_scroll_position_internal(position, true, false, window, cx)
     }
 
     /// Scrolls so that `row` is at the top of the editor view.
@@ -559,7 +567,7 @@ impl Editor {
         autoscroll: bool,
         window: &mut Window,
         cx: &mut Context<Self>,
-    ) {
+    ) -> WasScrolled {
         let map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
         self.set_scroll_position_taking_display_map(
             scroll_position,
@@ -568,7 +576,7 @@ impl Editor {
             map,
             window,
             cx,
-        );
+        )
     }
 
     fn set_scroll_position_taking_display_map(
@@ -579,7 +587,7 @@ impl Editor {
         display_map: DisplaySnapshot,
         window: &mut Window,
         cx: &mut Context<Self>,
-    ) {
+    ) -> WasScrolled {
         hide_hover(self, cx);
         let workspace_id = self.workspace.as_ref().and_then(|workspace| workspace.1);
 
@@ -593,7 +601,7 @@ impl Editor {
             scroll_position
         };
 
-        self.scroll_manager.set_scroll_position(
+        let editor_was_scrolled = self.scroll_manager.set_scroll_position(
             adjusted_position,
             &display_map,
             local,
@@ -605,6 +613,7 @@ impl Editor {
 
         self.refresh_inlay_hints(InlayHintRefreshReason::NewLinesShown, cx);
         self.refresh_colors(false, None, window, cx);
+        editor_was_scrolled
     }
 
     pub fn scroll_position(&self, cx: &mut Context<Self>) -> gpui::Point<f32> {

crates/editor/src/scroll/autoscroll.rs 🔗

@@ -1,6 +1,6 @@
 use crate::{
     DisplayRow, Editor, EditorMode, LineWithInvisibles, RowExt, SelectionEffects,
-    display_map::ToDisplayPoint,
+    display_map::ToDisplayPoint, scroll::WasScrolled,
 };
 use gpui::{Bounds, Context, Pixels, Window, px};
 use language::Point;
@@ -99,19 +99,21 @@ impl AutoscrollStrategy {
     }
 }
 
+pub(crate) struct NeedsHorizontalAutoscroll(pub(crate) bool);
+
 impl Editor {
     pub fn autoscroll_request(&self) -> Option<Autoscroll> {
         self.scroll_manager.autoscroll_request()
     }
 
-    pub fn autoscroll_vertically(
+    pub(crate) fn autoscroll_vertically(
         &mut self,
         bounds: Bounds<Pixels>,
         line_height: Pixels,
         max_scroll_top: f32,
         window: &mut Window,
         cx: &mut Context<Editor>,
-    ) -> bool {
+    ) -> (NeedsHorizontalAutoscroll, WasScrolled) {
         let viewport_height = bounds.size.height;
         let visible_lines = viewport_height / line_height;
         let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
@@ -129,12 +131,14 @@ impl Editor {
             scroll_position.y = max_scroll_top;
         }
 
-        if original_y != scroll_position.y {
-            self.set_scroll_position(scroll_position, window, cx);
-        }
+        let editor_was_scrolled = if original_y != scroll_position.y {
+            self.set_scroll_position(scroll_position, window, cx)
+        } else {
+            WasScrolled(false)
+        };
 
         let Some((autoscroll, local)) = self.scroll_manager.autoscroll_request.take() else {
-            return false;
+            return (NeedsHorizontalAutoscroll(false), editor_was_scrolled);
         };
 
         let mut target_top;
@@ -212,7 +216,7 @@ impl Editor {
             target_bottom = target_top + 1.;
         }
 
-        match strategy {
+        let was_autoscrolled = match strategy {
             AutoscrollStrategy::Fit | AutoscrollStrategy::Newest => {
                 let margin = margin.min(self.scroll_manager.vertical_scroll_margin);
                 let target_top = (target_top - margin).max(0.0);
@@ -225,39 +229,42 @@ impl Editor {
 
                 if needs_scroll_up && !needs_scroll_down {
                     scroll_position.y = target_top;
-                    self.set_scroll_position_internal(scroll_position, local, true, window, cx);
-                }
-                if !needs_scroll_up && needs_scroll_down {
+                } else if !needs_scroll_up && needs_scroll_down {
                     scroll_position.y = target_bottom - visible_lines;
-                    self.set_scroll_position_internal(scroll_position, local, true, window, cx);
+                }
+
+                if needs_scroll_up ^ needs_scroll_down {
+                    self.set_scroll_position_internal(scroll_position, local, true, window, cx)
+                } else {
+                    WasScrolled(false)
                 }
             }
             AutoscrollStrategy::Center => {
                 scroll_position.y = (target_top - margin).max(0.0);
-                self.set_scroll_position_internal(scroll_position, local, true, window, cx);
+                self.set_scroll_position_internal(scroll_position, local, true, window, cx)
             }
             AutoscrollStrategy::Focused => {
                 let margin = margin.min(self.scroll_manager.vertical_scroll_margin);
                 scroll_position.y = (target_top - margin).max(0.0);
-                self.set_scroll_position_internal(scroll_position, local, true, window, cx);
+                self.set_scroll_position_internal(scroll_position, local, true, window, cx)
             }
             AutoscrollStrategy::Top => {
                 scroll_position.y = (target_top).max(0.0);
-                self.set_scroll_position_internal(scroll_position, local, true, window, cx);
+                self.set_scroll_position_internal(scroll_position, local, true, window, cx)
             }
             AutoscrollStrategy::Bottom => {
                 scroll_position.y = (target_bottom - visible_lines).max(0.0);
-                self.set_scroll_position_internal(scroll_position, local, true, window, cx);
+                self.set_scroll_position_internal(scroll_position, local, true, window, cx)
             }
             AutoscrollStrategy::TopRelative(lines) => {
                 scroll_position.y = target_top - lines as f32;
-                self.set_scroll_position_internal(scroll_position, local, true, window, cx);
+                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.set_scroll_position_internal(scroll_position, local, true, window, cx)
             }
-        }
+        };
 
         self.scroll_manager.last_autoscroll = Some((
             self.scroll_manager.anchor.offset,
@@ -266,7 +273,8 @@ impl Editor {
             strategy,
         ));
 
-        true
+        let was_scrolled = WasScrolled(editor_was_scrolled.0 || was_autoscrolled.0);
+        (NeedsHorizontalAutoscroll(true), was_scrolled)
     }
 
     pub(crate) fn autoscroll_horizontally(
@@ -278,7 +286,7 @@ impl Editor {
         layouts: &[LineWithInvisibles],
         window: &mut Window,
         cx: &mut Context<Self>,
-    ) -> bool {
+    ) -> Option<gpui::Point<f32>> {
         let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
         let selections = self.selections.all::<Point>(cx);
         let mut scroll_position = self.scroll_manager.scroll_position(&display_map);
@@ -319,22 +327,26 @@ impl Editor {
         target_right = target_right.min(scroll_width);
 
         if target_right - target_left > viewport_width {
-            return false;
+            return None;
         }
 
         let scroll_left = self.scroll_manager.anchor.offset.x * em_advance;
         let scroll_right = scroll_left + viewport_width;
 
-        if target_left < scroll_left {
+        let was_scrolled = if target_left < scroll_left {
             scroll_position.x = target_left / em_advance;
-            self.set_scroll_position_internal(scroll_position, true, true, window, cx);
-            true
+            self.set_scroll_position_internal(scroll_position, true, true, window, cx)
         } else if target_right > scroll_right {
             scroll_position.x = (target_right - viewport_width) / em_advance;
-            self.set_scroll_position_internal(scroll_position, true, true, window, cx);
-            true
+            self.set_scroll_position_internal(scroll_position, true, true, window, cx)
+        } else {
+            WasScrolled(false)
+        };
+
+        if was_scrolled.0 {
+            Some(scroll_position)
         } else {
-            false
+            None
         }
     }