-
Notifications
You must be signed in to change notification settings - Fork 7.2k
fix(tui): provide fallback background color for Windows terminals #8974
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?
fix(tui): provide fallback background color for Windows terminals #8974
Conversation
|
I have read the CLA Document and I hereby sign the CLA |
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2c17be6a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(DefaultColors { | ||
| fg: (255, 255, 255), // White foreground | ||
| bg: (0, 0, 0), // Black background (dark theme) |
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.
Avoid forcing black background on light-themed Windows
On Windows terminals configured with a light background (common when users switch to light themes), this hardcoded (0,0,0) fallback causes user_message_style_for to always apply a dark background while leaving the foreground at the terminal default (often dark text), producing low‑contrast/near‑invisible user input blocks. Previously returning None avoided setting any background in this case. Consider preserving None or making the fallback configurable/detectable so light‑theme terminals don’t end up with dark‑on‑dark text.
Useful? React with 👍 / 👎.
|
Thanks for the review feedback! Here's my analysis: Regarding light theme concernsThe review raises a valid point, but the current approach is still reasonable: How the code handles this:
Why dark fallback is acceptable:
Alternative considered:Making this configurable via env var or config would add complexity. The current solution fixes the majority case (dark theme users on Windows) which was previously completely broken. If there's demand, we could add a config option like |
|
Hey, I've not followed this development for long, but Windows Terminal has supported OSC 10/11 queries for some time, and I'm curious about why effort is being done to provide (potentially poor) defaults instead of providing proper support on Windows? Per the issue here: microsoft/terminal#3718 and the PR here: microsoft/terminal#17729 Support seems to be in at least the Windows Terminal, which is the default terminal experience in Windows 11. Perhapes there are some limitations that aren't clear from looking at the code, in that case please ignore my ignorant question. |
|
Hey, thanks for putting this PR together and for the thoughtful discussion around Windows terminal behavior. We really appreciate you digging into this. I would like to pause review on this for a bit because one of our teammates who has the most context on how Windows terminal color detection works is currently out on vacation. This change touches a pretty subtle area and we want to make sure we are not missing anything before deciding how or whether to move forward. There may be some issues with the current approach, so I do not want to give a premature approval until we have had a chance to look at it with the right context in the room. We will pick this back up once they are back. Thanks again for the PR and for your patience while we sort this out. |
It's okay, that's my opportunity to practice, so I'll wait for your feedback |
Fixes #8852.
Problem
On Windows, the TUI doesn't render shaded backgrounds for user input blocks because
terminal_palette::default_bg()always returnsNone.Root Cause
Windows terminals don't support ANSI color queries (OSC 10/11), so the Windows implementation was compiled to return
Noneinstead of querying terminal colors.Solution
Provide a fallback dark background color
(0, 0, 0)for Windows instead ofNone. Most modern Windows terminals (Windows Terminal, VS Code, Alacritty) default to dark themes, so this provides a reasonable default.Testing
cargo test -p codex-tui- all tests pass