-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Implement dynamic 8-bit color palettes #19883
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,6 +213,8 @@ xlocnum | |
| xloctime | ||
| XMax | ||
| xmemory | ||
| XMVECTOR | ||
| XMVECTORF | ||
| XParse | ||
| xpath | ||
| xstddef | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,7 @@ codepages | |
| coinit | ||
| colorizing | ||
| COLORONCOLOR | ||
| colorref | ||
| COLORREFs | ||
| colorschemes | ||
| colorspec | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,6 +141,7 @@ Author(s): | |
| X(IMediaResource, BackgroundImagePath, "backgroundImage", implementation::MediaResource::Empty()) \ | ||
| X(Model::IntenseStyle, IntenseTextStyle, "intenseTextStyle", Model::IntenseStyle::Bright) \ | ||
| X(Core::AdjustTextMode, AdjustIndistinguishableColors, "adjustIndistinguishableColors", Core::AdjustTextMode::Automatic) \ | ||
| X(bool, GeneratePalette, "generatePalette", true) \ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the default indeed be true?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, yes. For the vast majority of users, I expect the downside to be low. More importantly, I have thought about ways to make this setting more discoverable. We could put it onto the compatibility page as well, given that it affects standard color rendering.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is a fair chance that this is going to break some applications. These colors were always intended to be absolute values, and now we're replacing them with colors that could be completely different, potentially resulting in color combinations that are unreadable. If that happens you're going to have some very angry users.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do like, however, that this "fixes" the behavior when using a light theme. It also improves the stylistic look when using themes that aren't traditional (e.g. with a non-black background color). While I believe that this would be most useful if enabled by default, it would certainly still be fine if we disable it instead.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would have thought the most common problem with light themes came from apps that are just using the 8 basic ANSI colors (when it's assumed that white is going to be visible on the default background color, and the default foreground color will be visible on black), and this mode doesn't help with that. That said, I'm assuming there must be some apps that would benefit from it, and I'm sure some people will love it, but I'd be very wary of forcing it on everyone and expecting them to opt out if they don't like it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @j4james The default palette interpolates from black to white. As a result, using color 231 (white) on a white background is unreadable. The generated palette instead interpolates from background to foreground. So for a light theme, color 231 is dark and readable against the background. Whether to set this as default is a difficult choice. I believe that quicker and wider adoption would lead program maintainers to consider the palette a viable choice and actually use it, making all the benefits a reality. Though breaking changes are a big deal so I understand either way.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was talking about apps using the basic ANSI colors though. ANSI white 7 (or the bright white 15) is still going to be unreadable on a light background. And apps that try to detect light color schemes, and then compensate by using dark colors from the 256-color palette, could actually end up being worse off with this mode (they might decide to use 16 or 17 in place of 231 when the color scheme is light, which would assumedly now have the opposite effect than what was intended). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh. I think ansi 7 for light themes should be dark. Like 15 should not mean bright white but instead intense/high contrast. I understand this may not be the case for some base16 themes and will result in problems trying to use them for programs created with dark themes in mind. Though this will be an issue regardless.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Just looking at the color schemes here, that's not typical for a light color scheme. Regardless of whether the scheme is light or dark, ANSI black is typically black (or at least a dark color), and ANSI white is typically white/gray (or at least a lighter color). There are obviously a lot of weird color schemes that do there own thing, but I thought it was generally expected that ANSI colors match their names, at least to some extent. |
||
| X(bool, UseAcrylic, "useAcrylic", false) | ||
|
|
||
| // Intentionally omitted Appearance settings: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated cleanup.