lambdaisland / deep-diff2

Deep diff Clojure data structures and pretty print the result
Eclipse Public License 1.0
296 stars 18 forks source link

Consider tweaking colors to draw eye to differences #14

Open lread opened 5 years ago

lread commented 5 years ago

Thanks so much for the kaocha, I am really enjoying using it!

Some of my integration tests include the comparison of largish data structures. It can take my eyeballs a while to find the diff between expected and actual in these data structures.

I do sometimes use the search feature in my terminal to search for + and - and that can help and I was thinking focusing only on the differences, as #13 suggests, would help.

But I am also thinking perhaps a different choice of colors might help. I like the choice of colors chosen for differences, but I find that the red can take a while to spot because it is already used in non-diff syntax highlighting.

My initial thoughts are:

  1. change the red to something else in default syntax highlighting, maybe blue.
  2. take advantage of background colors to really draw the eye to changes. We might even be able to keep the normal syntax highlighting foreground colors with this scheme.

I am happy to give some color schemes a whirl for review, then a PR with changes, if there is interest in this idea.

lread commented 5 years ago

Chatted on Slack with @plexus about this one. Summary:

  1. Go ahead with some mockups so we can get a feel for what this might look like
  2. Bear in mind that some people will not like the change, so in addition to any new coloring we'll still need to support the current coloring through some sort of config (analysis to come if we decide to proceed) - default would very likely be current coloring.
lread commented 5 years ago

Ok, I played around with some mockups.

Everybody likely has different terminal color schemes and fonts setup. I did my testing on macOS using iTerm2 using the "Roboto Mono for Powerline" font with the "Solarized Dark" color preset for for dark background testing and "Solarized Light" for light background testing. Note that the Solarized themes do compensate for yellow looking terrible on a light background.

README screenshot Here is the example presented on the deep-diff README... image It looks like whoever took this screenshot was using a similar terminal color scheme similar to the one I am using for my tests.

Option A) baseline If I run README example against current code, I get: image This closely matches the README screenshot.

On a light background: image

I won't explore it more unless requested, but if a terminal color scheme is chosen that does not compensate for yellow, you'll see something like this on a light background: image

Option B) Option A + blue delimiter characters image To me, this maybe a bit better, my eye is drawn to the red of the change.

Option C) Option A + background colors for insertions/deletions image Ok. no.

Option D) Option C + with black foreground for insertions/deletions image To me, this is great, I see my insertions and deletions clearly.

Option E) Option D + Option B And one last variation that combines option B and D image To me, Option E is slightly better than Option D as the distracting red is gone.

Let's see what this candidate looks like on a light background: image Looks good to me.

I will follow up with another message exploring how this looks with a larger diff.

lread commented 5 years ago

Where coloring changes really shine are for larger diffs. Let's compare the current highlighting of a larger diff to Option E highlighting.

Option A) larger diff image

Option E) larger diff image

plexus commented 5 years ago

I'd be curious if puget allows rgb colors, so we can come up with a sensible scheme that isn't dependent on people's terminal configuration.

lread commented 5 years ago

Good question, it looks like puget might support 8-bit colors via :fg-256 and :bg-256 https://github.com/greglook/puget/blob/d438931f35454cd2eb5afc10f458ae54dfcb4285/src/puget/color/ansi.clj#L28

I’ll try them out.

lread commented 5 years ago

Hiya @plexus, this git issue is more interesting than I originally thought it would be!

I am thinking ANSI RGB colors might not be a great idea for one solid reason and one subjective one.

solid reason: circleci does not seem render them I found a few scripts that test out terminal capabilities with regards to color.
Here's output from color-spaces.pl on macOS iTerm2: image And here's the output from the same script when viewed from circleci's web interface: image

