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

Suggestion: Make Stack.PrettyPrint's styles background agnostic #4183

Closed
mpilgrem opened this issue Jul 28, 2018 · 7 comments
Closed

Suggestion: Make Stack.PrettyPrint's styles background agnostic #4183

mpilgrem opened this issue Jul 28, 2018 · 7 comments

Comments

@mpilgrem
Copy link
Member

Should the styles set in module Stack.PrettyPrint force the background to be black ('dull' black)? For example (line 181) styleFile = bold . white . ondullblack. Beauty is in the eye of the beholder, but it seems to me that, for people using terminal themes with coloured backgrounds, the result is not pretty. For example, on a Mac Terminal with theme 'Novel':

image

I suggest it would be better to be agnostic as to background and have (for example): styleFile = bold . white.

This is on stack --version Version 1.8.0, Git revision 94d302c40d3d08dcaa0213f3f1ffdc1384842714 (6077 commits) x86_64 hpack-0.29.0 (given #4139).

mihaimaruseac added a commit that referenced this issue Jul 28, 2018
Fix #4183 Make PrettyPrint styles background-agnostic
mihaimaruseac added a commit that referenced this issue Jul 28, 2018
@mihaimaruseac
Copy link
Contributor

The problem is that text becomes harder to read on terminals with white background. So, unless we can find a solution which detects the background before determining what color to use, we will have to also set background color.

@mgsloan
Copy link
Contributor

mgsloan commented Jul 28, 2018

I agree that it's not pretty. Indeed it was a solution to text not being visible on light backgrounds - #4047 . An alternative may be to remove the background color and pick foreground colors that terminals are likely to display well.

@mpilgrem
Copy link
Member Author

This comment is also in response to the comments of @mihaimaruseac on the pull request #4184.

I acknowledge that if you do not force the background colour, there will always be some choice of terminal background colour that makes certain coloured foreground text 'invisible' - but perhaps stack does not need to accommodate all possible terminal background colours, only the most likely ones (including the extremes of dull black and vivid white)?

On a white (vivid white) terminal do you think it is only the 'bold vivid white' text on white (used by styleFile and styleUrl) that is problematic? If it is, perhaps a different foreground colour there would solve the problem.

For example 'vivid black' (black) or 'dull white' (which are both actually rendered as shades of grey on most terminals - see here) should have contrast on most terminal backgrounds.

This is 'vivid black' for styleFile on macOS Terminal's 'Basic' (white) theme:

image

This is 'vivid black' on Terminal's 'Novel' theme:

image

Alternatively (skipping shades of green and red as having special meaning or colours where the unused dull version is not particularly distinct from the vivid one), 'dull magenta' is currently not used by any style.

If that is an acceptable fix, I'll propose another pull request, accordingly.

mpilgrem added a commit to mpilgrem/stack that referenced this issue Jul 28, 2018
Also changes `styleFile` to `black` (vivid black) from `bold . white`.
@mihaimaruseac
Copy link
Contributor

I tend to agree with this.

@mgsloan
Copy link
Contributor

mgsloan commented Jul 29, 2018

Yep, I agree with removing the background colors that I added, and instead choosing different foreground colors. Glad you're adjusting these details!

mpilgrem added a commit to mpilgrem/stack that referenced this issue Jul 31, 2018
In order to ensure sufficient contrast on both black (dull black) and white (vivid white) terminals, also changes foreground text `bold . white` to `dullcyan` (affecting `styleFile` and `styleUrl`) and `yellow` to `dullyellow` (affecting `styleWarning` and `styleCurrent`).

`dullcyan` is used in preference to `black` (vivid black) because the latter is problematic with the solarised dark theme.

See also issue commercialhaskell#4047 and (reverted) commercialhaskell#4814.
@mpilgrem
Copy link
Member Author

mpilgrem commented Aug 4, 2018

I have done some further reading around and experimenting.

To avoid the reported 'vivid yellow' on 'vivid white' contrast problem (#4047), I suggest that 'dull yellow' substitute for 'vivid yellow'.

I do not use it myself, but I understand that 'solarized dark' is a very popular theme on a number of platforms and is, for example, accommodated by the (recent) Windows console colortool. 'vivid black' text does not work at all on solarized dark and, so, I now propose 'dull cyan' where I previously proposed 'vivid back'.

Here are some results:

The (new) Windows default console theme (known as 'campbell') (this is actually a screen shot of macOS Terminal.app; I have reproduced the Windows default there to ease experimentation):

image

The 'Basic' Terminal.app theme (vivid white):

image

The 'Solarized Dark' theme (note that stack's choice of 'vivid green' is not distinctive in this theme - however 'dull green' would be; I am going to propose a separate 'issue' to more fully address that sort of issue and the 'bigger picture' of colour choice for stack users):

image

Finally (for consistency with my earlier comments), the 'Novel' Terminal.app theme:

image

If this is acceptable, I will propose the corresponding pull request.

@mihaimaruseac
Copy link
Contributor

This looks good to me

mpilgrem added a commit to mpilgrem/stack that referenced this issue Aug 4, 2018
In order to ensure sufficient contrast on both black (dull black) and white (vivid white) terminals, also changes foreground text `bold . white` to `dullcyan` (affecting `styleFile` and `styleUrl`) and `yellow` to `dullyellow` (affecting `styleWarning` and `styleCurrent`).

`dullcyan` is used in preference to `black` (vivid black) because the latter is problematic with the solarised dark theme.

See also issue commercialhaskell#4047 and (reverted) commercialhaskell#4814.
mihaimaruseac added a commit that referenced this issue Aug 4, 2018
Fix #4183 Make PrettyPrint styles background-agnostic
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

No branches or pull requests

3 participants