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

Use 4-bit ANSI codes for base16 theme #934

Merged
merged 1 commit into from
Apr 22, 2020
Merged

Use 4-bit ANSI codes for base16 theme #934

merged 1 commit into from
Apr 22, 2020

Conversation

sharkdp
Copy link
Owner

@sharkdp sharkdp commented Apr 22, 2020

This PR changes the behavior for the base16 theme.

So far, we would print ANSI escape codes like

\x1b[38;5;__m

which require a terminal with 256 color support.

With this change, we use "4-bit" codes

\x1b[__m

like \x1b[31m for "red".

There is no change for the ansi-dark and ansi-light themes.

Unfortunately, ansi_term does not support "bright" versions of these colors (see table here). We currently fall back to the non-bright versions.

closes #865

@sharkdp
Copy link
Owner Author

sharkdp commented Apr 22, 2020

@mk12 FYI

@sharkdp sharkdp merged commit 319ab77 into master Apr 22, 2020
@sharkdp sharkdp deleted the fix-865 branch April 22, 2020 13:04
@@ -3,8 +3,8 @@
<plist version="1.0">
<dict>
<!--
The colors in this theme are encoded as #RRGGBBAA where RR is an ANSI
palette number from 00 to 0f, and AA is the special value 00 to indicate
The colors in this theme are encoded as #RRGGBBAA where RR is an base16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharkdp I think this should remain "ANSI palette number." The base16 numbers are mapped onto the ANSI palette as follows by base16 terminal templates (e.g. iTerm2):

ANSI Base16
0 0
1 8
2 B
3 A
4 D
5 E
6 C
7 5
8 3
9 9
A 1
B 2
C 4
D 6
E F
F 7

The RR values in this file are from the first column, not the second.

@mk12
Copy link
Contributor

mk12 commented Apr 23, 2020

Unfortunately, ansi_term does not support "bright" versions of these colors (see table here). We currently fall back to the non-bright versions.

This makes the base16 theme use the wrong colors :/ For example I think line numbers will become green instead of dim grey because 0x0B (ANSI bright green, but in base16 it means dim grey) is going to 0x02 instead.

Could we change ansi_light & ansi_dark to use the 0f alpha encoding, and leave base16 with 00? They seem more appropriate for this since they don't use the bright colors.

@sharkdp
Copy link
Owner Author

sharkdp commented Apr 23, 2020

Thank you for reviewing this (and sorry if I accidentally broke some of your work 😨).

This makes the base16 theme use the wrong colors :/ For example I think line numbers will become green instead of dim grey because 0x0B (ANSI bright green, but in base16 it means dim grey) is going to 0x02 instead.

I'm not sure I can follow. Line numbers in bat are drawn with the gutterForeground color of the respective Sublime Text scheme. For the base 16 scheme, that would be:

<key>gutterForeground</key>
<string>#0800000f</string>

With my code change, this will be mapped according to Color::Black:

    } else if color.a == 0x0f {
        match color.r {
            0x00 => Color::Black,
            0x01 => Color::Red,
            0x02 => Color::Green,
            0x03 => Color::Yellow,
            0x04 => Color::Blue,
            0x05 => Color::Purple,
            0x06 => Color::Cyan,
            0x07 => Color::White,
            // TODO: the following should be high-intensity variants of
            // these colors ("bright black", "bright red", ...).
            0x08 => Color::Black,
            0x09 => Color::Red,
            0x0a => Color::Green,
            0x0b => Color::Yellow,
            0x0c => Color::Blue,
            0x0d => Color::Purple,
            0x0e => Color::Cyan,
            0x0f => Color::White,

On my terminal emulator base16 scheme, it looks like this:
image

@sharkdp
Copy link
Owner Author

sharkdp commented Apr 24, 2020

Let me know if you think this PR should be reverted.

@mk12
Copy link
Contributor

mk12 commented Apr 24, 2020

Your screenshot looks fine, but I’m not sure how it’s working — I would have expected Color::black to blend in with the background. I’ll try it out on the weekend.

And don’t worry, I know you weren’t intending to break anything :) If I find any colors that aren’t working, I can make a PR to do as I mentioned before (use this 0x0f trick on ansi_light and ansi_dark instead). Or maybe I’ll just try adding bright support to ansi_term if it’s not too hard. In principle I think it is better for ansi_light, ansi_dark, and base16 to all use the 4 bit codes.

mk12 added a commit to mk12/bat that referenced this pull request May 2, 2020
This changes the base16 theme back from #RRGGBB0f to #RRGGBB00,
reverting part of sharkdp#934. That PR used the 0f encoding to produce ANSI
escape sequences 30-37 and 40-47 rather than 38;5 and 48;5 which require
256-color support. Unfortunately, it resulted in base16 using the wrong
colors becuase ansi_term does not support the bright variants (90-97 and
100-107) so it simply mapped them to the non-bright colors.

This PR makes combines the 00 and 0f alpha encodings into 00, and makes
them use the Color enum for the first 8 colors and Fixed otherwise. This
means the ansi-light and ansi-dark themes will work on terminals without
256-color support, and base16 will render bright colors correctly.
@mk12
Copy link
Contributor

mk12 commented May 2, 2020

Yeah, this makes the line numbers unreadable for me (I've selected a few lines to show that they're actually there, just blended with the background):

Screen Shot 2020-05-02 at 1 37 37 PM

I have fixed this in #972 while preserving your fix to #865. Though users with that issue should be using ansi-light or ansi-dark, not base16. Base16 is really a specialized color setup for people who want all their applications to use a consistent palette, and is only incidentally similar to the number of colors that non-256-color terminals support.

@ches
Copy link

ches commented May 6, 2020

I can corroborate the breakage, it's my primary theme and I've pinned bat to 0.13 for now—came to report it when I saw @mk12's PR, thanks for the fix!

sharkdp pushed a commit that referenced this pull request May 11, 2020
This changes the base16 theme back from #RRGGBB0f to #RRGGBB00,
reverting part of #934. That PR used the 0f encoding to produce ANSI
escape sequences 30-37 and 40-47 rather than 38;5 and 48;5 which require
256-color support. Unfortunately, it resulted in base16 using the wrong
colors becuase ansi_term does not support the bright variants (90-97 and
100-107) so it simply mapped them to the non-bright colors.

This PR makes combines the 00 and 0f alpha encodings into 00, and makes
them use the Color enum for the first 8 colors and Fixed otherwise. This
means the ansi-light and ansi-dark themes will work on terminals without
256-color support, and base16 will render bright colors correctly.
@sharkdp
Copy link
Owner Author

sharkdp commented May 11, 2020

Fixed in v0.15.1

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.

no color for bat in ssh from a Windows client.
3 participants