subjective reason: 16-color ANSI option is themable Maybe it is not important for all people who use kaocha to see the very same colors. People choose color themes in their terminals to get consistent coloring across all 16-color ANSI terminal apps they use. Like you said red and green have specific meanings, but maybe I like my red to be rust and you like yours to be indigo. Consistent terminal colors with 16-ANSI-color Vim themes gives a good overview of the concept.

I think this also puts configuration of color to address any accessibility issues with vision in the right place.

options I see our options as:

  1. do nothing, solve through #13
  2. add an option to allow kaocha users to change the color palette to whatever puget compatible colors they like. This might include defining and selecting color themes.
  3. add an option to allow kaocha users to invert the color for differences only.

All I really need to address my issue is to have an option to invert colors for differences. Circleci does seem to render background colors just fine: image So I am leaning toward option 3, but I am looking forward to learning what you think.

magnars commented 5 years ago

I'll just add a little point here: Allowing configurability to this, will let color blind people (red/green for me) to use colors that are more noticably different for them.

lread commented 5 years ago

Thanks @magnars, your input is much appreciated.

I have been focusing on the terminal and its typical configurability with regards to color, (or lack thereof when presented by circleci), but there is also REPL output when used from an editor and maybe other scenarios I have not thought of.

plexus commented 5 years ago

I would still be a fan of 2., making it configurable. I also prefer rbg colors over the 16 terminal colors because they are absolute. We can set a foreground and background and be sure it renders the same everywhere. There is such a wide range of color schemes out there that whatever we do will be unreadable for some.

If it's configurable at the deep-diff level then kaocha can always detect that CI=true and switch to something more basic.

@magnars that is a really great point. It's very unfortunate that the two colors that are so commonly used culturally to denote some kind of contrast (red/green) are also the most common colors that colorblind people have trouble with. what other colors would you use to convey added/deleted that aren't so problematic?

magnars commented 5 years ago

Maybe green additions, and dark gray retractions?

ons. 11. sep. 2019 kl. 10:08 skrev Arne Brasseur notifications@github.com:

I would still be a fan of 2., making it configurable. I also prefer rbg colors over the 16 terminal colors because they are absolute. We can set a foreground and background and be sure it renders the same everywhere. There is such a wide range of color schemes out there that whatever we do will be unreadable for some.

If it's configurable at the deep-diff level then kaocha can always detect that CI=true and switch to something more basic.

@magnars https://github.com/magnars that is a really great point. It's very unfortunate that the two colors that are so commonly used culturally to denote some kind of contrast (red/green) are also the most common colors that colorblind people have trouble with. what other colors would you use to convey added/deleted that aren't so problematic?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lambdaisland/deep-diff/issues/14?email_source=notifications&email_token=AACA4OJRD5WSCZC73IYJZ3TQJCROLA5CNFSM4ISXS6N2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6NUW7Y#issuecomment-530271103, or mute the thread https://github.com/notifications/unsubscribe-auth/AACA4OJ7HVP4DX3TIA6C5JTQJCROLANCNFSM4ISXS6NQ .

lread commented 5 years ago

Thanks for the reply @plexus!

I still think there is strong merit to a 16 color palette being themeable.

That said, option 2 does not prohibit using the 16 color palette for those that see the benefit. I’ll proceed with testing out support for RGB colors with puget.

lread commented 5 years ago

I was poking around a bit with puget. I confirm that it does support 8-bit and 24-bit ANSI schemes via:fg-256 and :bg-256.

8-bit scheme is expressed via [:fg-256 5 n] where n is between 0 and 255. We can combine foreground and background, for example, like so: [:fg-256 5 226 :bg-256 5 56].

24-bit scheme is expressed via [:fg-256 2 r g b] where r g b are each between 0 and 255. Foreground and background can be combined, for example: [:fg-256 2 205 236 255 :bg-256 2 110 22 188].

These all show nicely in iTerm2 on macOS. They did not, however, show properly in a cider repl session in spacemacs. Caveats on 8-bit and 24-bit scheme support would be included kaocha docs.