Fix fold tests with new representation

Mikayla Maki and Max created

Switch UI code from using display rows to using buffer rows
Make folds only show up on lines with line layouts

co-authored-by: Max <max@zed.dev>

Change summary

crates/editor/src/display_map.rs          |   9 -
crates/editor/src/display_map/fold_map.rs |   4 
crates/editor/src/editor.rs               |  93 +++++++--------------
crates/editor/src/element.rs              | 104 +++++++++++++-----------
4 files changed, 93 insertions(+), 117 deletions(-)

Detailed changes

crates/editor/src/display_map.rs 🔗

@@ -621,10 +621,10 @@ impl DisplaySnapshot {
     }
 
     pub fn fold_for_line(self: &Self, buffer_row: u32) -> Option<FoldStatus> {
-        if self.is_foldable(buffer_row) {
-            Some(FoldStatus::Foldable)
-        } else if self.is_line_folded(buffer_row) {
+        if self.is_line_folded(buffer_row) {
             Some(FoldStatus::Folded)
+        } else if self.is_foldable(buffer_row) {
+            Some(FoldStatus::Foldable)
         } else {
             None
         }
@@ -655,14 +655,13 @@ impl DisplaySnapshot {
 
     pub fn foldable_range(self: &Self, buffer_row: u32) -> Option<Range<Point>> {
         let start = Point::new(buffer_row, self.buffer_snapshot.line_len(buffer_row));
-
         if self.is_foldable(start.row) && !self.is_line_folded(start.row) {
             let (start_indent, _) = self.line_indent_for_buffer_row(buffer_row);
             let max_point = self.buffer_snapshot.max_point();
             let mut end = None;
 
             for row in (buffer_row + 1)..=max_point.row {
-                let (indent, is_blank) = self.line_indent(row);
+                let (indent, is_blank) = self.line_indent_for_buffer_row(row);
                 if !is_blank && indent <= start_indent {
                     let prev_row = row - 1;
                     end = Some(Point::new(

crates/editor/src/display_map/fold_map.rs 🔗

@@ -1599,8 +1599,8 @@ mod tests {
                 .iter()
                 .map(|range| range.start.to_point(&buffer_snapshot).row)
                 .collect::<HashSet<_>>();
-            for row in 0..=buffer_snapshot.max_buffer_row() {
-                assert_eq!(snapshot.is_line_folded(row), fold_start_rows.contains(&row));
+            for row in fold_start_rows {
+                assert!(snapshot.is_line_folded(row));
             }
 
             for _ in 0..5 {

crates/editor/src/editor.rs 🔗

@@ -40,7 +40,6 @@ use gpui::{
     keymap_matcher::KeymapContext,
     platform::CursorStyle,
     serde_json::json,
-    text_layout::Line,
     AnyViewHandle, AppContext, AsyncAppContext, ClipboardItem, Element, ElementBox, Entity,
     ModelHandle, MouseButton, MutableAppContext, RenderContext, Subscription, Task, View,
     ViewContext, ViewHandle, WeakViewHandle,
@@ -164,12 +163,12 @@ pub struct ToggleComments {
 
 #[derive(Clone, Default, Deserialize, PartialEq)]
 pub struct FoldAt {
-    pub display_row: u32,
+    pub buffer_row: u32,
 }
 
 #[derive(Clone, Default, Deserialize, PartialEq)]
 pub struct UnfoldAt {
-    pub display_row: u32,
+    pub buffer_row: u32,
 }
 
 #[derive(Clone, Default, Deserialize, PartialEq)]
@@ -2706,34 +2705,26 @@ impl Editor {
 
     pub fn render_fold_indicators(
         &self,
-        fold_data: Option<Vec<(u32, FoldStatus)>>,
-        active_rows: &BTreeMap<u32, bool>,
-        line_layouts: &Vec<Option<Line>>,
+        fold_data: Vec<Option<(FoldStatus, u32, bool)>>,
         style: &EditorStyle,
         gutter_hovered: bool,
         line_height: f32,
         gutter_margin: f32,
         cx: &mut RenderContext<Self>,
-    ) -> Option<Vec<(u32, ElementBox)>> {
+    ) -> Vec<Option<ElementBox>> {
         enum FoldIndicators {}
 
         let style = style.folds.clone();
 
-        fold_data.map(|fold_data| {
-            fold_data
-                .iter()
-                .copied()
-                .filter_map(|(fold_location, fold_status)| {
-                    let has_line_number = line_layouts[fold_location as usize].is_some();
-                    (has_line_number
-                        && (gutter_hovered
-                            || fold_status == FoldStatus::Folded
-                            || !*active_rows.get(&fold_location).unwrap_or(&true)))
-                    .then(|| {
-                        (
-                            fold_location,
+        fold_data
+            .iter()
+            .enumerate()
+            .map(|(ix, fold_data)| {
+                fold_data
+                    .map(|(fold_status, buffer_row, active)| {
+                        (active || gutter_hovered || fold_status == FoldStatus::Folded).then(|| {
                             MouseEventHandler::<FoldIndicators>::new(
-                                fold_location as usize,
+                                ix as usize,
                                 cx,
                                 |mouse_state, _| -> ElementBox {
                                     Svg::new(match fold_status {
@@ -2764,21 +2755,17 @@ impl Editor {
                             .on_click(MouseButton::Left, {
                                 move |_, cx| {
                                     cx.dispatch_any_action(match fold_status {
-                                        FoldStatus::Folded => Box::new(UnfoldAt {
-                                            display_row: fold_location,
-                                        }),
-                                        FoldStatus::Foldable => Box::new(FoldAt {
-                                            display_row: fold_location,
-                                        }),
+                                        FoldStatus::Folded => Box::new(UnfoldAt { buffer_row }),
+                                        FoldStatus::Foldable => Box::new(FoldAt { buffer_row }),
                                     });
                                 }
                             })
-                            .boxed(),
-                        )
+                            .boxed()
+                        })
                     })
-                })
-                .collect()
-        })
+                    .flatten()
+            })
+            .collect()
     }
 
     pub fn context_menu_visible(&self) -> bool {
@@ -5762,24 +5749,14 @@ impl Editor {
         let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
 
         let selections = self.selections.all::<Point>(cx);
-        dbg!(&selections);
         for selection in selections {
             let range = selection.range().sorted();
             let buffer_start_row = range.start.row;
 
             for row in (0..=range.end.row).rev() {
-                dbg!(row);
-                let fold_range = dbg!(display_map.foldable_range(row));
+                let fold_range = display_map.foldable_range(row);
 
                 if let Some(fold_range) = fold_range {
-                    let display_point = fold_range.end.to_display_point(&display_map);
-                    let line = display_map
-                        .chars_at(DisplayPoint::new(display_point.row(), 0))
-                        .map(|(ccharr, _)| ccharr)
-                        .take_while(|charr| charr != &'\n')
-                        .collect::<String>();
-                    dbg!(line);
-
                     if fold_range.end.row >= buffer_start_row {
                         fold_ranges.push(fold_range);
                         if row <= range.start.row {
@@ -5794,11 +5771,11 @@ impl Editor {
     }
 
     pub fn fold_at(&mut self, fold_at: &FoldAt, cx: &mut ViewContext<Self>) {
-        let display_row = fold_at.display_row;
+        let buffer_row = fold_at.buffer_row;
 
         let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
 
-        if let Some(fold_range) = display_map.foldable_range(display_row) {
+        if let Some(fold_range) = display_map.foldable_range(buffer_row) {
             let autoscroll = self
                 .selections
                 .all::<Point>(cx)
@@ -5831,25 +5808,19 @@ impl Editor {
     pub fn unfold_at(&mut self, unfold_at: &UnfoldAt, cx: &mut ViewContext<Self>) {
         let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
 
-        let intersection_range = DisplayPoint::new(unfold_at.display_row, 0)
-            ..DisplayPoint::new(
-                unfold_at.display_row,
-                display_map.line_len(unfold_at.display_row),
+        let intersection_range = Point::new(unfold_at.buffer_row, 0)
+            ..Point::new(
+                unfold_at.buffer_row,
+                display_map.buffer_snapshot.line_len(unfold_at.buffer_row),
             );
 
-        let autoscroll =
-            self.selections.all::<Point>(cx).iter().any(|selection| {
-                intersection_range.overlaps(&selection.display_range(&display_map))
-            });
-
-        let display_point = DisplayPoint::new(unfold_at.display_row, 0).to_point(&display_map);
-
-        let mut point_range = display_point..display_point;
-
-        point_range.start.column = 0;
-        point_range.end.column = display_map.buffer_snapshot.line_len(point_range.end.row);
+        let autoscroll = self
+            .selections
+            .all::<Point>(cx)
+            .iter()
+            .any(|selection| selection.range().overlaps(&intersection_range));
 
-        self.unfold_ranges(std::iter::once(point_range), true, autoscroll, cx)
+        self.unfold_ranges(std::iter::once(intersection_range), true, autoscroll, cx)
     }
 
     pub fn fold_selected_ranges(&mut self, _: &FoldSelectedRanges, cx: &mut ViewContext<Self>) {

crates/editor/src/element.rs 🔗

@@ -574,6 +574,20 @@ impl EditorElement {
             }
         }
 
+        for (ix, fold_indicator) in layout.fold_indicators.iter_mut().enumerate() {
+            if let Some(indicator) = fold_indicator.as_mut() {
+                let mut x = bounds.width() - layout.gutter_padding;
+                let mut y = ix as f32 * line_height - scroll_top;
+
+                x += ((layout.gutter_padding + layout.gutter_margin) - indicator.size().x()) / 2.;
+                y += (line_height - indicator.size().y()) / 2.;
+
+                let indicator_origin = bounds.origin() + vec2f(x, y);
+
+                indicator.paint(indicator_origin, visible_bounds, cx);
+            }
+        }
+
         if let Some((row, indicator)) = layout.code_actions_indicator.as_mut() {
             let mut x = 0.;
             let mut y = *row as f32 * line_height - scroll_top;
@@ -581,19 +595,6 @@ impl EditorElement {
             y += (line_height - indicator.size().y()) / 2.;
             indicator.paint(bounds.origin() + vec2f(x, y), visible_bounds, cx);
         }
-
-        layout.fold_indicators.as_mut().map(|fold_indicators| {
-            for (line, fold_indicator) in fold_indicators.iter_mut() {
-                let mut x = bounds.width() - layout.gutter_padding;
-                let mut y = *line as f32 * line_height - scroll_top;
-
-                x += ((layout.gutter_padding + layout.gutter_margin) - fold_indicator.size().x())
-                    / 2.;
-                y += (line_height - fold_indicator.size().y()) / 2.;
-
-                fold_indicator.paint(bounds.origin() + vec2f(x, y), visible_bounds, cx);
-            }
-        });
     }
 
     fn paint_diff_hunks(bounds: RectF, layout: &mut LayoutState, cx: &mut PaintContext) {
@@ -739,10 +740,15 @@ impl EditorElement {
                 });
 
                 let display_row = range.start.row();
+
+                let buffer_row = DisplayPoint::new(display_row, 0)
+                    .to_point(&layout.position_map.snapshot.display_snapshot)
+                    .row;
+
                 cx.scene.push_mouse_region(
                     MouseRegion::new::<FoldMarkers>(self.view.id(), *id as usize, bound)
                         .on_click(MouseButton::Left, move |_, cx| {
-                            cx.dispatch_action(UnfoldAt { display_row })
+                            cx.dispatch_action(UnfoldAt { buffer_row })
                         })
                         .with_notify_on_hover(true)
                         .with_notify_on_click(true),
@@ -1181,24 +1187,6 @@ impl EditorElement {
             .width()
     }
 
-    fn get_fold_indicators(
-        &self,
-        is_singleton: bool,
-        display_rows: Range<u32>,
-        snapshot: &EditorSnapshot,
-    ) -> Option<Vec<(u32, FoldStatus)>> {
-        is_singleton.then(|| {
-            display_rows
-                .into_iter()
-                .filter_map(|display_row| {
-                    snapshot
-                        .fold_for_line(display_row)
-                        .map(|fold_status| (display_row, fold_status))
-                })
-                .collect()
-        })
-    }
-
     //Folds contained in a hunk are ignored apart from shrinking visual size
     //If a fold contains any hunks then that fold line is marked as modified
     fn layout_git_gutters(
@@ -1226,12 +1214,17 @@ impl EditorElement {
         &self,
         rows: Range<u32>,
         active_rows: &BTreeMap<u32, bool>,
+        is_singleton: bool,
         snapshot: &EditorSnapshot,
         cx: &LayoutContext,
-    ) -> Vec<Option<text_layout::Line>> {
+    ) -> (
+        Vec<Option<text_layout::Line>>,
+        Vec<Option<(FoldStatus, BufferRow, bool)>>,
+    ) {
         let style = &self.style;
         let include_line_numbers = snapshot.mode == EditorMode::Full;
         let mut line_number_layouts = Vec::with_capacity(rows.len());
+        let mut fold_statuses = Vec::with_capacity(rows.len());
         let mut line_number = String::new();
         for (ix, row) in snapshot
             .buffer_rows(rows.start)
@@ -1239,10 +1232,10 @@ impl EditorElement {
             .enumerate()
         {
             let display_row = rows.start + ix as u32;
-            let color = if active_rows.contains_key(&display_row) {
-                style.line_number_active
+            let (active, color) = if active_rows.contains_key(&display_row) {
+                (true, style.line_number_active)
             } else {
-                style.line_number
+                (false, style.line_number)
             };
             if let Some(buffer_row) = row {
                 if include_line_numbers {
@@ -1260,13 +1253,23 @@ impl EditorElement {
                             },
                         )],
                     )));
+                    fold_statuses.push(
+                        is_singleton
+                            .then(|| {
+                                snapshot
+                                    .fold_for_line(buffer_row)
+                                    .map(|fold_status| (fold_status, buffer_row, active))
+                            })
+                            .flatten(),
+                    )
                 }
             } else {
+                fold_statuses.push(None);
                 line_number_layouts.push(None);
             }
         }
 
-        line_number_layouts
+        (line_number_layouts, fold_statuses)
     }
 
     fn layout_lines(
@@ -1797,13 +1800,16 @@ impl Element for EditorElement {
             })
             .collect();
 
-        let line_number_layouts =
-            self.layout_line_numbers(start_row..end_row, &active_rows, &snapshot, cx);
+        let (line_number_layouts, fold_statuses) = self.layout_line_numbers(
+            start_row..end_row,
+            &active_rows,
+            is_singleton,
+            &snapshot,
+            cx,
+        );
 
         let display_hunks = self.layout_git_gutters(start_row..end_row, &snapshot);
 
-        let folds = self.get_fold_indicators(is_singleton, start_row..end_row, &snapshot);
-
         let scrollbar_row_range = scroll_position.y()..(scroll_position.y() + height_in_lines);
 
         let mut max_visible_line_width = 0.0;
@@ -1896,9 +1902,7 @@ impl Element for EditorElement {
             mode = view.mode;
 
             view.render_fold_indicators(
-                folds,
-                &active_rows,
-                &line_number_layouts,
+                fold_statuses,
                 &style,
                 view.gutter_hovered,
                 line_height,
@@ -1930,8 +1934,8 @@ impl Element for EditorElement {
             );
         }
 
-        fold_indicators.as_mut().map(|fold_indicators| {
-            for (_, indicator) in fold_indicators.iter_mut() {
+        for fold_indicator in fold_indicators.iter_mut() {
+            if let Some(indicator) = fold_indicator.as_mut() {
                 indicator.layout(
                     SizeConstraint::strict_along(
                         Axis::Vertical,
@@ -1940,7 +1944,7 @@ impl Element for EditorElement {
                     cx,
                 );
             }
-        });
+        }
 
         if let Some((_, hover_popovers)) = hover.as_mut() {
             for hover_popover in hover_popovers.iter_mut() {
@@ -2124,7 +2128,7 @@ pub struct LayoutState {
     context_menu: Option<(DisplayPoint, ElementBox)>,
     code_actions_indicator: Option<(u32, ElementBox)>,
     hover_popovers: Option<(DisplayPoint, Vec<ElementBox>)>,
-    fold_indicators: Option<Vec<(u32, ElementBox)>>,
+    fold_indicators: Vec<Option<ElementBox>>,
 }
 
 pub struct PositionMap {
@@ -2525,7 +2529,9 @@ mod tests {
             let snapshot = editor.snapshot(cx);
             let mut presenter = cx.build_presenter(window_id, 30., Default::default());
             let layout_cx = presenter.build_layout_context(Vector2F::zero(), false, cx);
-            element.layout_line_numbers(0..6, &Default::default(), &snapshot, &layout_cx)
+            element
+                .layout_line_numbers(0..6, &Default::default(), false, &snapshot, &layout_cx)
+                .0
         });
         assert_eq!(layouts.len(), 6);
     }