Wilfred / difftastic

a structural diff that understands syntax 🟥🟩
https://difftastic.wilfred.me.uk/
MIT License
20.69k stars 339 forks source link

In the solarize color schema environment, bright_green becomes achromatic. #611

Open 9enki opened 9 months ago

9enki commented 9 months ago

(1) A description of the issue. A screenshot is often helpful too.

I am using difftastic on windows terminal. Attached below are the high light results when the colorscheme is campbell and when it is solarized dark. When I use solarized dark, the parts that are supposed to be green become achromatic.

campbell image

solarized dark image

I changed this line to bright_black bright_yellow bright_blue bright_magenta bright_purple bright_purple bright_cyan bright_white and verified the results. The result shows that not only bright_green but also bright_yellow bright_blue bright_cyan are achromatic.

image

The workaround is to set bright_green to green so it is not achromatic. image

Is there a smarter solution?

(3) The version of difftastic you're using (see difft --version) and your operating system.

image

9enki commented 9 months ago

I have one more question.

Even if bright_green is changed to green as described above, the case as in the attached image is partially achromatic.

campbell: image

solarized dark image

Please let me know if there is a workaround for this as well.

9enki commented 9 months ago

The second issue was avoided by eliminating bold() as shown below. However, if there is a better solution, I would like to know.

image

