Skip to content
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

fix for non-color consoles #91

Merged
merged 1 commit into from
Oct 8, 2019
Merged

fix for non-color consoles #91

merged 1 commit into from
Oct 8, 2019

Conversation

rivy
Copy link
Contributor

@rivy rivy commented Sep 5, 2019

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 [92de415]) ...

change ~ allow operation despite non-color output

* customize function for platforms without color support
- return only non-colorized output for "format", "list", and "paint" subcommands
- disable "colorcheck" and "pick" commands with an error

.# Discussion

Versions of Windows prior to one of the v1511 Windows 10 builds (likely
10.0.10586.589; released on 2016-09-13) do not have built-in console
interpretation of ANSI escapes. After Win10 v1511, ANSI escape interpretation
is available, when enabled via `SetConsoleMode()` (see [4]). Earlier versions
(back through Windows 7 to XP) *do* have 8-bit color available via the Console
API, but it's more cumbersome to access (especially from the `rust` realm)[5].

Pre-ANSI escape color may be too cumbersome to use, but the majority of
`pastel` has usefulness even if not displayed in color. "colorcheck" and "pick"
are obvious sub-commands which are unusable without some color support and so
both are disabled with an error message. "format" and "list" are modified to
only display non-colorized output. "paint", a judgement call about error vs
un-colorized, is left operational but without colorized output.

[1] [MS ~ Release Blog Post](https://devblogs.microsoft.com/commandline/24-bit-color-in-the-windows-console) @@ <https://archive.is/6UTBA>
[2] [Reddit ~ Win 10 ANSI Escapes](https://www.reddit.com/r/Windows10/comments/44czox/windows_10_v1511_adds_support_for_ansi_escape) @@ <https://archive.is/7l6tz>
[3] [MS TechNet Win 10 Versions](https://docs.microsoft.com/en-us/windows/release-information/) @@ <https://archive.is/sv3LN>
[4] [`ansi-term` ~ enable ANSI](https://github.com/rivy/rust.ansi-term/blob/ff7eba98d55ad609c7fcc8c7bb0859b37c7545cc/src/windows.rs#L25-L57)
[5] [rust-users ~ Windows color](https://users.rust-lang.org/t/colored-terminal-output/24604/12) @@ <https://web.archive.org/web/20190905231230/https://users.rust-lang.org/t/colored-terminal-output/24604/12>

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 the rust 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 within run(), it's unavailable to main(). 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 the main(). Any thoughts?

@rivy rivy force-pushed the fix.non-color branch 2 times, most recently from 5bbc9dd to af67428 Compare September 6, 2019 03:05
@sharkdp
Copy link
Owner

sharkdp commented Sep 8, 2019

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")?

@rivy
Copy link
Contributor Author

rivy commented Sep 8, 2019

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...

@sharkdp
Copy link
Owner

sharkdp commented Sep 9, 2019

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...

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...

@rivy rivy force-pushed the fix.non-color branch 2 times, most recently from e453f71 to 6282d9c Compare September 19, 2019 02:02
@rivy
Copy link
Contributor Author

rivy commented Sep 19, 2019

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).

@rivy rivy changed the title WIP ~ fix for non-color consoles fix for non-color consoles Sep 19, 2019
…sole support

.# Discussion

Versions of Windows prior to one of the v1511 Windows 10 builds (likely
10.0.10586.589; released on 2016-09-13) do not have built-in console
interpretation of ANSI escapes. After Win10 v1511, ANSI escape interpretation
is available, when enabled via `SetConsoleMode()` (see [4]). Earlier versions
(back through Windows 7 to XP) *do* have 8-bit color available via the Console
API, but it's more cumbersome to access (especially from the `rust` realm)[5].

Pre-ANSI escape color may be too cumbersome to use, but the majority of
`pastel` has usefulness even if not displayed in color. "colorcheck" and "pick"
are obvious sub-commands which are unusable without some color support and so
both are disabled with an error message. "format" and "list" are modified to
only display non-colorized output. "paint", a judgement call about error vs
un-colorized, is left operational but without colorized output.

[1] [MS ~ Release Blog Post](https://devblogs.microsoft.com/commandline/24-bit-color-in-the-windows-console) @@ <https://archive.is/6UTBA>
[2] [Reddit ~ Win 10 ANSI Escapes](https://www.reddit.com/r/Windows10/comments/44czox/windows_10_v1511_adds_support_for_ansi_escape) @@ <https://archive.is/7l6tz>
[3] [MS TechNet Win 10 Versions](https://docs.microsoft.com/en-us/windows/release-information/) @@ <https://archive.is/sv3LN>
[4] [`ansi-term` ~ enable ANSI](https://github.com/rivy/rust.ansi-term/blob/ff7eba98d55ad609c7fcc8c7bb0859b37c7545cc/src/windows.rs#L25-L57)
[5] [rust-users ~ Windows color](https://users.rust-lang.org/t/colored-terminal-output/24604/12) @@ <https://web.archive.org/web/20190905231230/https://users.rust-lang.org/t/colored-terminal-output/24604/12>
@rivy
Copy link
Contributor Author

rivy commented Sep 30, 2019

This is the minimal change, everything else stripped out.

src/cli/commands/format.rs Outdated Show resolved Hide resolved
src/cli/cli.rs Show resolved Hide resolved
@rivy
Copy link
Contributor Author

rivy commented Oct 6, 2019 via email

@sharkdp
Copy link
Owner

sharkdp commented Oct 6, 2019

Failing test without.

how can this be? we don't even have a test case that triggers this else branch.

@rivy
Copy link
Contributor Author

rivy commented Oct 6, 2019

See in-line comment ... the else is triggered (always, unless overridden by command-line flags) on non-colorizing/non-VT100 machines.

@sharkdp sharkdp merged commit 57bea78 into sharkdp:master Oct 8, 2019
@sharkdp
Copy link
Owner

sharkdp commented Oct 8, 2019

Thank you!

@rivy rivy deleted the fix.non-color branch October 10, 2019 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants