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

WIP: Colors: Add ansi_default and make ansi_* colors actually output their ansi sequences #1460

Closed
wants to merge 10 commits into from

Conversation

earboxer
Copy link

@earboxer earboxer commented Dec 31, 2022

Please review the following checklist.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

@willmcgugan
Copy link
Collaborator

We don't intend to support the 16 ansi colors, because they make it impossible to guarantee a consistent UI on all platforms. If you rely on a user's selection, them some color combinations will always look bad for some users.

@earboxer
Copy link
Author

earboxer commented Dec 31, 2022

I mostly just want to support a default background color.

I enjoy terminal-based applications because I use a semi-transparent terminal window and I like to see my desktop background. The textual application I'm developing has no big use for color.

If we don't go ahead with this, maybe we could instead use a environment variable to disable colors (or to downgrade to ANSI colors), like Textualize/rich#2449 ?

I've always been a guy to customize my terminal's color-scheme, so using the ANSI colors seemed like a natural way to give more consistency to the users, as their terminal emulator has configured the colors already to a palette they're used to.

Application developers who want conformity in their app's appearance could just use the non ansi_ colors; this patch just empowers developers to give some of the themability to the users. I think the target audience for my application (people like myself) would appreciate the customizability of user-configurable terminal colors.

Comment on lines +163 to +166
if rich_color.type is ColorType.STANDARD:
return cls(r, g, b, math.nan)
elif rich_color.type is ColorType.DEFAULT:
return cls(1, 1, 1, math.nan)
Copy link
Author

Choose a reason for hiding this comment

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

(I think this might be the controversial change from this PR, as a lot of rich color names are made into ansi colors)

@willmcgugan
Copy link
Collaborator

If we don't go ahead with this, maybe we could instead use a environment variable to disable colors (or to downgrade to ANSI colors), like Textualize/rich#2449 ?

Textual supports all of the env vars that Rich does.

Supporting ansi colors isn't going to happen, sorry. It makes it impossible to guarantee an app will have a usable color scheme on all platforms. It might make some users happy, but we will be flooded with issues that users can't see cyan on black. Or that yellow looks too much like green. I know this from 3 years of Rich issues.

If we made it optional it would be a performance hit, and we would get issues that ansi colors don't work with opacity and alpha blending.

The Textual color system will eventually be themeable, which means we can guarantee it will look good on all platforms. You will also be able to match it with your ansi color theme, and still have opacity and alpha blending, plus multiple shades.

I understand you like your ansi theme, and have carefully picked one to your liking. I do to. But ansi colors are predicated on 1970s VDU technology. Supporting them would hold Textual back for little benefit.

@earboxer
Copy link
Author

earboxer commented Jan 2, 2023

It makes it impossible to guarantee an app will have a usable color scheme on all platforms.

This argument makes sense (or course barring the platforms which only support default + the 16 ansi colors like many framebuffer terminal emulators).

we would get issues that ansi colors don't work with opacity and alpha blending.

Yeah, looking at the past issues, that's probably true (even if we tried to make it clear in the documentation that ansi_ colors don't blend, but round to the closest color).

You will also be able to match it with your ansi color theme, and still have opacity and alpha blending, plus multiple shades.

Yeah, personally I guess I don't care about the ansi colors for my use-case, but I do care about ansi_default:

Here's my (rough, WIP) app, both with the default background color, and with ansi_default background color (from this PR):
2023-01-02T10:36:18,755713610-05:00
2023-01-02T10:36:04,745206305-05:00

A couple things to note:

  • my beautiful desktop background is visible through the terminal window (this makes me happy :)
  • the background color of Input "rounds" to ansi_black because it's trying to lighten ansi_default. (If I took out the other ansi colors, then it would round to a slightly lighter black, so might look better).
  • The left-side border of Input is inverted, and ansi_default as a foreground colors looks different (white) then when used as a background color (actually transparent in my terminal). (We could probably fix this by changing our borders to not be inverted).

Would a separate PR implementing just ansi_default be considered (assuming those two points are taken care of)?

@earboxer
Copy link
Author

earboxer commented Jan 3, 2023

Scrollbars also need to be changed to not be inverted.

It would be really great to use https://en.wikipedia.org/wiki/Symbols_for_Legacy_Computing, as they provide the "right" and "upper" blocks that Block Elements leave out, but they look bad, at least in the fonts that I have :(

@earboxer
Copy link
Author

earboxer commented Jan 7, 2023

"Symbols for Legacy Computing" is a relatively new block (Unicode 13.0 in 2020), so it will be a while until they catch on with fonts. (It was clearly designed to fill out the missing "Block Elements"

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.

2 participants