Fix bugs in autoscroll with 'fit' strategy (#2893)

Max Brunsfeld created 

This fixes a bug where text moved up and down by one pixel in the buffer
search query editor, while typing.

Release  notes:
* Fixed a bug where editors didn't auto-scroll when typing if all
cursors could not fit within the viewport.

Change summary

crates/editor/src/editor_tests.rs      | 68 ++++++++++++++++++++++++++
crates/editor/src/element.rs           |  7 -
crates/editor/src/scroll/autoscroll.rs | 73 +++++++++++++++------------
3 files changed, 111 insertions(+), 37 deletions(-)

Detailed changes

crates/editor/src/editor_tests.rs πŸ”— 

@@ -1434,6 +1434,74 @@ async fn test_scroll_page_up_page_down(cx: &mut gpui::TestAppContext) {
     });
 }
 
+#[gpui::test]
+async fn test_autoscroll(cx: &mut gpui::TestAppContext) {
+    init_test(cx, |_| {});
+    let mut cx = EditorTestContext::new(cx).await;
+
+    let line_height = cx.update_editor(|editor, cx| {
+        editor.set_vertical_scroll_margin(2, cx);
+        editor.style(cx).text.line_height(cx.font_cache())
+    });
+
+    let window = cx.window;
+    window.simulate_resize(vec2f(1000., 6.0 * line_height), &mut cx);
+
+    cx.set_state(
+        &r#"Λ‡one
+            two
+            three
+            four
+            five
+            six
+            seven
+            eight
+            nine
+            ten
+        "#,
+    );
+    cx.update_editor(|editor, cx| {
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 0.0));
+    });
+
+    // Add a cursor below the visible area. Since both cursors cannot fit
+    // on screen, the editor autoscrolls to reveal the newest cursor, and
+    // allows the vertical scroll margin below that cursor.
+    cx.update_editor(|editor, cx| {
+        editor.change_selections(Some(Autoscroll::fit()), cx, |selections| {
+            selections.select_ranges([
+                Point::new(0, 0)..Point::new(0, 0),
+                Point::new(6, 0)..Point::new(6, 0),
+            ]);
+        })
+    });
+    cx.update_editor(|editor, cx| {
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 3.0));
+    });
+
+    // Move down. The editor cursor scrolls down to track the newest cursor.
+    cx.update_editor(|editor, cx| {
+        editor.move_down(&Default::default(), cx);
+    });
+    cx.update_editor(|editor, cx| {
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 4.0));
+    });
+
+    // Add a cursor above the visible area. Since both cursors fit on screen,
+    // the editor scrolls to show both.
+    cx.update_editor(|editor, cx| {
+        editor.change_selections(Some(Autoscroll::fit()), cx, |selections| {
+            selections.select_ranges([
+                Point::new(1, 0)..Point::new(1, 0),
+                Point::new(6, 0)..Point::new(6, 0),
+            ]);
+        })
+    });
+    cx.update_editor(|editor, cx| {
+        assert_eq!(editor.snapshot(cx).scroll_position(), vec2f(0., 1.0));
+    });
+}
+
 #[gpui::test]
 async fn test_move_page_up_page_down(cx: &mut gpui::TestAppContext) {
     init_test(cx, |_| {});

crates/editor/src/element.rs πŸ”— 

@@ -2177,14 +2177,11 @@ impl Element<Editor> for EditorElement {
                 scroll_height
                     .min(constraint.max_along(Axis::Vertical))
                     .max(constraint.min_along(Axis::Vertical))
+                    .max(line_height)
                     .min(line_height * max_lines as f32),
             )
         } else if let EditorMode::SingleLine = snapshot.mode {
-            size.set_y(
-                line_height
-                    .min(constraint.max_along(Axis::Vertical))
-                    .max(constraint.min_along(Axis::Vertical)),
-            )
+            size.set_y(line_height.max(constraint.min_along(Axis::Vertical)))
         } else if size.y().is_infinite() {
             size.set_y(scroll_height);
         }

crates/editor/src/scroll/autoscroll.rs πŸ”— 

@@ -65,47 +65,52 @@ impl Editor {
             self.set_scroll_position(scroll_position, cx);
         }
 
