More git hunk highlighting fixes (#18459)

Max Brunsfeld created

Follow-up to https://github.com/zed-industries/zed/pull/18454

Release Notes:

- N/A

Change summary

crates/assistant/src/inline_assistant.rs      |  8 +-
crates/editor/src/editor.rs                   | 63 ++++++++++----------
crates/editor/src/editor_tests.rs             | 11 ++-
crates/editor/src/hunk_diff.rs                | 29 +--------
crates/editor/src/test/editor_test_context.rs | 32 +++-------
crates/go_to_line/src/go_to_line.rs           | 22 ++++---
crates/outline/src/outline.rs                 |  4 
7 files changed, 69 insertions(+), 100 deletions(-)

Detailed changes

crates/assistant/src/inline_assistant.rs 🔗

@@ -1208,7 +1208,7 @@ impl InlineAssistant {
                     editor.set_read_only(true);
                     editor.set_show_inline_completions(Some(false), cx);
                     editor.highlight_rows::<DeletedLines>(
-                        Anchor::min()..=Anchor::max(),
+                        Anchor::min()..Anchor::max(),
                         cx.theme().status().deleted_background,
                         false,
                         cx,
@@ -2557,7 +2557,7 @@ enum CodegenStatus {
 #[derive(Default)]
 struct Diff {
     deleted_row_ranges: Vec<(Anchor, RangeInclusive<u32>)>,
-    inserted_row_ranges: Vec<RangeInclusive<Anchor>>,
+    inserted_row_ranges: Vec<Range<Anchor>>,
 }
 
 impl Diff {
@@ -3103,7 +3103,7 @@ impl CodegenAlternative {
                         new_end_row,
                         new_snapshot.line_len(MultiBufferRow(new_end_row)),
                     ));
-                    self.diff.inserted_row_ranges.push(start..=end);
+                    self.diff.inserted_row_ranges.push(start..end);
                     new_row += lines;
                 }
             }
@@ -3181,7 +3181,7 @@ impl CodegenAlternative {
                                     new_end_row,
                                     new_snapshot.line_len(MultiBufferRow(new_end_row)),
                                 ));
-                                inserted_row_ranges.push(start..=end);
+                                inserted_row_ranges.push(start..end);
                                 new_row += line_count;
                             }
                         }

crates/editor/src/editor.rs 🔗

