Skip to content

Commit cf44511

Browse files
authored
refactor TUI event loop to enable dropping + recreating crossterm event stream (openai#7961)
Introduces an `EventBroker` between the crossterm `EventStream` source and the consumers in the TUI. This enables dropping + recreating the `crossterm_events` without invalidating the consumer. Dropping and recreating the crossterm event stream enables us to fully relinquish `stdin` while the app keeps running. If the stream is not dropped, it will continue to read from `stdin` even when it is not actively being polled, potentially stealing input from other processes. See [here](https://www.reddit.com/r/rust/comments/1f3o33u/myterious_crossterm_input_after_running_vim/?utm_source=chatgpt.com) and [here](https://ratatui.rs/recipes/apps/spawn-vim/) for details. ### Tests Added tests for new `EventBroker` setup, existing tests pass, tested locally.
1 parent bef36f4 commit cf44511

File tree

4 files changed

+545
-80
lines changed

4 files changed

+545
-80
lines changed

codex-rs/Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

codex-rs/tui/Cargo.toml

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ workspace = true
2323

2424
[dependencies]
2525
anyhow = { workspace = true }
26-
async-stream = { workspace = true }
2726
base64 = { workspace = true }
2827
chrono = { workspace = true, features = ["serde"] }
2928
clap = { workspace = true, features = ["derive"] }
@@ -81,7 +80,7 @@ tokio = { workspace = true, features = [
8180
"test-util",
8281
"time",
8382
] }
84-
tokio-stream = { workspace = true }
83+
tokio-stream = { workspace = true, features = ["sync"] }
8584
toml = { workspace = true }
8685
tracing = { workspace = true, features = ["log"] }
8786
tracing-appender = { workspace = true }

codex-rs/tui/src/tui.rs

Lines changed: 32 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ use crossterm::event::DisableBracketedPaste;
1616
use crossterm::event::DisableFocusChange;
1717
use crossterm::event::EnableBracketedPaste;
1818
use crossterm::event::EnableFocusChange;
19-
use crossterm::event::Event;
2019
use crossterm::event::KeyEvent;
2120
use crossterm::event::KeyboardEnhancementFlags;
2221
use crossterm::event::PopKeyboardEnhancementFlags;
@@ -32,7 +31,6 @@ use ratatui::crossterm::terminal::enable_raw_mode;
3231
use ratatui::layout::Offset;
3332
use ratatui::layout::Rect;
3433
use ratatui::text::Line;
35-
use tokio::select;
3634
use tokio::sync::broadcast;
3735
use tokio_stream::Stream;
3836

@@ -42,11 +40,12 @@ use crate::custom_terminal::Terminal as CustomTerminal;
4240
use crate::notifications::DesktopNotificationBackend;
4341
use crate::notifications::NotificationBackendKind;
4442
use crate::notifications::detect_backend;
45-
#[cfg(unix)]
46-
use crate::tui::job_control::SUSPEND_KEY;
43+
use crate::tui::event_stream::EventBroker;
44+
use crate::tui::event_stream::TuiEventStream;
4745
#[cfg(unix)]
4846
use crate::tui::job_control::SuspendContext;
4947

48+
mod event_stream;
5049
mod frame_requester;
5150
#[cfg(unix)]
5251
mod job_control;
@@ -156,7 +155,7 @@ fn set_panic_hook() {
156155
}));
157156
}
158157

