From 0daa070448d3a5078cd274ddbd5ec18e425c63d3 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Fri, 27 Sep 2024 13:48:37 -0700 Subject: [PATCH] More git hunk highlighting fixes (#18459) Follow-up to https://github.com/zed-industries/zed/pull/18454 Release Notes: - N/A --- 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(-) diff --git a/crates/assistant/src/inline_assistant.rs b/crates/assistant/src/inline_assistant.rs index e2f2fa190d..fac70f233c 100644 --- a/crates/assistant/src/inline_assistant.rs +++ b/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::( - 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)>, - inserted_row_ranges: Vec>, + inserted_row_ranges: Vec>, } 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; } } diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 48785dbaa5..b604f388de 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -821,7 +821,7 @@ impl SelectionHistory { struct RowHighlight { index: usize, - range: RangeInclusive, + range: Range, 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( &mut self, - range: RangeInclusive, + range: Range, color: Hsla, should_autoscroll: bool, cx: &mut ViewContext, @@ -11513,8 +11515,8 @@ impl Editor { let row_highlights = self.highlighted_rows.entry(TypeId::of::()).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( - &self, - ) -> impl '_ + Iterator, Hsla)> { + pub fn highlighted_rows(&self) -> impl '_ + Iterator, Hsla)> { self.highlighted_rows .get(&TypeId::of::()) .map_or(&[] as &[_], |vec| vec.as_slice()) @@ -11650,9 +11641,17 @@ impl Editor { .fold( BTreeMap::::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 } diff --git a/crates/editor/src/editor_tests.rs b/crates/editor/src/editor_tests.rs index b17d94a5eb..249d0a4746 100644 --- a/crates/editor/src/editor_tests.rs +++ b/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() { diff --git a/crates/editor/src/hunk_diff.rs b/crates/editor/src/hunk_diff.rs index 3e18b992c1..cf2a857b67 100644 --- a/crates/editor/src/hunk_diff.rs +++ b/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::( - 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::( - 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::( - 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, - snapshot: &EditorSnapshot, -) -> RangeInclusive { - 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 { diff --git a/crates/editor/src/test/editor_test_context.rs b/crates/editor/src/test/editor_test_context.rs index 2ec4f4a3b7..7234d97c5b 100644 --- a/crates/editor/src/test/editor_test_context.rs +++ b/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::() .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::>(); 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(), ); } } diff --git a/crates/go_to_line/src/go_to_line.rs b/crates/go_to_line/src/go_to_line.rs index fd631648c2..0e9482b759 100644 --- a/crates/go_to_line/src/go_to_line.rs +++ b/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::(); active_editor.highlight_rows::( - 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 "} }), ) diff --git a/crates/outline/src/outline.rs b/crates/outline/src/outline.rs index 520311b6f3..1d82d06ad8 100644 --- a/crates/outline/src/outline.rs +++ b/crates/outline/src/outline.rs @@ -143,7 +143,7 @@ impl OutlineViewDelegate { self.active_editor.update(cx, |active_editor, cx| { active_editor.clear_row_highlights::(); active_editor.highlight_rows::( - 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::(); active_editor.focus(cx);