codespace: progress indicator, stdlib logger#4555
Conversation
codespace: use progress indicator
output.Logger|
I think this is ready for 👀 - I plan to look at how we can possibility set the right spinner color tomorrow based on terminal bg. I think that's the only thing blocking from this shipping (apart from reviews). Feel free to pull down the PR and run a few commands, it "feels" snappier, I like the spinner. |
What's wrong with simply using the foreground color? Presumably the user chose it for its contrast against the background. |
Nothing, I am actually starting to think it's my terminal that's messed up.
🤔 |
|
I've updated the logic to make it |
pkg/iostreams/iostreams.go
Outdated
| // 11 means Braille. See https://github.com/briandowns/spinner#available-character-sets | ||
| sp := spinner.New(spinner.CharSets[11], 400*time.Millisecond, opts...) | ||
|
|
||
| if s.TerminalTheme() == "light" { |
There was a problem hiding this comment.
I can't really guess what this does: a "terminal theme" is not a concept in the ANSI terminal spec; it appears to be a heuristic inferred from a handful of nonstandard environment variables such as COLORTERM, NO_COLOR, CLICOLOR, and CLICOLOR_FORCE.
I would hope that saying nothing at all about color here would yield a sensible behavior. Indeed, it does for me, when I run this PR without this line: I see gray braille animated against a white background.
There was a problem hiding this comment.
It has to be my setup then 😭 - happy to remove this. I think @mislav was going to take a look.
- Use "cyan" instead of "white" color which is barely legible on light terminal backgrounds - Use 120ms instead of 400ms rendering delay so that the spinner feels faster - Rename StartProgressIndicatorWithSuffix to StartProgressIndicatorWithLabel to be agnostic to presentation logic - Enable changing the label of the currently active progress indicator - Make Start/StopProgressIndicator methods parallelism-safe
I've merged the progress indicator into this PR. I think it is ready for review. The major TODO outstanding is figuring out the spinner color.
OP:
NewAppto accept aniostreams.IOStreamspointer instead of anoutput.LoggerAppnow creates an error logger, and checks for interactivity (TTY).as a progress indicator) - Ideally this would be done with the spinner, I have another PR that does this but trying to keep the diffs reasonable. In order to support a dotter-like UI, we are using*App.Printand*App.Printlnto do so which noops in non-interactive environments, matching the existing behavior.*log.Loggerfor app printing, because*log.Loggerautomatically appends a\nto all logs, so it is impossible to use the logger to print to the same line, once we switch to a progress indicator, we can switch to a logger like this since we don't need same-line printing.This PR should essentially retain the exact same behavior we have today except without
*output.Logger