-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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.
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?
Conversation
// Don't try to refresh if the cache is already attempted and failed. | ||
if cache.attempted && cache.value.is_none() { | ||
return; | ||
} |
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.
Does failing once on this suggest that this query will always fail, or are transient errors expected?
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.
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?
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.
Yeah, it's pretty likely that we need to do this only on startup to avoid spamming the terminal like in #4945
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.
we definitely need to do it after startup sometimes, because the terminal theme can change (some themes follow light/dark mode, for example)
} | ||
|
||
let deadline = Instant::now() + Duration::from_millis(200); | ||
stdout_handle.write_all(b"\x1b]10;?\x07\x1b]11;?\x07")?; |
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.
non blocking nit: might be nice to use a few self documenting constants here to make it obvious what this is doing
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};..." |
match tty.read(&mut chunk) { | ||
Ok(0) => break, | ||
Ok(n) => { | ||
deadline = Instant::now() + Duration::from_millis(200); |
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.
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.
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.
It's probably also a good idea to log that we hit the timeout on this to be able to diagnose failure conditions around this code.
this bumps the timeout when reading the responses to OSC 10/11 so that we're less likely to pass the deadline halfway through reading the response.