Skip to content

tui: wait longer for color query results #5004

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −3
Open

tui: wait longer for color query results #5004

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions codex-rs/tui/src/terminal_palette.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ mod imp {

pub(super) fn requery_default_colors() {
if let Ok(mut cache) = default_colors_cache().lock() {
// Don't try to refresh if the cache is already attempted and failed.
if cache.attempted && cache.value.is_none() {
return;
}
Comment on lines +99 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does failing once on this suggest that this query will always fail, or are transient errors expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not quite sure what the best behavior is here, but I want to avoid a situation where we're on a terminal that doesn't support this query and we constantly re-issue the query and wait for the full timeout. Perhaps if our first query succeeds we always re-query, otherwise we never re-query?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it's pretty likely that we need to do this only on startup to avoid spamming the terminal like in #4945

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we definitely need to do it after startup sometimes, because the terminal theme can change (some themes follow light/dark mode, for example)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem with doing it outside of the normal terminal events flow is that it has some overlap / race problems with crossterm's (possibly async) event code. Maybe this functionality might be worth building into crossterm as another event type (for various OSC query results).

cache.refresh_with(|| query_default_colors().unwrap_or_default());
}
}
Expand All @@ -114,8 +118,6 @@ mod imp {
if !stdout_handle.is_terminal() {
return Ok(None);
}
stdout_handle.write_all(b"\x1b]10;?\x07\x1b]11;?\x07")?;
stdout_handle.flush()?;

let mut tty = match OpenOptions::new().read(true).open("/dev/tty") {
Ok(file) => file,
Expand All @@ -130,7 +132,10 @@ mod imp {
}
}

let deadline = Instant::now() + Duration::from_millis(200);
stdout_handle.write_all(b"\x1b]10;?\x07\x1b]11;?\x07")?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

non blocking nit: might be nice to use a few self documenting constants here to make it obvious what this is doing

Suggested change
stdout_handle.write_all(b"\x1b]10;?\x07\x1b]11;?\x07")?;
const OSC = "\x1b]";
const VT_100_FG = 10;
...
write!(stdout_handle, "{OSC}{VT_100_FG};..."

stdout_handle.flush()?;

let mut deadline = Instant::now() + Duration::from_millis(200);
let mut buffer = Vec::new();
let mut fg = None;
let mut bg = None;
Expand All @@ -140,6 +145,7 @@ mod imp {
match tty.read(&mut chunk) {
Ok(0) => break,
Ok(n) => {
deadline = Instant::now() + Duration::from_millis(200);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cause this code to block forever if there's continuous keys being pressed or other input happening, which might have a low likelihood edge case for some keyboard modes. It's likely one of those things that can be dealt with when it happens, but it might be worth capping the deadline at a total duration to avoid having to touch this again.

buffer.extend_from_slice(&chunk[..n]);
if fg.is_none() {
fg = parse_osc_color(&buffer, 10);
Expand Down
Loading