haf / expecto

A smooth testing lib for F#. APIs made for humans! Strong testing methodologies for everyone!
Apache License 2.0
663 stars 96 forks source link

Save our eyes; colours must have contrast by default #315

Closed haf closed 4 years ago

haf commented 5 years ago

E.g. red on black has horrible contrast. https://www.viget.com/articles/color-contrast/

These are the de-facto colours now:

image
AnthonyLloyd commented 5 years ago

My reasoning for making 8 the default was that it was the middle ground between no color and 256. It's supported almost everywhere (not teamcity etc). Also 256 doesn't work in Travis, AppVeyor, VSCode and ConEmu (my terminal, it doesn't look good as currently set up). Not strongly fixed to this default though.

8 colours isn't a bug, we need to support it. Most people use a colour scheme to make red look ok.

It's more complicated than a right or wrong choice unfortunately. The 256 colour don't look correct in ConEmu in any scheme I've found, plus peoples scheme have to support other things that are 8 colours like fake etc.

Luckily you cover 256 and I cover 8 so we can at least make both look good where they tend to be used.

Default is tricky. There's an argument for making it 0 and people having to make and active choice for 8 vs 256 based on what they use.

jzabroski commented 5 years ago

It's important not to do an automatic color contrast algorithm. They don't work. It's best to pick a color palette that works.

haf commented 5 years ago

I consider it a bug that I can't read the text, like you saw in the screenshot. This is my own fault; I chose the red colour (I presume?) for 256 (you haven't changed it?)

AnthonyLloyd commented 5 years ago

I've not changed the 256 colours.

isatty is available as a PInvoke on Linux but not Windows as far as I know.

I don't know the modern libraries you list but fake, dotnet seem to use 8 colours.

I've never heard of ansicolor.dll.

We could go for 8 on Windows and 256 on Linux. Checking isatty if available and defaulting to zero in that case. This will mean Travis will still be incorrect as it doesn't support it.

haf commented 5 years ago

This will mean Travis will still be incorrect as it doesn't support it.

No, I don't think it will, since Travis doesn't say it's a TTY.

We could go for 8 on Windows and 256 on Linux. Checking isatty if available and defaulting to zero in that case.

Yes, this seems to be the Ultimate™ tradeoff! 🦋

AnthonyLloyd commented 5 years ago

No, I don't think it will, since Travis doesn't say it's a TTY.

But it supports 8 colours? Makes the story more complicated. May be best to just leave isatty out.

8 windows, 256 non-windows may at least flush out some more views.

haf commented 5 years ago

You're right; it seems it supports 8 colours and it reports to be a TTY to programs. It also seems to parse away cursor-move chars.

No, it's not best to leave isatty out IMO; it's like a guard; if it returns false, let's use no colours or fall back to the stuff I've written below. (For (another) reference: https://aweirdimagination.net/2015/02/21/256-color-terminals/)

This https://github.com/dtolnay/isatty/blob/master/src/lib.rs — suggests using getconsolemode on Windows; and the docs suggest checking the flag ENABLE_PROCESSED_OUTPUT = 0x0001

However, this is new; if the terminal supports 256 colours, you have this set:

image

So how about this?

let isatty fd =
  if Env.windows then Native.getConsoleMode() &&& 0x0001 > 0
  else Native.isatty()

let colours =
  if isatty stdout && (env "TERM").Contains "256color" then Colours 256
  elif isatty stdout then Colours 8
  else Colours 0

(Also Windows is a ghetto; why are you using it? 🧟‍♀️)

AnthonyLloyd commented 5 years ago

I guess my problem with getconsolemode is similar to isatty. I bet it indicates 256 but 256 looks washed out in ConEmu. It's really down to the colour scheme. In the end the docs are going to have to say if it looks rubbish try switching your colour scheme or switch the --colours.

(Also Windows is a ghetto; why are you using it? 🧟‍♀️)

Need to support windows stuff like WPF, Excel. Macs are expensive (relative) and too lock in for me. Plus I prefer Windows 10 UI. Easy Linux/Docker is tempting but would be a smell if really necessary.

haf commented 5 years ago

It's really down to the colour scheme.

We can do a good basic colour scheme in this lib and then it's up to every person how the terminal is configured to look.

I've implemented the IsWindows check for now, but I think my suggested code above is actually the best way to move forward with the colours-problem. It might also be an idea to remove the mutable static colours field and move to using DVars, so that if Expecto noticies itself being used inside the .net core test running (for example), it can disable progress bars and the like. What do you think?

AnthonyLloyd commented 5 years ago

This isn't the normal default colour. It's just if you use Expect without the run functions.

haf commented 5 years ago

Yes I know; like in .fsx scripts.