-        let (autoscroll, local) =
-            if let Some(autoscroll) = self.scroll_manager.autoscroll_request.take() {
-                autoscroll
-            } else {
-                return false;
-            };
-
-        let first_cursor_top;
-        let last_cursor_bottom;
+        let Some((autoscroll, local)) = self.scroll_manager.autoscroll_request.take() else {
+            return false;
+        };
+
+        let mut target_top;
+        let mut target_bottom;
         if let Some(highlighted_rows) = &self.highlighted_rows {
-            first_cursor_top = highlighted_rows.start as f32;
-            last_cursor_bottom = first_cursor_top + 1.;
-        } else if autoscroll == Autoscroll::newest() {
-            let newest_selection = self.selections.newest::<Point>(cx);
-            first_cursor_top = newest_selection.head().to_display_point(&display_map).row() as f32;
-            last_cursor_bottom = first_cursor_top + 1.;
+            target_top = highlighted_rows.start as f32;
+            target_bottom = target_top + 1.;
         } else {
             let selections = self.selections.all::<Point>(cx);
-            first_cursor_top = selections
+            target_top = selections
                 .first()
                 .unwrap()
                 .head()
                 .to_display_point(&display_map)
                 .row() as f32;
-            last_cursor_bottom = selections
+            target_bottom = selections
                 .last()
                 .unwrap()
                 .head()
                 .to_display_point(&display_map)
                 .row() as f32
                 + 1.0;
+
+            // If the selections can't all fit on screen, scroll to the newest.
+            if autoscroll == Autoscroll::newest()
+                || autoscroll == Autoscroll::fit() && target_bottom - target_top > visible_lines
+            {
+                let newest_selection_top = selections
+                    .iter()
+                    .max_by_key(|s| s.id)
+                    .unwrap()
+                    .head()
+                    .to_display_point(&display_map)
+                    .row() as f32;
+                target_top = newest_selection_top;
+                target_bottom = newest_selection_top + 1.;
+            }
         }
 
         let margin = if matches!(self.mode, EditorMode::AutoHeight { .. }) {
             0.
         } else {
-            ((visible_lines - (last_cursor_bottom - first_cursor_top)) / 2.0).floor()
+            ((visible_lines - (target_bottom - target_top)) / 2.0).floor()
         };
-        if margin < 0.0 {
-            return false;
-        }
 
         let strategy = match autoscroll {
             Autoscroll::Strategy(strategy) => strategy,
@@ -113,8 +118,8 @@ impl Editor {
                 let last_autoscroll = &self.scroll_manager.last_autoscroll;
                 if let Some(last_autoscroll) = last_autoscroll {
                     if self.scroll_manager.anchor.offset == last_autoscroll.0
-                        && first_cursor_top == last_autoscroll.1
-                        && last_cursor_bottom == last_autoscroll.2
+                        && target_top == last_autoscroll.1
+                        && target_bottom == last_autoscroll.2
                     {
                         last_autoscroll.3.next()
                     } else {
@@ -129,37 +134,41 @@ impl Editor {
         match strategy {
             AutoscrollStrategy::Fit | AutoscrollStrategy::Newest => {
                 let margin = margin.min(self.scroll_manager.vertical_scroll_margin);
-                let target_top = (first_cursor_top - margin).max(0.0);
-                let target_bottom = last_cursor_bottom + margin;
+                let target_top = (target_top - margin).max(0.0);
+                let target_bottom = target_bottom + margin;
                 let start_row = scroll_position.y();
                 let end_row = start_row + visible_lines;
 
-                if target_top < start_row {
+                let needs_scroll_up = target_top < start_row;
+                let needs_scroll_down = target_bottom >= end_row;
+
+                if needs_scroll_up && !needs_scroll_down {
                     scroll_position.set_y(target_top);
                     self.set_scroll_position_internal(scroll_position, local, true, cx);
-                } else if target_bottom >= end_row {
+                }
+                if !needs_scroll_up && needs_scroll_down {
                     scroll_position.set_y(target_bottom - visible_lines);
                     self.set_scroll_position_internal(scroll_position, local, true, cx);
                 }
             }
             AutoscrollStrategy::Center => {
-                scroll_position.set_y((first_cursor_top - margin).max(0.0));
+                scroll_position.set_y((target_top - margin).max(0.0));
                 self.set_scroll_position_internal(scroll_position, local, true, cx);
             }
             AutoscrollStrategy::Top => {
-                scroll_position.set_y((first_cursor_top).max(0.0));
+                scroll_position.set_y((target_top).max(0.0));
                 self.set_scroll_position_internal(scroll_position, local, true, cx);
             }
             AutoscrollStrategy::Bottom => {
-                scroll_position.set_y((last_cursor_bottom - visible_lines).max(0.0));
+                scroll_position.set_y((target_bottom - visible_lines).max(0.0));
                 self.set_scroll_position_internal(scroll_position, local, true, cx);
             }
         }
 
         self.scroll_manager.last_autoscroll = Some((
             self.scroll_manager.anchor.offset,
-            first_cursor_top,
-            last_cursor_bottom,
+            target_top,
+            target_bottom,
             strategy,
         ));