diff --git a/src/display/style.rs b/src/display/style.rs
index a7f7a3010..7be7c9fe7 100644
--- a/src/display/style.rs
+++ b/src/display/style.rs
@@ -300,8 +300,8 @@ fn style_lines(lines: &[&str], styles: &[(SingleLineSpan, Style)]) -> Vec<String
 pub(crate) fn novel_style(style: Style, side: Side, background: BackgroundColor) -> Style {
     if background.is_dark() {
         match side {
-            Side::Left => style.bright_red(),
-            Side::Right => style.bright_green(),
+            Side::Left => style.red(),
+            Side::Right => style.green(),
         }
     } else {
         match side {
@@ -337,13 +337,13 @@ pub(crate) fn color_positions(
                             AtomKind::Comment => {
                                 style = style.italic();
                                 style = if background.is_dark() {
-                                    style.bright_blue()
+                                    style.blue()
                                 } else {
                                     style.blue()
                                 };
                             }
                             AtomKind::Keyword | AtomKind::Type => {
-                                style = style.bold();
+                                style = style;
                             }
                             AtomKind::TreeSitterError => style = style.purple(),
                             AtomKind::Normal => {}
@@ -361,14 +361,14 @@ pub(crate) fn color_positions(
                             | TokenKind::Atom(AtomKind::Type)
                     )
                 {
-                    style = style.bold();
+                    style = style;
                 }
                 if matches!(highlight, TokenKind::Atom(AtomKind::Comment)) {
                     style = style.italic();
                 }
             }
             MatchKind::NovelWord { highlight } => {
-                style = novel_style(style, side, background).bold();
+                style = novel_style(style, side, background);

                 // Underline novel words inside comments in code, but
                 // don't apply it to every single line in plaintext.
@@ -415,11 +415,10 @@ fn apply_header_color(
         if hunk_num != 1 {
             s.to_string()
         } else if background.is_dark() {
-            s.bright_yellow().to_string()
+            s.yellow().to_string()
         } else {
             s.yellow().to_string()
         }
-        .bold()
         .to_string()
     } else {
         s.to_string()
@@ -430,11 +429,10 @@ fn apply_header_color(
 pub(crate) fn print_warning(s: &str, display_options: &DisplayOptions) {
     let prefix = if display_options.use_color {
         if display_options.background_color.is_dark() {
-            "warning: ".bright_yellow().to_string()
+            "warning: ".yellow().to_string()
         } else {
             "warning: ".yellow().to_string()
         }
-        .bold()
         .to_string()
     } else {
         "warning: ".to_string()
@@ -459,7 +457,7 @@ pub(crate) fn apply_line_number_color(
             // For changed lines, show the line number as red/green
             // and bold. This works well for syntactic diffs, where
             // most content is not bold.
-            style = novel_style(style, side, display_options.background_color).bold();
+            style = novel_style(style, side, display_options.background_color);
         } else {
             // For unchanged lines, dim the line numbers so it's
             // clearly separate from the content.
Wilfred commented 9 months ago

It looks like your theme doesn't support bright or bold ANSI colour codes. Could you use a different theme?

jez commented 7 months ago

tl;dr: Solarized users may wish to pass --background=light to difftastic, even if they are using Solarized Dark.


@Wilfred The Solarized color scheme and many other color schemes use the bright variants of colors not for colors, but for various shades of white/black/gray.

In themes like Solarized, base16 themes, and others, bright colors actually hold monochrome colors meant for styling UI elements, like plain text or dividing lines, etc. For example:

image

In this Solarized screenshot, notice how the "normal" text like fname, text for comments, the background of the line with the cursor, the line numbers, the background of the line numbers, and the keywords like class and def are all styled with various monochrome colors, instead of either the default foreground/background color or some vibrant color. This is made possible by having the terminal color scheme store those monochrome colors in the bright variants of the normal 8 terminal colors.


I notice that there is a --background=light flag that nominally is to provide support for people using difftastic on light color schemes, but the current implementation of this flag seems to be "prefer non-bright colors instead of bright colors." This is exactly the behavior that Solarized or base16 color schemes need for them to display correctly.

After discovering this flag, I'm unblocked from being able to use difftastic with Solarized. I figured I'd post so that @9enki and whoever else also might discover this option.


The only other feature request I could have here would be "more customization options around colors." This would be useful independently of whether a given user uses Solarized. For example, people with Red/Green colorblindness tend to use colors like blue and orange for rendering diffs. Having a configuration option for the various pieces of UI that difftastic uses would be useful. For example, git has configuration options allowing for fine-grained customization of the colors of various UI elements in diffs. From git-config(1):

       color.diff.<slot>
           Use customized color for diff colorization.  <slot> specifies which
           part of the patch to use the specified color, and is one of context
           (context text - plain is a historical synonym), meta
           (metainformation), frag (hunk header), func (function in hunk
           header), old (removed lines), new (added lines), commit (commit
           headers), whitespace (highlighting whitespace errors), oldMoved
           (deleted lines), newMoved (added lines), oldMovedDimmed,
           oldMovedAlternative, oldMovedAlternativeDimmed, newMovedDimmed,
           newMovedAlternative newMovedAlternativeDimmed (See the <mode> setting
           of --color-moved in git-diff(1) for details), contextDimmed,
           oldDimmed, newDimmed, contextBold, oldBold, and newBold (see git-
           range-diff(1) for details).

...

       color
           The value for a variable that takes a color is a list of colors (at
           most two, one for foreground and one for background) and attributes
           (as many as you want), separated by spaces.

           The basic colors accepted are normal, black, red, green, yellow,
           blue, magenta, cyan, white and default. The first color given is the
           foreground; the second is the background. All the basic colors except
           normal and default have a bright variant that can be specified by
           prefixing the color with bright, like brightred.

           The color normal makes no change to the color. It is the same as an
           empty string, but can be used as the foreground color when specifying
           a background color alone (for example, "normal red").

           The color default explicitly resets the color to the terminal
           default, for example to specify a cleared background. Although it
           varies between terminals, this is usually not the same as setting to
           "white black".

           Colors may also be given as numbers between 0 and 255; these use ANSI
           256-color mode (but note that not all terminals may support this). If
           your terminal supports it, you may also specify 24-bit RGB values as
           hex, like #ff0ab3.

           The accepted attributes are bold, dim, ul, blink, reverse, italic,
           and strike (for crossed-out or "strikethrough" letters). The position
           of any attributes with respect to the colors (before, after, or in
           between), doesn’t matter. Specific attributes may be turned off by
           prefixing them with no or no- (e.g., noreverse, no-ul, etc).

           The pseudo-attribute reset resets all colors and attributes before
           applying the specified coloring. For example, reset green will result
           in a green foreground and default background without any active
           attributes.

           An empty color string produces no color effect at all. This can be
           used to avoid coloring specific elements without disabling color
           entirely.

           For git’s pre-defined color slots, the attributes are meant to be
           reset at the beginning of each item in the colored output. So setting
           color.decorate.branch to black will paint that branch name in a plain
           black, even if the previous thing on the same output line (e.g.
           opening parenthesis before the list of branch names in log --decorate
           output) is set to be painted with bold or some other attribute.
           However, custom log formats may do more complicated and layered
           coloring, and the negated forms may be useful there.