Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs in diff hunk highlighting #18454

Merged
merged 2 commits into from
Sep 27, 2024
Merged

Fix bugs in diff hunk highlighting #18454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be24757
Update diff hunk tests to reveal bugs in row highlighting
maxbrunsfeld Sep 27, 2024
6d40929
Change editor row highlight system to fix git hunk highlighting bugs
maxbrunsfeld Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions crates/assistant/src/inline_assistant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,7 +1142,7 @@ impl InlineAssistant {
for row_range in inserted_row_ranges {
editor.highlight_rows::<InlineAssist>(
row_range,
Some(cx.theme().status().info_background),
cx.theme().status().info_background,
false,
cx,
);
Expand Down Expand Up @@ -1209,7 +1209,7 @@ impl InlineAssistant {
editor.set_show_inline_completions(Some(false), cx);
editor.highlight_rows::<DeletedLines>(
Anchor::min()..=Anchor::max(),
Some(cx.theme().status().deleted_background),
cx.theme().status().deleted_background,
false,
cx,
);
Expand Down
5 changes: 4 additions & 1 deletion crates/editor/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ test-support = [
"workspace/test-support",
"tree-sitter-rust",
"tree-sitter-typescript",
"tree-sitter-html"
"tree-sitter-html",
"unindent",
]

[dependencies]
Expand Down Expand Up @@ -54,6 +55,7 @@ markdown.workspace = true
multi_buffer.workspace = true
ordered-float.workspace = true
parking_lot.workspace = true
pretty_assertions.workspace = true
project.workspace = true
rand.workspace = true
rpc.workspace = true
Expand All @@ -74,6 +76,7 @@ theme.workspace = true
tree-sitter-html = { workspace = true, optional = true }
tree-sitter-rust = { workspace = true, optional = true }
tree-sitter-typescript = { workspace = true, optional = true }
unindent = { workspace = true, optional = true }
ui.workspace = true
url.workspace = true
util.workspace = true
Expand Down
157 changes: 119 additions & 38 deletions crates/editor/src/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,7 +822,7 @@ impl SelectionHistory {
struct RowHighlight {
index: usize,
range: RangeInclusive<Anchor>,
color: Option<Hsla>,
color: Hsla,
should_autoscroll: bool,
}

Expand Down Expand Up @@ -11500,41 +11500,125 @@ impl Editor {
}
}

/// Adds or removes (on `None` color) a highlight for the rows corresponding to the anchor range given.
/// On matching anchor range, replaces the old highlight; does not clear the other existing highlights.
/// If multiple anchor ranges will produce highlights for the same row, the last range added will be used.
/// Adds a row highlight for the given range. If a row has multiple highlights, the
/// last highlight added will be used.
pub fn highlight_rows<T: 'static>(
&mut self,
rows: RangeInclusive<Anchor>,
color: Option<Hsla>,
range: RangeInclusive<Anchor>,
color: Hsla,
should_autoscroll: bool,
cx: &mut ViewContext<Self>,
) {
let snapshot = self.buffer().read(cx).snapshot(cx);
let row_highlights = self.highlighted_rows.entry(TypeId::of::<T>()).or_default();
let existing_highlight_index = row_highlights.binary_search_by(|highlight| {
highlight
.range
.start()
.cmp(rows.start(), &snapshot)
.then(highlight.range.end().cmp(rows.end(), &snapshot))
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))
});
match (color, existing_highlight_index) {
(Some(_), Ok(ix)) | (_, Err(ix)) => row_highlights.insert(
ix,
RowHighlight {
index: post_inc(&mut self.highlight_order),
range: rows,
should_autoscroll,
color,
},
),
(None, Ok(i)) => {
row_highlights.remove(i);

if let Err(mut ix) = ix {
let index = post_inc(&mut self.highlight_order);

// If this range intersects with the preceding highlight, then merge it with
// the preceding highlight. Otherwise insert a new highlight.
let mut merged = false;
if ix > 0 {
let prev_highlight = &mut row_highlights[ix - 1];
if prev_highlight
.range
.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();
}
merged = true;
prev_highlight.index = index;
prev_highlight.color = color;
prev_highlight.should_autoscroll = should_autoscroll;
}
}

if !merged {
row_highlights.insert(
ix,
RowHighlight {
range: range.clone(),
index,
color,
should_autoscroll,
},
);
}

// If any of the following highlights intersect with this one, merge them.
while let Some(next_highlight) = row_highlights.get(ix + 1) {
let highlight = &row_highlights[ix];
if next_highlight
.range
.start()
.cmp(&highlight.range.end(), &snapshot)
.is_le()
{
if next_highlight
.range
.end()
.cmp(&highlight.range.end(), &snapshot)
.is_gt()
{
row_highlights[ix].range =
*highlight.range.start()..=*next_highlight.range.end();
}
row_highlights.remove(ix + 1);
} else {
break;
}
}
}
}

/// Remove any highlighted row ranges of the given type that intersect the
/// given ranges.
pub fn remove_highlighted_rows<T: 'static>(
&mut self,
ranges_to_remove: Vec<Range<Anchor>>,
cx: &mut ViewContext<Self>,
) {
let snapshot = self.buffer().read(cx).snapshot(cx);
let row_highlights = self.highlighted_rows.entry(TypeId::of::<T>()).or_default();
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 => {
ranges_to_remove.next();
}
Ordering::Equal => {
return false;
}
Ordering::Greater => {
match range_to_remove.start.cmp(&highlight.range.end(), &snapshot) {
Ordering::Less | Ordering::Equal => {
return false;
}
Ordering::Greater => break,
}
}
}
}

true
})
}

/// Clear all anchor ranges for a certain highlight context type, so no corresponding rows will be highlighted.
pub fn clear_row_highlights<T: 'static>(&mut self) {
self.highlighted_rows.remove(&TypeId::of::<T>());
Expand All @@ -11543,13 +11627,12 @@ 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,
) -> Option<impl Iterator<Item = (&RangeInclusive<Anchor>, Option<&Hsla>)>> {
Some(
self.highlighted_rows
.get(&TypeId::of::<T>())?
.iter()
.map(|highlight| (&highlight.range, highlight.color.as_ref())),
)
) -> impl '_ + Iterator<Item = (RangeInclusive<Anchor>, Hsla)> {
self.highlighted_rows
.get(&TypeId::of::<T>())
.map_or(&[] as &[_], |vec| vec.as_slice())
.iter()
.map(|highlight| (highlight.range.clone(), highlight.color))
}

/// Merges all anchor ranges for all context types ever set, picking the last highlight added in case of a row conflict.
Expand All @@ -11574,10 +11657,7 @@ impl Editor {
used_highlight_orders.entry(row).or_insert(highlight.index);
if highlight.index >= *used_index {
*used_index = highlight.index;
match highlight.color {
Some(hsla) => unique_rows.insert(DisplayRow(row), hsla),
None => unique_rows.remove(&DisplayRow(row)),
};
unique_rows.insert(DisplayRow(row), highlight.color);
}
}
unique_rows
Expand All @@ -11593,10 +11673,11 @@ impl Editor {
.values()
.flat_map(|highlighted_rows| highlighted_rows.iter())
.filter_map(|highlight| {
if highlight.color.is_none() || !highlight.should_autoscroll {
return None;
if highlight.should_autoscroll {
Some(highlight.range.start().to_display_point(snapshot).row())
} else {
None
}
Some(highlight.range.start().to_display_point(snapshot).row())
})
.min()
}
Expand Down
Loading
Loading