refactor(terminal): cleanup deprecated flush logic #6373
Conversation
Removes flush logic that was leftover to test against ratatui's flush Plus a small amount of tidying up
codex-rs/tui/src/custom_terminal.rs
Outdated
| let previous_buffer = &self.buffers[1 - self.current]; | ||
| let current_buffer = &self.buffers[self.current]; |
There was a problem hiding this comment.
| let previous_buffer = &self.buffers[1 - self.current]; | |
| let current_buffer = &self.buffers[self.current]; | |
| let previous_buffer = self.previous_buffer_mut(); | |
| let current_buffer = self.current_buffer_mut(); |
This requires a clone due to the borrow checker, but It's likely that this is probably fine due to 10-20k cells being fast enough to not be too concerned about perf issues on most modern hardware.
|
@codex review this and check my logic about cloning the buffers being fine from a perf perspective. Use a worst case estimate of a Raspberry Pi 4 and a buffer size of around 50k cells (this represents my guesstimate for tiny text on a 27in 4k monitor). Ack and explain the ack if the estimates are off. |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Avoids cloning the buffers - only clone the changed cells
|
ChatGPT gave an estimate for a Raspberry Pi 4 (using this as a lowest threshold system). 60fps -> 60% of one core at worst case scenario (big 4k monitor, tiny font = 59k cells). So instead this clones and now the DrawCommand owns the cell instead of a ref in the diff. This means that it cuts that down to a much more reasonable level of only the changes (which are unlikely to be full screen 60fps, more like 100 characters per second for a normal change, and a couple of ms when doing a full screen refresh. |
|
|
||
| /// Gets the current buffer as a mutable reference. | ||
| pub fn current_buffer_mut(&mut self) -> &mut Buffer { | ||
| fn current_buffer_mut(&mut self) -> &mut Buffer { |
There was a problem hiding this comment.
No longer pub because there's nowhere that calls this external to the custom terminal.
Removes flush logic that was leftover to test against ratatui's flush
Cleaned up the flush logic so it's a bit more intent revealing.
DrawCommand now owns the Cells that it draws as this works around a borrow checker problem.