fix(highlight_source): Apply styling to multiline annotations - #434
fix(highlight_source): Apply styling to multiline annotations#434momomuchu wants to merge 2 commits into
Conversation
|
Note that eventually we'd have to add a mechanism to provide highlighting exclusions (so that something like rustc can avoid highlighting trailing comments within the multiline span). |
epage
left a comment
There was a problem hiding this comment.
I'll probably leave any further reviewing to others but at least noticed this in pre-review that might help with the main review.
There was a problem hiding this comment.
Huh, we don't have a CONTRIBUTING.md for this but something we've found very helpful is to add tests in the commit before, with them passing, showing the bad behavior. This makes it so when we diff for code changes, we also diff for the effect of that code that change.
23aa1b6 to
117bf4d
Compare
Reproduces rust-lang#427: highlight_source(true) has no visible effect on multiline annotations (MultilineStart/MultilineLine/MultilineEnd). The generated snapshots show the code text without the expected styling. Fixed in the next commit.
highlight_source was silently ignored on MultilineStart/MultilineLine/ MultilineEnd annotations: the fast paths that render them could return before the styling logic ran, and the styling match arm was placed after the multiline connector arm so it never matched. Style multiline annotations up front instead. Fixes rust-lang#427
117bf4d to
1989758
Compare
|
Done. Restructured into two commits: the test first with pre-fix goldens showing the missing styling, then the fix with the goldens regenerated. Tree content is unchanged from the previous head, only the history was split. |
Muscraft
left a comment
There was a problem hiding this comment.
Thanks for taking the time to fix this issue! I'm sorry for the extremely long delay in reviewing this! My life has been very busy recently, leaving me with very little time to review this properly.
| continue; | ||
| } | ||
| let (start_char, end_char) = match ann.annotation_type { | ||
| LineAnnotationType::Singleline => continue, |
There was a problem hiding this comment.
| LineAnnotationType::Singleline => continue, | |
| LineAnnotationType::Singleline => (ann.start.char, ann.end.char), |
Note: You will need to remove the existing code for SingleLine:
annotate-snippets-rs/src/renderer/render.rs
Lines 1192 to 1200 in c203ec6
| let rest_of_line = line_info.line[ann.start.byte..].chars().count(); | ||
| (ann.start.char, ann.start.char + rest_of_line) |
There was a problem hiding this comment.
ann.start.byte is the byte offset from the start of the source, not the current line
| let rest_of_line = line_info.line[ann.start.byte..].chars().count(); | |
| (ann.start.char, ann.start.char + rest_of_line) | |
| (ann.start.char, line_info.line.chars().count()) |
| buffer.set_style_range( | ||
| line_offset, | ||
| (code_offset + start_char).saturating_sub(left), | ||
| (code_offset + end_char).saturating_sub(left), | ||
| underline.style, | ||
| ann.is_primary(), | ||
| ); |
There was a problem hiding this comment.
The current code does not account for tabs (\t), and should do something similar to style_substitution_highlights:
| buffer.set_style_range( | |
| line_offset, | |
| (code_offset + start_char).saturating_sub(left), | |
| (code_offset + end_char).saturating_sub(left), | |
| underline.style, | |
| ann.is_primary(), | |
| ); | |
| let extra_width_start = extra_width_from_tabs(line_info.line, start_char); | |
| let extra_width_end = extra_width_from_tabs(line_info.line, end_char); | |
| buffer.set_style_range( | |
| line_offset, | |
| (code_offset + start_char + extra_width_start).saturating_sub(left), | |
| (code_offset + end_char + extra_width_end).saturating_sub(left), | |
| underline.style, | |
| ann.is_primary(), | |
| ); | |
Possible regression test
use annotate_snippets::{
AnnotationKind, Group, Level, Patch, Renderer, Snippet, renderer::DecorStyle,
};
use snapbox::{assert_data_eq, file};
#[test]
fn case() {
let source = "prefix\tstart\n\tmiddle\nend";
let report = &[Group::with_level(Level::ERROR).element(
Snippet::source(source)
.annotation(AnnotationKind::Primary.span(7..24).highlight_source(true)),
)];
let expected_ascii = file!["<test-name>.ascii.term.svg": TermSvg];
let renderer = Renderer::styled();
assert_data_eq!(renderer.render(report), expected_ascii);
let expected_unicode = file!["<test-name>.unicode.term.svg": TermSvg];
let renderer = renderer.decor_style(DecorStyle::Unicode);
assert_data_eq!(renderer.render(report), expected_unicode);
}| // per-annotation styling logic matches on `MultilineStart`/`MultilineEnd` first to draw the | ||
| // horizontal connector, which made the `highlight_source` arm structurally unreachable for | ||
| // multiline annotations (see issue #427: `highlight_source` was silently ignored on them). | ||
| for ann in &line_info.annotations { |
There was a problem hiding this comment.
While playing around with these changes, I realized that when .fold(false) is set, lines that would normally be folded are not highlighted (when they should be). I suspect that annotated_lines is the source of the issue, but I have not checked.
Possible regression test
use annotate_snippets::{
AnnotationKind, Group, Level, Patch, Renderer, Snippet, renderer::DecorStyle,
};
use snapbox::{assert_data_eq, file};
#[test]
fn case() {
let source = r#"line
line
line
line
normally folded
normally folded
normally folded
line
primary end
normally folded
normally folded
normally folded
line
context end
"#;
let report = &[Group::with_level(Level::ERROR).element(
Snippet::source(source)
.fold(false)
.annotation(AnnotationKind::Primary.span(1..84).highlight_source(true))
.annotation(AnnotationKind::Context.span(0..149).highlight_source(true)),
)];
let expected_ascii = file!["<test-name>.ascii.term.svg": TermSvg];
let renderer = Renderer::styled();
assert_data_eq!(renderer.render(report), expected_ascii);
let expected_unicode = file!["<test-name>.unicode.term.svg": TermSvg];
let renderer = renderer.decor_style(DecorStyle::Unicode);
assert_data_eq!(renderer.render(report), expected_unicode);
}
Fixes #427.
highlight_source was silently ignored on multiline annotations
(MultilineStart/MultilineLine/MultilineEnd). Two things conspired to
cause this:
no leading text" case, and the "only connector lines, nothing to
show" case) that can return before the per-annotation styling logic
ever runs.
applies highlight_source is placed after the arm that draws the
MultilineStart/MultilineEnd connector, so it never matches for those
variants, since Rust match arms are first-match-wins.
Styling multiline annotations up front, before any of that, sidesteps
both issues instead of trying to thread a fix through three separate
spots.
Added a regression test (ascii + unicode goldens) using the repro from
the issue.