-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Taskbar progress reporting via ANSI codes #14615
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,112 @@ struct Format { | |
style: ProgressStyle, | ||
max_width: usize, | ||
max_print: usize, | ||
taskbar: TaskbarProgress, | ||
} | ||
|
||
/// Taskbar progressbar | ||
/// | ||
/// Outputs ANSI sequences according to the `Operating system commands`. | ||
struct TaskbarProgress { | ||
enabled: bool, | ||
error: bool, | ||
} | ||
|
||
/// A taskbar progress value printable as ANSI OSC 9;4 escape code | ||
#[cfg_attr(test, derive(PartialEq, Debug))] | ||
enum TaskbarValue { | ||
/// No output. | ||
None, | ||
/// Remove progress. | ||
Remove, | ||
/// Progress value (0-100). | ||
Value(f64), | ||
/// Indeterminate state (no bar, just animation) | ||
Indeterminate, | ||
/// Progress value in an error state (0-100). | ||
Error(f64), | ||
} | ||
|
||
enum ProgressOutput { | ||
/// Print progress without a message | ||
PrintNow, | ||
/// Progress, message and taskbar progress | ||
TextAndTaskbar(String, TaskbarValue), | ||
/// Only taskbar progress, no message and no text progress | ||
Taskbar(TaskbarValue), | ||
} | ||
|
||
impl TaskbarProgress { | ||
#[cfg(test)] | ||
fn new(enabled: bool) -> Self { | ||
Self { | ||
enabled, | ||
error: false, | ||
} | ||
} | ||
|
||
/// Create a `TaskbarProgress` from Cargo's configuration. | ||
/// Autodetects support if not explicitly enabled or disabled. | ||
fn from_config(gctx: &GlobalContext) -> Self { | ||
let enabled = gctx.progress_config().taskbar.unwrap_or_else(|| { | ||
// Windows Terminal session. | ||
gctx.get_env("WT_SESSION").is_ok() | ||
// Compatibility with ConEmu's ANSI support. | ||
|| gctx.get_env("ConEmuANSI").ok() == Some("ON".into()) | ||
}); | ||
Comment on lines
+124
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we break the detection out into a function like we do for hyperlinks? (and this would be allowed to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally I see us breaking this out into a crate as some point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll see what I can do.
After Lennart's push I think that it may be beneficial for the community to have a crate exactly for this one purpose: detecting supported terminal and serializing progress value and status into escape sequences. |
||
|
||
Self { | ||
enabled, | ||
error: false, | ||
} | ||
} | ||
|
||
fn progress_state(&self, value: TaskbarValue) -> TaskbarValue { | ||
match (self.enabled, self.error) { | ||
(true, false) => value, | ||
(true, true) => match value { | ||
TaskbarValue::Value(v) => TaskbarValue::Error(v), | ||
_ => TaskbarValue::Error(100.0), | ||
}, | ||
(false, _) => TaskbarValue::None, | ||
} | ||
} | ||
|
||
pub fn remove(&self) -> TaskbarValue { | ||
self.progress_state(TaskbarValue::Remove) | ||
} | ||
|
||
pub fn value(&self, percent: f64) -> TaskbarValue { | ||
self.progress_state(TaskbarValue::Value(percent)) | ||
} | ||
|
||
pub fn indeterminate(&self) -> TaskbarValue { | ||
self.progress_state(TaskbarValue::Indeterminate) | ||
} | ||
|
||
pub fn error(&mut self) { | ||
self.error = true; | ||
} | ||
} | ||
|
||
impl std::fmt::Display for TaskbarValue { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
// From https://conemu.github.io/en/AnsiEscapeCodes.html#ConEmu_specific_OSC | ||
// ESC ] 9 ; 4 ; st ; pr ST | ||
// When st is 0: remove progress. | ||
// When st is 1: set progress value to pr (number, 0-100). | ||
// When st is 2: set error state in taskbar, pr is optional. | ||
epage marked this conversation as resolved.
Show resolved
Hide resolved
Comment on lines
+167
to
+171
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From #14615 (comment)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am hesitant to have a ctrl+c handler but I also have had enough bad experience on Windows with colors being leaked from a program and could see myself being annoyed about progress state leaking as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's my prvious work on ctrl+c handler: de22a7f The approach works, but then we need to set the handler depending on Progress.state. Can you please advice on this? Do you want to have a handler in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ehuss I'd be interested in your opinion on this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we're now detecting a terminal regardless of the OS, a ctrl+c handler needs to be portable too, not like in my first implementation. If we still want to go this way, a few questions:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is used when cargo executes binaries for |
||
// When st is 3: set indeterminate state, pr is ignored. | ||
// When st is 4: set paused state, pr is optional. | ||
let (state, progress) = match self { | ||
Self::None => return Ok(()), // No output | ||
Self::Remove => (0, 0.0), | ||
Self::Value(v) => (1, *v), | ||
Self::Indeterminate => (3, 0.0), | ||
Self::Error(v) => (2, *v), | ||
}; | ||
write!(f, "\x1b]9;4;{state};{progress:.0}\x1b\\") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like it was the right choise, see recent patch comments for systemd. Also: |
||
} | ||
} | ||
|
||
impl<'gctx> Progress<'gctx> { | ||
|
@@ -126,6 +232,7 @@ impl<'gctx> Progress<'gctx> { | |
// 50 gives some space for text after the progress bar, | ||
// even on narrow (e.g. 80 char) terminals. | ||
max_print: 50, | ||
taskbar: TaskbarProgress::from_config(gctx), | ||
}, | ||
name: name.to_string(), | ||
done: false, | ||
|
@@ -223,7 +330,7 @@ impl<'gctx> Progress<'gctx> { | |
/// calling it too often. | ||
pub fn print_now(&mut self, msg: &str) -> CargoResult<()> { | ||
match &mut self.state { | ||
Some(s) => s.print("", msg), | ||
Some(s) => s.print(ProgressOutput::PrintNow, msg), | ||
None => Ok(()), | ||
} | ||
} | ||
|
@@ -234,6 +341,13 @@ impl<'gctx> Progress<'gctx> { | |
s.clear(); | ||
} | ||
} | ||
|
||
/// Sets the taskbar progress to the error state. | ||
pub fn indicate_error(&mut self) { | ||
if let Some(s) = &mut self.state { | ||
s.format.taskbar.error() | ||
} | ||
} | ||
} | ||
|
||
impl Throttle { | ||
|
@@ -269,6 +383,7 @@ impl Throttle { | |
impl<'gctx> State<'gctx> { | ||
fn tick(&mut self, cur: usize, max: usize, msg: &str) -> CargoResult<()> { | ||
if self.done { | ||
write!(self.gctx.shell().err(), "{}", self.format.taskbar.remove())?; | ||
return Ok(()); | ||
} | ||
|
||
|
@@ -280,21 +395,30 @@ impl<'gctx> State<'gctx> { | |
// return back to the beginning of the line for the next print. | ||
self.try_update_max_width(); | ||
if let Some(pbar) = self.format.progress(cur, max) { | ||
self.print(&pbar, msg)?; | ||
self.print(pbar, msg)?; | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn print(&mut self, prefix: &str, msg: &str) -> CargoResult<()> { | ||
fn print(&mut self, progress: ProgressOutput, msg: &str) -> CargoResult<()> { | ||
self.throttle.update(); | ||
self.try_update_max_width(); | ||
|
||
let (mut line, taskbar) = match progress { | ||
ProgressOutput::PrintNow => (String::new(), None), | ||
ProgressOutput::TextAndTaskbar(prefix, taskbar_value) => (prefix, Some(taskbar_value)), | ||
ProgressOutput::Taskbar(taskbar_value) => (String::new(), Some(taskbar_value)), | ||
}; | ||
|
||
// make sure we have enough room for the header | ||
if self.format.max_width < 15 { | ||
// even if we don't have space we can still output taskbar progress | ||
if let Some(tb) = taskbar { | ||
write!(self.gctx.shell().err(), "{tb}\r")?; | ||
} | ||
return Ok(()); | ||
} | ||
|
||
let mut line = prefix.to_string(); | ||
self.format.render(&mut line, msg); | ||
while line.len() < self.format.max_width - 15 { | ||
line.push(' '); | ||
|
@@ -305,7 +429,11 @@ impl<'gctx> State<'gctx> { | |
let mut shell = self.gctx.shell(); | ||
shell.set_needs_clear(false); | ||
shell.status_header(&self.name)?; | ||
write!(shell.err(), "{}\r", line)?; | ||
if let Some(tb) = taskbar { | ||
write!(shell.err(), "{line}{tb}\r")?; | ||
} else { | ||
write!(shell.err(), "{line}\r")?; | ||
} | ||
self.last_line = Some(line); | ||
shell.set_needs_clear(true); | ||
} | ||
|
@@ -314,6 +442,8 @@ impl<'gctx> State<'gctx> { | |
} | ||
|
||
fn clear(&mut self) { | ||
// Always clear the taskbar progress | ||
let _ = write!(self.gctx.shell().err(), "{}", self.format.taskbar.remove()); | ||
// No need to clear if the progress is not currently being displayed. | ||
if self.last_line.is_some() && !self.gctx.shell().is_cleared() { | ||
self.gctx.shell().err_erase_line(); | ||
|
@@ -331,19 +461,27 @@ impl<'gctx> State<'gctx> { | |
} | ||
|
||
impl Format { | ||
fn progress(&self, cur: usize, max: usize) -> Option<String> { | ||
fn progress(&self, cur: usize, max: usize) -> Option<ProgressOutput> { | ||
assert!(cur <= max); | ||
// Render the percentage at the far right and then figure how long the | ||
// progress bar is | ||
let pct = (cur as f64) / (max as f64); | ||
let pct = if !pct.is_finite() { 0.0 } else { pct }; | ||
let stats = match self.style { | ||
ProgressStyle::Percentage => format!(" {:6.02}%", pct * 100.0), | ||
ProgressStyle::Ratio => format!(" {}/{}", cur, max), | ||
ProgressStyle::Ratio => format!(" {cur}/{max}"), | ||
ProgressStyle::Indeterminate => String::new(), | ||
}; | ||
let taskbar = match self.style { | ||
ProgressStyle::Percentage | ProgressStyle::Ratio => self.taskbar.value(pct * 100.0), | ||
ProgressStyle::Indeterminate => self.taskbar.indeterminate(), | ||
}; | ||
|
||
let extra_len = stats.len() + 2 /* [ and ] */ + 15 /* status header */; | ||
let Some(display_width) = self.width().checked_sub(extra_len) else { | ||
if self.taskbar.enabled { | ||
return Some(ProgressOutput::Taskbar(taskbar)); | ||
} | ||
return None; | ||
}; | ||
|
||
|
@@ -371,7 +509,7 @@ impl Format { | |
string.push(']'); | ||
string.push_str(&stats); | ||
|
||
Some(string) | ||
Some(ProgressOutput::TextAndTaskbar(string, taskbar)) | ||
} | ||
|
||
fn render(&self, string: &mut String, msg: &str) { | ||
|
@@ -398,7 +536,11 @@ impl Format { | |
|
||
#[cfg(test)] | ||
fn progress_status(&self, cur: usize, max: usize, msg: &str) -> Option<String> { | ||
let mut ret = self.progress(cur, max)?; | ||
let mut ret = match self.progress(cur, max)? { | ||
// Check only the variant that contains text | ||
ProgressOutput::TextAndTaskbar(text, _) => text, | ||
_ => return None, | ||
}; | ||
self.render(&mut ret, msg); | ||
Some(ret) | ||
} | ||
|
@@ -420,6 +562,7 @@ fn test_progress_status() { | |
style: ProgressStyle::Ratio, | ||
max_print: 40, | ||
max_width: 60, | ||
taskbar: TaskbarProgress::new(false), | ||
}; | ||
assert_eq!( | ||
format.progress_status(0, 4, ""), | ||
|
@@ -493,6 +636,7 @@ fn test_progress_status_percentage() { | |
style: ProgressStyle::Percentage, | ||
max_print: 40, | ||
max_width: 60, | ||
taskbar: TaskbarProgress::new(false), | ||
}; | ||
assert_eq!( | ||
format.progress_status(0, 77, ""), | ||
|
@@ -518,6 +662,7 @@ fn test_progress_status_too_short() { | |
style: ProgressStyle::Percentage, | ||
max_print: 25, | ||
max_width: 25, | ||
taskbar: TaskbarProgress::new(false), | ||
}; | ||
assert_eq!( | ||
format.progress_status(1, 1, ""), | ||
|
@@ -528,6 +673,25 @@ fn test_progress_status_too_short() { | |
style: ProgressStyle::Percentage, | ||
max_print: 24, | ||
max_width: 24, | ||
taskbar: TaskbarProgress::new(false), | ||
}; | ||
assert_eq!(format.progress_status(1, 1, ""), None); | ||
} | ||
|
||
#[test] | ||
fn test_taskbar_disabled() { | ||
let taskbar = TaskbarProgress::new(false); | ||
let mut out = String::new(); | ||
out.push_str(&taskbar.remove().to_string()); | ||
out.push_str(&taskbar.value(10.0).to_string()); | ||
out.push_str(&taskbar.indeterminate().to_string()); | ||
assert!(out.is_empty()); | ||
} | ||
|
||
#[test] | ||
fn test_taskbar_error_state() { | ||
let mut progress = TaskbarProgress::new(true); | ||
assert_eq!(progress.value(10.0), TaskbarValue::Value(10.0)); | ||
progress.error(); | ||
assert_eq!(progress.value(50.0), TaskbarValue::Error(50.0)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we feel too risky (like excessive notification on Kitty), should we roll this out only on nightly for a while?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It will take time for linux users to upgrade their terminals. And who knows what other bugs systemd will catch. I definitely want people to associate Rust ecosystem with quality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if this is needed. This feature is "auto" by default and the only places where it auto-enables are inside of terminals that are known to support this feature.
So the only way this would be a problem is if the user does
CARGO_TERM_PROGRESS_TASKBAR = true cargo ...
and that is by design so people can experiment with this feature and report back on support so we can expand it.When it comes to a terminal having bad behavior in an old version and good behavior in a new, we can sometimes do version detection, e.g. https://github.com/zkat/supports-hyperlinks/blob/a2cc083668eb47c046f32ae0bc89d5bbdb8398ef/src/lib.rs#L23-L28