159-
#[derive(Debug)]
158+
#[derive(Clone, Debug)]
160159
pub enum TuiEvent {
161160
Key(KeyEvent),
162161
Paste(String),
@@ -166,6 +165,7 @@ pub enum TuiEvent {
166165
pub struct Tui {
167166
frame_requester: FrameRequester,
168167
draw_tx: broadcast::Sender<()>,
168+
event_broker: Arc<EventBroker>,
169169
pub(crate) terminal: Terminal,
170170
pending_history_lines: Vec<Line<'static>>,
171171
alt_saved_viewport: Option<ratatui::layout::Rect>,
@@ -194,6 +194,7 @@ impl Tui {
194194
Self {
195195
frame_requester,
196196
draw_tx,
197+
event_broker: Arc::new(EventBroker::new()),
197198
terminal,
198199
pending_history_lines: vec![],
199200
alt_saved_viewport: None,
@@ -214,6 +215,18 @@ impl Tui {
214215
self.enhanced_keys_supported
215216
}
216217

218+
// todo(sayan) unused for now; intend to use to enable opening external editors
219+
#[allow(unused)]
220+
pub fn pause_events(&mut self) {
221+
self.event_broker.pause_events();
222+
}
223+
224+
// todo(sayan) unused for now; intend to use to enable opening external editors
225+
#[allow(unused)]
226+
pub fn resume_events(&mut self) {
227+
self.event_broker.resume_events();
228+
}
229+
217230
/// Emit a desktop notification now if the terminal is unfocused.
218231
/// Returns true if a notification was posted.
219232
pub fn notify(&mut self, message: impl AsRef<str>) -> bool {
@@ -262,79 +275,21 @@ impl Tui {
262275
}
263276

264277
pub fn event_stream(&self) -> Pin<Box<dyn Stream<Item = TuiEvent> + Send + 'static>> {
265-
use tokio_stream::StreamExt;
266-
267-
let mut crossterm_events = crossterm::event::EventStream::new();
268-
let mut draw_rx = self.draw_tx.subscribe();
269-
270-
// State for tracking how we should resume from ^Z suspend.
271-
#[cfg(unix)]
272-
let suspend_context = self.suspend_context.clone();
273278
#[cfg(unix)]
274-
let alt_screen_active = self.alt_screen_active.clone();
275-
276-
let terminal_focused = self.terminal_focused.clone();
277-
let event_stream = async_stream::stream! {
278-
loop {
279-
select! {
280-
event_result = crossterm_events.next() => {
281-
match event_result {
282-
Some(Ok(event)) => {
283-
match event {
284-
Event::Key(key_event) => {
285-
#[cfg(unix)]
286-
if SUSPEND_KEY.is_press(key_event) {
287-
let _ = suspend_context.suspend(&alt_screen_active);
288-
// We continue here after resume.
289-
yield TuiEvent::Draw;
290-
continue;
291-
}
292-
yield TuiEvent::Key(key_event);
293-
}
294-
Event::Resize(_, _) => {
295-
yield TuiEvent::Draw;
296-
}
297-
Event::Paste(pasted) => {
298-
yield TuiEvent::Paste(pasted);
299-
}
300-
Event::FocusGained => {
301-
terminal_focused.store(true, Ordering::Relaxed);
302-
crate::terminal_palette::requery_default_colors();
303-
yield TuiEvent::Draw;
304-
}
305-
Event::FocusLost => {
306-
terminal_focused.store(false, Ordering::Relaxed);
307-
}
308-
_ => {}
309-
}
310-
}
311-
Some(Err(_)) | None => {
312-
// Exit the loop in case of broken pipe as we will never
313-
// recover from it
314-
break;
315-
}
316-
}
317-
}
318-
result = draw_rx.recv() => {
319-
match result {
320-
Ok(_) => {
321-
yield TuiEvent::Draw;
322-
}
323-
Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => {
324-
// We dropped one or more draw notifications; coalesce to a single draw.
325-
yield TuiEvent::Draw;
326-
}
327-
Err(tokio::sync::broadcast::error::RecvError::Closed) => {
328-
// Sender dropped. This stream likely outlived its owning `Tui`;
329-
// exit to avoid spinning on a permanently-closed receiver.
330-
break;
331-
}
332-
}
333-
}
334-
}
335-
}
336-
};
337-
Box::pin(event_stream)
279+
let stream = TuiEventStream::new(
280+
self.event_broker.clone(),
281+
self.draw_tx.subscribe(),
282+
self.terminal_focused.clone(),
283+
self.suspend_context.clone(),
284+
self.alt_screen_active.clone(),
285+
);
286+
#[cfg(not(unix))]
287+
let stream = TuiEventStream::new(
288+
self.event_broker.clone(),
289+
self.draw_tx.subscribe(),
290+
self.terminal_focused.clone(),
291+
);
292+
Box::pin(stream)
338293
}
339294

340295
/// Enter alternate screen and expand the viewport to full terminal size, saving the current

0 commit comments

Comments
 (0)