Skip to content

feat(config): add [workspace-]diagnostics fields to statusline #13288

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

Merged
merged 3 commits into from
Apr 8, 2025
Merged

feat(config): add [workspace-]diagnostics fields to statusline #13288

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
601fcb1
feat(config): add `rev` variant to `diagnostics` and a display `min` …
RoloEdits Apr 7, 2025
a9d3c65
refactor: change `w_diagnostics` to `workspace_diagnostics`
RoloEdits Apr 7, 2025
3d4125b
docs(editor): add entries for `diagnostics` and `workspace-diagnostics`
RoloEdits Apr 7, 2025
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
162 changes: 102 additions & 60 deletions helix-term/src/ui/statusline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,78 +226,120 @@ fn render_diagnostics<F>(context: &mut RenderContext, write: F)
where
F: Fn(&mut RenderContext, String, Option<Style>) + Copy,
{
let (warnings, errors) = context
.doc
.diagnostics()
.iter()
.fold((0, 0), |mut counts, diag| {
use helix_core::diagnostic::Severity;
match diag.severity {
Some(Severity::Warning) => counts.0 += 1,
Some(Severity::Error) | None => counts.1 += 1,
_ => {}
}
counts
});

if warnings > 0 {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("warning")),
);
write(context, format!(" {} ", warnings), None);
}
use helix_core::diagnostic::Severity;
let (hints, info, warnings, errors) =
context
.doc
.diagnostics()
.iter()
.fold((0, 0, 0, 0), |mut counts, diag| {
match diag.severity {
Some(Severity::Hint) | None => counts.0 += 1,
Some(Severity::Info) => counts.1 += 1,
Some(Severity::Warning) => counts.2 += 1,
Some(Severity::Error) => counts.3 += 1,
}
counts
});
Comment on lines +235 to +243
Copy link
Contributor

@Rudxain Rudxain Apr 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It could be smaller (both the source and the machine-code)

I'm unsure if the code is worth rewriting as it's not really perf-critical.

However, the implicit i32 instead of something like u16 feels like it could be improved.

I had the idea that if all of the counts are "exhausted" (either by overflow, or when reaching an arbitrary limit), then the loop can break early and the counts could be displayed as "999+" (just an example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the datatype to something that cannot be negative could be a type improvement, though we already start them at 0 and only count up, but adding a branch just for checking if its 999 and then again for if there should be a + would probably remove any gained performance for anything else we do.

Most of the time its only looped <100 times, which is very quick no matter the instructions, and ive not seen this be a hotspot. The main one with the statusline (so far) was patched in #12385 (it even shows the statusline highlighted) for the relative path.

As far as the easiest win, even just reordering the most often case to the top and least common at the bottom would address most branches we get here. Its almost never None, yet there is a branch for that at the top. Given the defaults, having Warning, Error, Hint, Info, and then None, top to bottom, would probably be best order?

Thankfully, helix runs very well, and from what I have seen in profiling, although has some hotspots, they might just need to take that long for the work being done.

Addressing this match loop would be one of the very last things I would look at for perf gains.

If you want to start to target some performance bottlenecks, I think opening an issue could bring in collaboration (and spotlight to issues). For instance, I have been meaning to look at the path normalization function, as that takes a lot of time, and look into a full lexical resolution ( .. in paths), with a syscall as a fallback. I just dont know enough about this domain to really say if it can be avoided or not.

There has also been talk to get a benchmark suite setup for some critical areas first. This way we can test things like having static collections be a Vec, or if a HashMap would actually be faster at the size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the datatype to something that cannot be negative could be a type improvement, though we already start them at 0 and only count up, but adding a branch just for checking if its 999 and then again for if there should be a + would probably remove any gained performance for anything else we do.

Most of the time its only looped <100 times, which is very quick no matter the instructions, and ive not seen this be a hotspot.

Fair enough

As far as the easiest win, even just reordering the most often case

I think it would be better to wait for likely to stabilize

Thankfully, helix runs very well, and from what I have seen in profiling, although has some hotspots, they might just need to take that long for the work being done.

Fair enough x2

Addressing this match loop would be one of the very last things I would look at for perf gains.

I agree, lol. Even if it was perf-critical, it would still be efficient-enough in practice

[...] path normalization function, as that takes a lot of time, and look into a full lexical resolution ( .. in paths), with a syscall as a fallback

I might check that code later. Thanks for the pointers!


if errors > 0 {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("error")),
);
write(context, format!(" {} ", errors), None);
for sev in &context.editor.config().statusline.diagnostics {
match sev {
Severity::Hint if hints > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("hint")),
);
write(context, format!(" {} ", hints), None);
}
Severity::Info if info > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("info")),
);
write(context, format!(" {} ", info), None);
}
Severity::Warning if warnings > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("warning")),
);
write(context, format!(" {} ", warnings), None);
}
Severity::Error if errors > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("error")),
);
write(context, format!(" {} ", errors), None);
}
_ => {}
}
}
}

fn render_workspace_diagnostics<F>(context: &mut RenderContext, write: F)
where
F: Fn(&mut RenderContext, String, Option<Style>) + Copy,
{
let (warnings, errors) =
context
.editor
.diagnostics
.values()
.flatten()
.fold((0, 0), |mut counts, (diag, _)| {
match diag.severity {
Some(DiagnosticSeverity::WARNING) => counts.0 += 1,
Some(DiagnosticSeverity::ERROR) | None => counts.1 += 1,
_ => {}
}
counts
});
use helix_core::diagnostic::Severity;
let (hints, info, warnings, errors) = context.editor.diagnostics.values().flatten().fold(
(0, 0, 0, 0),
|mut counts, (diag, _)| {
match diag.severity {
Some(DiagnosticSeverity::HINT) | None => counts.0 += 1,
Some(DiagnosticSeverity::INFORMATION) => counts.1 += 1,
Some(DiagnosticSeverity::WARNING) => counts.2 += 1,
Some(DiagnosticSeverity::ERROR) => counts.3 += 1,
_ => {}
}
counts
},
);

if warnings > 0 || errors > 0 {
if hints > 0 || info > 0 || warnings > 0 || errors > 0 {
write(context, " W ".into(), None);
}

if warnings > 0 {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("warning")),
);
write(context, format!(" {} ", warnings), None);
}

if errors > 0 {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("error")),
);
write(context, format!(" {} ", errors), None);
for sev in &context.editor.config().statusline.w_diagnostics {
match sev {
Severity::Hint if hints > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("hint")),
);
write(context, format!(" {} ", hints), None);
}
Severity::Info if info > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("info")),
);
write(context, format!(" {} ", info), None);
}
Severity::Warning if warnings > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("warning")),
);
write(context, format!(" {} ", warnings), None);
}
Severity::Error if errors > 0 => {
write(
context,
"●".to_string(),
Some(context.editor.theme.get("error")),
);
write(context, format!(" {} ", errors), None);
}
_ => {}
}
}
}

Expand Down
4 changes: 4 additions & 0 deletions helix-view/src/editor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,8 @@ pub struct StatusLineConfig {
pub right: Vec<StatusLineElement>,
pub separator: String,
pub mode: ModeConfig,
pub diagnostics: Vec<Severity>,
pub w_diagnostics: Vec<Severity>,
}

impl Default for StatusLineConfig {
Expand All @@ -521,6 +523,8 @@ impl Default for StatusLineConfig {
],
separator: String::from("│"),
mode: ModeConfig::default(),
diagnostics: vec![Severity::Warning, Severity::Error],
w_diagnostics: vec![Severity::Warning, Severity::Error],
}
}
}
Expand Down