Closed rivy closed 4 years ago
Thank you for looking into this.
I don't understand why we need a new parameter in config
for this. We can already switch off colors completely by setting --color-mode=off
or PASTEL_COLOR_MODE=off
.
Couldn't we just use the result from output_vt100::try_init().is_ok()
to automatically switch the mode to "off" if no mode has been specified (i.e. if the mode is set to "auto")?
I added it as a config
parameter because that seemed to be to be the right carrier for the information to the subcommands, each of which might react differently depending on available colorizable output (errors, etc). I wanted it to be a transparent and hands-off as possible for the user.
I can rethink it using global_matches.value_of("color-mode")
as the information carrier, and using PASTEL_COLOR_MODE=off
for Win7 machines with command line overrides via --color-mode=...
. That seems like more user-work though...
Hmm, maybe just defaulting it to false
for non-ANSI-color machines would work with no extra user intervention.
You would want to be able to output ANSI color, when requested, as those machines can display it via various mechanisms (eg, piping to less
).
I'll give an alternate implementation a look.
Ultimately, I'd love to just couple an ANSI to console API output filter, but I'm not sure how to do that yet.
What about the command responses? Errors, etc...
I can rethink it using
global_matches.value_of("color-mode")
as the information carrier, and usingPASTEL_COLOR_MODE=off
for Win7 machines with command line overrides via--color-mode=...
. That seems like more user-work though...
Well.. I wanted it to be set to "off" automatically if the mode is set to "auto" (which is the default).
Hmm, maybe just defaulting it to
false
for non-ANSI-color machines would work with no extra user intervention.
That's what I meant, yes.
You would want to be able to output ANSI color, when requested, as those machines can display it via various mechanisms (eg, piping to
less
).
Yes, that could be done via --color-mode=24bit
.
Ultimately, I'd love to just couple an ANSI to console API output filter, but I'm not sure how to do that yet.
I think ripgrep
uses a library which does colorization for older Windows machines in a way that is transparent for the developer.
But to be honest, I don't really want to add (a lot of) platform-dependend code to pastel
and I'd rather sacrifice colored-output support for older Windows versions. Windows 7 support ends January next year...
I've slimmed it down to a minimum working set. It works for Windows 7 without panics.
The PR is currently based on the distinct refactor for testing, but the real change set is small (see https://github.com/sharkdp/pastel/pull/91/files/aa929c44977d8218262b0e5dabd19dbefc6b858a..13064d983f08e3fc3e02fe6da9d0dfbbdebae675).
This is the minimal change, everything else stripped out.
Failing test without.
On Sun, 6 Oct 2019, 14:45 David Peter, notifications@github.com wrote:
@sharkdp commented on this pull request.
In src/cli/commands/format.rs https://github.com/sharkdp/pastel/pull/91#discussion_r331806776:
@@ -56,7 +56,7 @@ impl ColorCommand for FormatCommand { .paint(output, color.text_color().ansi_style().on(color)) )?; } else {
- write!(out.handle, "{}", output)?;
- writeln!(out.handle, "{}", output)?;
why has this been changed?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sharkdp/pastel/pull/91?email_source=notifications&email_token=AAATSBEGIFUJDNCTUFXEAGDQNI553A5CNFSM4IUDR7OKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCHAO4YI#pullrequestreview-297856609, or mute the thread https://github.com/notifications/unsubscribe-auth/AAATSBFETSXHKQR5KHMBA43QNI553ANCNFSM4IUDR7OA .
Failing test without.
how can this be? we don't even have a test case that triggers this else
branch.
See in-line comment ... the else is triggered (always, unless overridden by command-line flags) on non-colorizing/non-VT100 machines.
Thank you!
This is an initial version that compiles and works for Windows 7. I created a long note-to-self log message with reasoning and references (from [92de415c]) ...
I'd love to just implement 8-bit color, but, from discussion, it looks like that would involve using
termcolor
. And I'm to new to therust
landscape to tackle that directly. I'm going to have to play around with some toy versions first just to understand how to use that crate.So, barring that for the moment, this iteration does work for Windows 7 (and likely prior versions). There is one grumble...
main()
prints it's error messages surrounded with ANSI-color escapes. Since the color testing isn't done and added to the config structure until withinrun()
, it's unavailable tomain()
. If it needs to be fixed, the refactor is a bit large, and I haven't seen a good, understandable example of a CLI written in rust that handles color from themain()
. Any thoughts?