-
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?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
@rustbot author |
Hello, Ed, thank you for the review and sorry for the late feedback. It's been a lot of work lately. |
Understandable! |
// 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. |
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.
From #14615 (comment)
This also raises another small issue: if user stops cargo with ctrl+c, taskbar progress is not cleared and it stays until the next invocation of a program with taskbar progress report capability. Microsoft's own winget behaves like this.
I've tried to set ctrl+c handler in cargo and clear taskbar from there. It works, but it 50-100 LOC which would be used rarely. What do you think of this?
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 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 comment
The 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 src/bin/cargo/main.rs
?
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.
@ehuss I'd be interested in your opinion on this.
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.
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:
- Can we use external crate like https://crates.io/crates/ctrlc? I assume your policy is to use as few external crates as possible
- Will it iterfere with https://github.com/rust-lang/cargo/blob/master/crates/cargo-util/src/process_builder.rs#L259-L276? I don't fully understand when this code is used.
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.
It is used when cargo executes binaries for cargo run
or cargo <plugin>
. Not familiar with Windows but I would assume for these execvp cases we are not expected to see them returning.
5ff7484
to
584ee00
Compare
This is looking really great and making me jealous that my terminal doesn't support it! |
10e457d
to
c6d0d63
Compare
c6d0d63
to
aaad896
Compare
Happy to hear! I've cleaned up the code a little it the latest commit. I think it looks like it's finished. |
30233e5
to
371087c
Compare
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 comment
The 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 comment
The 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:
@rustbot review |
Hello, @epage and everyone interested. Hope, you're well! I have a good news! The progress in standartisation of the sequence is going well. Soon it's gonna be supported by systemd and GNOME vte. Here is the message from Christian Persch:
And, cosequently support to Ptyxis was added. From Lennart Poettering:
And the bug in kitty about the conflict with their OSC 9 implementation was fixed here. Also in foot. So, to sum it up, Ed, it looks like you will get the support in your terminal earlier than you anticipated. |
Thank you for driving this! I use Kitty so may gain nothing, but still a great feature! |
// 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. |
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.
It is used when cargo executes binaries for cargo run
or cargo <plugin>
. Not familiar with Windows but I would assume for these execvp cases we are not expected to see them returning.
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
371087c
to
0495692
Compare
0495692
to
ca4c121
Compare
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()) | ||
}); |
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.
Can we break the detection out into a function like we do for hyperlinks?
(and this would be allowed to use std::env::var_os
, like hyperlinks)
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see what I can do.
Ideally I see us breaking this out into a crate as some point
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.
What does this PR try to resolve?
A few terminal emulators support progress output to Windows taskbar.
winget
uses this to show install progress.Notably, Windows Terminal recently (2020) added support for ANSI codes specified in ConEmu (another terminal emulator for Windows) documentation. Also, in "Learn Windows".
I've found the previous attempt to add this feature: #11436
As per @weihanglo's request, I've added the config option to enable/disable this feature. It's enabled on supported terminal emulators.
Fixes #11432
How should we test and review this PR?
Run
cargo build
in Windows Terminal with configuration optionterm.progress.taskbar
set totrue
.Not sure
#[cfg(windows)]
? Probably no, because the feature is also usable in WSL.winget
is also behaves like alike. I've experimented withctrl_c handler
and it's totally fixable.Enabled
is a sensible default for WSL because it works on linux builds in Windows Terminal tooTLDR
term.progress.taskbar
option with bool type is addedVideos
Windows.PowerShell.2024-09-29.23-21-21.mp4
cmd.2024-09-29.23-36-25.mp4