@@ -821,7 +821,7 @@ impl SelectionHistory {
 
 struct RowHighlight {
     index: usize,
-    range: RangeInclusive<Anchor>,
+    range: Range<Anchor>,
     color: Hsla,
     should_autoscroll: bool,
 }
@@ -11502,9 +11502,11 @@ impl Editor {
 
     /// Adds a row highlight for the given range. If a row has multiple highlights, the
     /// last highlight added will be used.
+    ///
+    /// If the range ends at the beginning of a line, then that line will not be highlighted.
     pub fn highlight_rows<T: 'static>(
         &mut self,
-        range: RangeInclusive<Anchor>,
+        range: Range<Anchor>,
         color: Hsla,
         should_autoscroll: bool,
         cx: &mut ViewContext<Self>,
@@ -11513,8 +11515,8 @@ impl Editor {
         let row_highlights = self.highlighted_rows.entry(TypeId::of::<T>()).or_default();
         let ix = row_highlights.binary_search_by(|highlight| {
             Ordering::Equal
-                .then_with(|| highlight.range.start().cmp(&range.start(), &snapshot))
-                .then_with(|| highlight.range.end().cmp(&range.end(), &snapshot))
+                .then_with(|| highlight.range.start.cmp(&range.start, &snapshot))
+                .then_with(|| highlight.range.end.cmp(&range.end, &snapshot))
         });
 
         if let Err(mut ix) = ix {
@@ -11527,18 +11529,13 @@ impl Editor {
                 let prev_highlight = &mut row_highlights[ix - 1];
                 if prev_highlight
                     .range
-                    .end()
-                    .cmp(&range.start(), &snapshot)
+                    .end
+                    .cmp(&range.start, &snapshot)
                     .is_ge()
                 {
                     ix -= 1;
-                    if prev_highlight
-                        .range
-                        .end()
-                        .cmp(&range.end(), &snapshot)
-                        .is_lt()
-                    {
-                        prev_highlight.range = *prev_highlight.range.start()..=*range.end();
+                    if prev_highlight.range.end.cmp(&range.end, &snapshot).is_lt() {
+                        prev_highlight.range.end = range.end;
                     }
                     merged = true;
                     prev_highlight.index = index;
@@ -11564,18 +11561,17 @@ impl Editor {
                 let highlight = &row_highlights[ix];
                 if next_highlight
                     .range
-                    .start()
-                    .cmp(&highlight.range.end(), &snapshot)
+                    .start
+                    .cmp(&highlight.range.end, &snapshot)
                     .is_le()
                 {
                     if next_highlight
                         .range
-                        .end()
-                        .cmp(&highlight.range.end(), &snapshot)
+                        .end
+                        .cmp(&highlight.range.end, &snapshot)
                         .is_gt()
                     {
-                        row_highlights[ix].range =
-                            *highlight.range.start()..=*next_highlight.range.end();
+                        row_highlights[ix].range.end = next_highlight.range.end;
                     }
                     row_highlights.remove(ix + 1);
                 } else {
@@ -11597,15 +11593,12 @@ impl Editor {
         let mut ranges_to_remove = ranges_to_remove.iter().peekable();
         row_highlights.retain(|highlight| {
             while let Some(range_to_remove) = ranges_to_remove.peek() {
-                match range_to_remove.end.cmp(&highlight.range.start(), &snapshot) {
-                    Ordering::Less => {
+                match range_to_remove.end.cmp(&highlight.range.start, &snapshot) {
+                    Ordering::Less | Ordering::Equal => {
                         ranges_to_remove.next();
                     }
-                    Ordering::Equal => {
-                        return false;
-                    }
                     Ordering::Greater => {
-                        match range_to_remove.start.cmp(&highlight.range.end(), &snapshot) {
+                        match range_to_remove.start.cmp(&highlight.range.end, &snapshot) {
                             Ordering::Less | Ordering::Equal => {
                                 return false;
                             }
@@ -11625,9 +11618,7 @@ impl Editor {
     }
 
     /// For a highlight given context type, gets all anchor ranges that will be used for row highlighting.
-    pub fn highlighted_rows<T: 'static>(
-        &self,
-    ) -> impl '_ + Iterator<Item = (RangeInclusive<Anchor>, Hsla)> {
+    pub fn highlighted_rows<T: 'static>(&self) -> impl '_ + Iterator<Item = (Range<Anchor>, Hsla)> {
         self.highlighted_rows
             .get(&TypeId::of::<T>())
             .map_or(&[] as &[_], |vec| vec.as_slice())
@@ -11650,9 +11641,17 @@ impl Editor {
             .fold(
                 BTreeMap::<DisplayRow, Hsla>::new(),
                 |mut unique_rows, highlight| {
-                    let start_row = highlight.range.start().to_display_point(&snapshot).row();
-                    let end_row = highlight.range.end().to_display_point(&snapshot).row();
-                    for row in start_row.0..=end_row.0 {
+                    let start = highlight.range.start.to_display_point(&snapshot);
+                    let end = highlight.range.end.to_display_point(&snapshot);
+                    let start_row = start.row().0;
+                    let end_row = if highlight.range.end.text_anchor != text::Anchor::MAX
+                        && end.column() == 0
+                    {
+                        end.row().0.saturating_sub(1)
+                    } else {
+                        end.row().0
+                    };
+                    for row in start_row..=end_row {
                         let used_index =
                             used_highlight_orders.entry(row).or_insert(highlight.index);
                         if highlight.index >= *used_index {
@@ -11674,7 +11673,7 @@ impl Editor {
             .flat_map(|highlighted_rows| highlighted_rows.iter())
             .filter_map(|highlight| {
                 if highlight.should_autoscroll {
-                    Some(highlight.range.start().to_display_point(snapshot).row())
+                    Some(highlight.range.start.to_display_point(snapshot).row())
                 } else {
                     None
                 }

crates/editor/src/editor_tests.rs 🔗

@@ -11832,7 +11832,6 @@ async fn test_edits_around_expanded_insertion_hunks(
     );
 
     cx.update_editor(|editor, cx| {
-        editor.move_up(&MoveUp, cx);
         editor.delete_line(&DeleteLine, cx);
     });
     executor.run_until_parked();
@@ -11846,7 +11845,7 @@ async fn test_edits_around_expanded_insertion_hunks(
       + const B: u32 = 42;
       + const C: u32 = 42;
       + const D: u32 = 42;
-      +
+      + const E: u32 = 42;
 
         fn main() {
             println!("hello");
@@ -11872,8 +11871,8 @@ async fn test_edits_around_expanded_insertion_hunks(
         use some::mod2;
 
         const A: u32 = 42;
+        const B: u32 = 42;
         ˇ
-
         fn main() {
             println!("hello");
 
@@ -11889,8 +11888,8 @@ async fn test_edits_around_expanded_insertion_hunks(
         use some::mod2;
 
         const A: u32 = 42;
+      + const B: u32 = 42;
 
-      +
         fn main() {
             println!("hello");
 
@@ -11907,7 +11906,9 @@ async fn test_edits_around_expanded_insertion_hunks(
     executor.run_until_parked();
     cx.assert_diff_hunks(
         r#"
-
+        use some::mod1;
+      - use some::mod2;
+      -
       - const A: u32 = 42;
 
         fn main() {

crates/editor/src/hunk_diff.rs 🔗

@@ -6,10 +6,7 @@ use multi_buffer::{
     Anchor, AnchorRangeExt, ExcerptRange, MultiBuffer, MultiBufferDiffHunk, MultiBufferRow,
     MultiBufferSnapshot, ToPoint,
 };
-use std::{
-    ops::{Range, RangeInclusive},
-    sync::Arc,
-};
+use std::{ops::Range, sync::Arc};
 use ui::{
     prelude::*, ActiveTheme, ContextMenu, IconButtonShape, InteractiveElement, IntoElement,
     ParentElement, PopoverMenu, Styled, Tooltip, ViewContext, VisualContext,
@@ -19,7 +16,7 @@ use util::RangeExt;
 use crate::{
     editor_settings::CurrentLineHighlight, hunk_status, hunks_for_selections, BlockDisposition,
     BlockProperties, BlockStyle, CustomBlockId, DiffRowHighlight, DisplayRow, DisplaySnapshot,
-    Editor, EditorElement, EditorSnapshot, ExpandAllHunkDiffs, GoToHunk, GoToPrevHunk, RevertFile,
+    Editor, EditorElement, ExpandAllHunkDiffs, GoToHunk, GoToPrevHunk, RevertFile,
     RevertSelectedHunks, ToDisplayPoint, ToggleHunkDiff,
 };
 
@@ -298,7 +295,7 @@ impl Editor {
             }
             DiffHunkStatus::Added => {
                 self.highlight_rows::<DiffRowHighlight>(
-                    to_inclusive_row_range(hunk_start..hunk_end, &snapshot),
+                    hunk_start..hunk_end,
                     added_hunk_color(cx),
                     false,
                     cx,
@@ -307,7 +304,7 @@ impl Editor {
             }
             DiffHunkStatus::Modified => {
                 self.highlight_rows::<DiffRowHighlight>(
-                    to_inclusive_row_range(hunk_start..hunk_end, &snapshot),
+                    hunk_start..hunk_end,
                     added_hunk_color(cx),
                     false,
                     cx,
@@ -960,7 +957,7 @@ fn editor_with_deleted_text(
         editor.set_read_only(true);
         editor.set_show_inline_completions(Some(false), cx);
         editor.highlight_rows::<DiffRowHighlight>(
-            Anchor::min()..=Anchor::max(),
+            Anchor::min()..Anchor::max(),
             deleted_color,
             false,
             cx,
@@ -1039,22 +1036,6 @@ fn buffer_diff_hunk(
     None
 }
 
-fn to_inclusive_row_range(
-    row_range: Range<Anchor>,
-    snapshot: &EditorSnapshot,
-) -> RangeInclusive<Anchor> {
-    let mut end = row_range.end.to_point(&snapshot.buffer_snapshot);
-    if end.column == 0 && end.row > 0 {
-        end = Point::new(
-            end.row - 1,
-            snapshot
-                .buffer_snapshot
-                .line_len(MultiBufferRow(end.row - 1)),
-        );
-    }
-    row_range.start..=snapshot.buffer_snapshot.anchor_after(end)
-}
-
 impl DisplayDiffHunk {
     pub fn start_display_row(&self) -> DisplayRow {
         match self {

crates/editor/src/test/editor_test_context.rs 🔗

@@ -9,7 +9,6 @@ use gpui::{
     AnyWindowHandle, AppContext, Keystroke, ModelContext, Pixels, Point, View, ViewContext,
     VisualTestContext, WindowHandle,
 };
-use indoc::indoc;
 use itertools::Itertools;
 use language::{Buffer, BufferSnapshot, LanguageRegistry};
 use multi_buffer::{ExcerptRange, ToPoint};
@@ -337,8 +336,9 @@ impl EditorTestContext {
             let insertions = editor
                 .highlighted_rows::<DiffRowHighlight>()
                 .map(|(range, _)| {
-                    range.start().to_point(&snapshot.buffer_snapshot).row
-                        ..range.end().to_point(&snapshot.buffer_snapshot).row + 1
+                    let start = range.start.to_point(&snapshot.buffer_snapshot);
+                    let end = range.end.to_point(&snapshot.buffer_snapshot);
+                    start.row..end.row
                 })
                 .collect::<Vec<_>>();
             let deletions = editor
@@ -384,13 +384,8 @@ impl EditorTestContext {
     /// See the `util::test::marked_text_ranges` function for more information.
     #[track_caller]
     pub fn assert_editor_state(&mut self, marked_text: &str) {
-        let (unmarked_text, expected_selections) = marked_text_ranges(marked_text, true);
-        let buffer_text = self.buffer_text();
-
-        if buffer_text != unmarked_text {
-            panic!("Unmarked text doesn't match buffer text\nBuffer text: {buffer_text:?}\nUnmarked text: {unmarked_text:?}\nRaw buffer text\n{buffer_text}\nRaw unmarked text\n{unmarked_text}");
-        }
-
+        let (expected_text, expected_selections) = marked_text_ranges(marked_text, true);
+        pretty_assertions::assert_eq!(self.buffer_text(), expected_text, "unexpected buffer text");
         self.assert_selections(expected_selections, marked_text.to_string())
     }
 
@@ -463,20 +458,11 @@ impl EditorTestContext {
         let actual_marked_text =
             generate_marked_text(&self.buffer_text(), &actual_selections, true);
         if expected_selections != actual_selections {
-            panic!(
-                indoc! {"
-
-                {}Editor has unexpected selections.
-
-                Expected selections:
-                {}
-
-                Actual selections:
-                {}
-            "},
-                self.assertion_context(),
-                expected_marked_text,
+            pretty_assertions::assert_eq!(
                 actual_marked_text,
+                expected_marked_text,
+                "{}Editor has unexpected selections",
+                self.assertion_context(),
             );
         }
     }

crates/go_to_line/src/go_to_line.rs 🔗

@@ -116,11 +116,13 @@ impl GoToLine {
         if let Some(point) = self.point_from_query(cx) {
             self.active_editor.update(cx, |active_editor, cx| {
                 let snapshot = active_editor.snapshot(cx).display_snapshot;
-                let point = snapshot.buffer_snapshot.clip_point(point, Bias::Left);
-                let anchor = snapshot.buffer_snapshot.anchor_before(point);
+                let start = snapshot.buffer_snapshot.clip_point(point, Bias::Left);
+                let end = start + Point::new(1, 0);
+                let start = snapshot.buffer_snapshot.anchor_before(start);
+                let end = snapshot.buffer_snapshot.anchor_after(end);
                 active_editor.clear_row_highlights::<GoToLineRowHighlights>();
                 active_editor.highlight_rows::<GoToLineRowHighlights>(
-                    anchor..=anchor,
+                    start..end,
                     cx.theme().colors().editor_highlighted_line_background,
                     true,
                     cx,
@@ -244,13 +246,13 @@ mod tests {
                         field_1: i32,  // display line 3
                         field_2: i32,  // display line 4
                     }                  // display line 5
-                                       // display line 7
-                    struct Another {   // display line 8
-                        field_1: i32,  // display line 9
-                        field_2: i32,  // display line 10
-                        field_3: i32,  // display line 11
-                        field_4: i32,  // display line 12
-                    }                  // display line 13
+                                       // display line 6
+                    struct Another {   // display line 7
+                        field_1: i32,  // display line 8
+                        field_2: i32,  // display line 9
+                        field_3: i32,  // display line 10
+                        field_4: i32,  // display line 11
+                    }                  // display line 12
                 "}
             }),
         )

crates/outline/src/outline.rs 🔗

@@ -143,7 +143,7 @@ impl OutlineViewDelegate {
             self.active_editor.update(cx, |active_editor, cx| {
                 active_editor.clear_row_highlights::<OutlineRowHighlights>();
                 active_editor.highlight_rows::<OutlineRowHighlights>(
-                    outline_item.range.start..=outline_item.range.end,
+                    outline_item.range.start..outline_item.range.end,
                     cx.theme().colors().editor_highlighted_line_background,
                     true,
                     cx,
@@ -245,7 +245,7 @@ impl PickerDelegate for OutlineViewDelegate {
                 .next();
             if let Some((rows, _)) = highlight {
                 active_editor.change_selections(Some(Autoscroll::center()), cx, |s| {
-                    s.select_ranges([*rows.start()..*rows.start()])
+                    s.select_ranges([rows.start..rows.start])
                 });
                 active_editor.clear_row_highlights::<OutlineRowHighlights>();
                 active_editor.focus(cx);