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

Jump to bottom

Fix precedence of ui.virtual.whitespace #8750

Merged
merged 3 commits into from
Nov 22, 2023
Merged

Fix precedence of ui.virtual.whitespace #8750

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92e6932
Separate iters for overlay and syntax highlighting.
chtenb Nov 7, 2023
d925c0c
Fix clippy warning
chtenb Nov 9, 2023
c44513d
Address feedback
chtenb Nov 21, 2023
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
84 changes: 60 additions & 24 deletions helix-term/src/ui/document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ pub fn render_document(
doc: &Document,
offset: ViewPosition,
doc_annotations: &TextAnnotations,
highlight_iter: impl Iterator<Item = HighlightEvent>,
syntax_highlight_iter: impl Iterator<Item = HighlightEvent>,
overlay_highlight_iter: impl Iterator<Item = HighlightEvent>,
theme: &Theme,
line_decoration: &mut [Box<dyn LineDecoration + '_>],
translated_positions: &mut [TranslatedPosition],
Expand All @@ -109,7 +110,8 @@ pub fn render_document(
offset,
&doc.text_format(viewport.width, Some(theme)),
doc_annotations,
highlight_iter,
syntax_highlight_iter,
overlay_highlight_iter,
theme,
line_decoration,
translated_positions,
Expand Down Expand Up @@ -157,7 +159,8 @@ pub fn render_text<'t>(
offset: ViewPosition,
text_fmt: &TextFormat,
text_annotations: &TextAnnotations,
highlight_iter: impl Iterator<Item = HighlightEvent>,
syntax_highlight_iter: impl Iterator<Item = HighlightEvent>,
overlay_highlight_iter: impl Iterator<Item = HighlightEvent>,
theme: &Theme,
line_decorations: &mut [Box<dyn LineDecoration + '_>],
translated_positions: &mut [TranslatedPosition],
Expand All @@ -178,10 +181,16 @@ pub fn render_text<'t>(

let (mut formatter, mut first_visible_char_idx) =
DocumentFormatter::new_at_prev_checkpoint(text, text_fmt, text_annotations, offset.anchor);
let mut styles = StyleIter {
let mut syntax_styles = StyleIter {
text_style: renderer.text_style,
active_highlights: Vec::with_capacity(64),
highlight_iter,
highlight_iter: syntax_highlight_iter,
theme,
};
let mut overlay_styles = StyleIter {
text_style: renderer.text_style,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using render.text_style is wrong here it will set the whole document to the text colour as the overlay is patched last, this should just be Style::default so if there is no style here we don't override the past text highlighting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see StyleIter for why it will highlight all the text the default text color:

helix/helix-term/src/ui/document.rs

Lines 64 to 66 in 41b307b

.fold(self.text_style, |acc, span| {
acc.patch(self.theme.highlight(span.0))
});
as overlay_highlight_iter covers the whole viewport

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this more deeply, this text_style is set based on the given ui.text scope. The documentation about this says "Command prompts, popup text, etc.", but as this is really the default text style that is used, we should probably change that description.

active_highlights: Vec::with_capacity(64),
highlight_iter: overlay_highlight_iter,
theme,
};

Expand All @@ -193,7 +202,10 @@ pub fn render_text<'t>(
};
let mut is_in_indent_area = true;
let mut last_line_indent_level = 0;
let mut style_span = styles
let mut syntax_style_span = syntax_styles
.next()
.unwrap_or_else(|| (Style::default(), usize::MAX));
let mut overlay_style_span = overlay_styles
.next()
.unwrap_or_else(|| (Style::default(), usize::MAX));

Expand Down Expand Up @@ -221,9 +233,16 @@ pub fn render_text<'t>(

// skip any graphemes on visual lines before the block start
if pos.row < row_off {
if char_pos >= style_span.1 {
style_span = if let Some(style_span) = styles.next() {
style_span
if char_pos >= syntax_style_span.1 {
syntax_style_span = if let Some(syntax_style_span) = syntax_styles.next() {
syntax_style_span
} else {
break;
}
}
if char_pos >= overlay_style_span.1 {
overlay_style_span = if let Some(overlay_style_span) = overlay_styles.next() {
overlay_style_span
} else {
break;
}
Expand Down Expand Up @@ -260,8 +279,15 @@ pub fn render_text<'t>(
}

// acquire the correct grapheme style
if char_pos >= style_span.1 {
style_span = styles.next().unwrap_or((Style::default(), usize::MAX));
if char_pos >= syntax_style_span.1 {
syntax_style_span = syntax_styles
.next()
.unwrap_or((Style::default(), usize::MAX));
}
if char_pos >= overlay_style_span.1 {
overlay_style_span = overlay_styles
.next()
.unwrap_or((Style::default(), usize::MAX));
}
char_pos += grapheme.doc_chars();

Expand All @@ -275,22 +301,25 @@ pub fn render_text<'t>(
pos,
);

let grapheme_style = if let GraphemeSource::VirtualText { highlight } = grapheme.source {
let style = renderer.text_style;
if let Some(highlight) = highlight {
style.patch(theme.highlight(highlight.0))
let (syntax_style, overlay_style) =
if let GraphemeSource::VirtualText { highlight } = grapheme.source {
let mut style = renderer.text_style;
if let Some(highlight) = highlight {
style = style.patch(theme.highlight(highlight.0))
}
(style, Style::default())
} else {
style
}
} else {
style_span.0
};
(syntax_style_span.0, overlay_style_span.0)
};

let virt = grapheme.is_virtual();
let is_virtual = grapheme.is_virtual();
renderer.draw_grapheme(
grapheme.grapheme,
grapheme_style,
virt,
GraphemeStyle {
syntax_style,
overlay_style,
},
is_virtual,
&mut last_line_indent_level,
&mut is_in_indent_area,
pos,
Expand Down Expand Up @@ -322,6 +351,11 @@ pub struct TextRenderer<'a> {
pub viewport: Rect,
}

pub struct GraphemeStyle {
syntax_style: Style,
overlay_style: Style,
}

impl<'a> TextRenderer<'a> {
pub fn new(
surface: &'a mut Surface,
Expand Down Expand Up @@ -395,7 +429,7 @@ impl<'a> TextRenderer<'a> {
pub fn draw_grapheme(
&mut self,
grapheme: Grapheme,
mut style: Style,
grapheme_style: GraphemeStyle,
is_virtual: bool,
last_indent_level: &mut usize,
is_in_indent_area: &mut bool,
Expand All @@ -405,9 +439,11 @@ impl<'a> TextRenderer<'a> {
let is_whitespace = grapheme.is_whitespace();

// TODO is it correct to apply the whitespace style to all unicode white spaces?
let mut style = grapheme_style.syntax_style;
if is_whitespace {
style = style.patch(self.whitespace_style);
}
style = style.patch(grapheme_style.overlay_style);

let width = grapheme.width();
let space = if is_virtual { " " } else { &self.space };
Expand Down
96 changes: 58 additions & 38 deletions helix-term/src/ui/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,20 @@ impl EditorView {
line_decorations.push(Box::new(line_decoration));
}

let mut highlights =
let syntax_highlights =
Self::doc_syntax_highlights(doc, view.offset.anchor, inner.height, theme);
let overlay_highlights = Self::overlay_syntax_highlights(

let mut overlay_highlights =
Self::empty_highlight_iter(doc, view.offset.anchor, inner.height);
let overlay_syntax_highlights = Self::overlay_syntax_highlights(
doc,
view.offset.anchor,
inner.height,
&text_annotations,
);
if !overlay_highlights.is_empty() {
highlights = Box::new(syntax::merge(highlights, overlay_highlights));
if !overlay_syntax_highlights.is_empty() {
overlay_highlights =
Box::new(syntax::merge(overlay_highlights, overlay_syntax_highlights));
}

for diagnostic in Self::doc_diagnostics_highlights(doc, theme) {
Expand All @@ -142,12 +146,12 @@ impl EditorView {
if diagnostic.is_empty() {
continue;
}
highlights = Box::new(syntax::merge(highlights, diagnostic));
overlay_highlights = Box::new(syntax::merge(overlay_highlights, diagnostic));
}

let highlights: Box<dyn Iterator<Item = HighlightEvent>> = if is_focused {
if is_focused {
let highlights = syntax::merge(
highlights,
overlay_highlights,
Self::doc_selection_highlights(
editor.mode(),
doc,
Expand All @@ -158,13 +162,11 @@ impl EditorView {
);
let focused_view_elements = Self::highlight_focused_view_elements(view, doc, theme);
if focused_view_elements.is_empty() {
Box::new(highlights)
overlay_highlights = Box::new(highlights)
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
} else {
Box::new(syntax::merge(highlights, focused_view_elements))
overlay_highlights = Box::new(syntax::merge(highlights, focused_view_elements))
}
} else {
Box::new(highlights)
};
}

let gutter_overflow = view.gutter_offset(doc) == 0;
if !gutter_overflow {
Expand Down Expand Up @@ -197,7 +199,8 @@ impl EditorView {
doc,
view.offset,
&text_annotations,
highlights,
syntax_highlights,
overlay_highlights,
theme,
&mut line_decorations,
&mut translated_positions,
Expand Down Expand Up @@ -257,27 +260,39 @@ impl EditorView {
.for_each(|area| surface.set_style(area, ruler_theme))
}

pub fn overlay_syntax_highlights(
fn viewport_byte_range(
text: helix_core::RopeSlice,
row: usize,
height: u16,
) -> std::ops::Range<usize> {
// Calculate viewport byte ranges:
// Saturating subs to make it inclusive zero indexing.
let last_line = text.len_lines().saturating_sub(1);
let last_visible_line = (row + height as usize).saturating_sub(1).min(last_line);
let start = text.line_to_byte(row.min(last_line));
let end = text.line_to_byte(last_visible_line + 1);

start..end
}

pub fn empty_highlight_iter(
doc: &Document,
anchor: usize,
height: u16,
text_annotations: &TextAnnotations,
) -> Vec<(usize, std::ops::Range<usize>)> {
) -> Box<dyn Iterator<Item = HighlightEvent>> {
let text = doc.text().slice(..);
let row = text.char_to_line(anchor.min(text.len_chars()));

let range = {
// Calculate viewport byte ranges:
// Saturating subs to make it inclusive zero indexing.
let last_line = text.len_lines().saturating_sub(1);
let last_visible_line = (row + height as usize).saturating_sub(1).min(last_line);
let start = text.line_to_byte(row.min(last_line));
let end = text.line_to_byte(last_visible_line + 1);

start..end
};

text_annotations.collect_overlay_highlights(range)
// Calculate viewport byte ranges:
// Saturating subs to make it inclusive zero indexing.
let range = Self::viewport_byte_range(text, row, height);
Box::new(
[HighlightEvent::Source {
start: text.byte_to_char(range.start),
end: text.byte_to_char(range.end),
}]
.into_iter(),
)
}

/// Get syntax highlights for a document in a view represented by the first line
Expand All @@ -292,16 +307,7 @@ impl EditorView {
let text = doc.text().slice(..);
let row = text.char_to_line(anchor.min(text.len_chars()));

let range = {
// Calculate viewport byte ranges:
// Saturating subs to make it inclusive zero indexing.
let last_line = text.len_lines().saturating_sub(1);
let last_visible_line = (row + height as usize).saturating_sub(1).min(last_line);
let start = text.line_to_byte(row.min(last_line));
let end = text.line_to_byte(last_visible_line + 1);

start..end
};
let range = Self::viewport_byte_range(text, row, height);

match doc.syntax() {
Some(syntax) => {
Expand Down Expand Up @@ -334,6 +340,20 @@ impl EditorView {
}
}

pub fn overlay_syntax_highlights(
doc: &Document,
anchor: usize,
height: u16,
text_annotations: &TextAnnotations,
) -> Vec<(usize, std::ops::Range<usize>)> {
let text = doc.text().slice(..);
let row = text.char_to_line(anchor.min(text.len_chars()));

let range = Self::viewport_byte_range(text, row, height);

text_annotations.collect_overlay_highlights(range)
}

/// Get highlight spans for document diagnostics
pub fn doc_diagnostics_highlights(
doc: &Document,
Expand Down
10 changes: 7 additions & 3 deletions helix-term/src/ui/picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -736,17 +736,20 @@ impl<T: Item + 'static> Picker<T> {
}
}

let mut highlights = EditorView::doc_syntax_highlights(
let syntax_highlights = EditorView::doc_syntax_highlights(
doc,
offset.anchor,
area.height,
&cx.editor.theme,
);

let mut overlay_highlights =
EditorView::empty_highlight_iter(doc, offset.anchor, area.height);
for spans in EditorView::doc_diagnostics_highlights(doc, &cx.editor.theme) {
if spans.is_empty() {
continue;
}
highlights = Box::new(helix_core::syntax::merge(highlights, spans));
overlay_highlights = Box::new(helix_core::syntax::merge(overlay_highlights, spans));
}
let mut decorations: Vec<Box<dyn LineDecoration>> = Vec::new();

Expand Down Expand Up @@ -777,7 +780,8 @@ impl<T: Item + 'static> Picker<T> {
offset,
// TODO: compute text annotations asynchronously here (like inlay hints)
&TextAnnotations::default(),
highlights,
syntax_highlights,
overlay_highlights,
&cx.editor.theme,
&mut decorations,
&mut [],
Expand Down
Loading