From 983014cc40e6a96548c731b7de26e3a1a70d6426 Mon Sep 17 00:00:00 2001 From: Piotr Kufel Date: Fri, 9 Aug 2024 18:07:50 -0700 Subject: [PATCH] Clean up key event handling (#13574) # Description Cleanups: - Add "key_press" to event reading function names to match reality - Move the relevant comment about why only key press events are interesting one layer up - Remove code duplication in handle_events - Make `try_next` try harder (instead of bail on a boring event); I think that was the original intention - Remove recursion from `next` (I think that's clearer? but maybe just what I'm used to) # User-Facing Changes None # Tests + Formatting This cleans up existing code, no new test coverage. --- crates/nu-explore/src/pager/events.rs | 53 +++++++++++++-------------- crates/nu-explore/src/pager/mod.rs | 47 +++++++++++++----------- 2 files changed, 50 insertions(+), 50 deletions(-) diff --git a/crates/nu-explore/src/pager/events.rs b/crates/nu-explore/src/pager/events.rs index b408b10f1d..6023ddd8e3 100644 --- a/crates/nu-explore/src/pager/events.rs +++ b/crates/nu-explore/src/pager/events.rs @@ -32,38 +32,35 @@ impl UIEvents { } } - pub fn next(&self) -> Result> { - let now = Instant::now(); - match poll(self.tick_rate) { - Ok(true) => { - if let Event::Key(event) = read()? { - // We are only interested in Pressed events; - // It's crucial because there are cases where terminal MIGHT produce false events; - // 2 events 1 for release 1 for press. - // Want to react only on 1 of them so we do. - if event.kind == KeyEventKind::Press { - return Ok(Some(event)); - } - } - - let time_spent = now.elapsed(); - let rest = self.tick_rate.saturating_sub(time_spent); - - Self { tick_rate: rest }.next() + /// Read the next key press event, dropping any other preceding events. Returns None if no + /// relevant event is found within the configured tick_rate. + pub fn next_key_press(&self) -> Result> { + let deadline = Instant::now() + self.tick_rate; + loop { + let timeout = deadline.saturating_duration_since(Instant::now()); + if !poll(timeout)? { + return Ok(None); + } + if let Event::Key(event) = read()? { + if event.kind == KeyEventKind::Press { + return Ok(Some(event)); + } } - Ok(false) => Ok(None), - Err(err) => Err(err), } } - pub fn try_next(&self) -> Result> { - match poll(Duration::default()) { - Ok(true) => match read()? { - Event::Key(event) if event.kind == KeyEventKind::Press => Ok(Some(event)), - _ => Ok(None), - }, - Ok(false) => Ok(None), - Err(err) => Err(err), + /// Read the next key press event, dropping any other preceding events. If no key event is + /// available, returns immediately. + pub fn try_next_key_press(&self) -> Result> { + loop { + if !poll(Duration::ZERO)? { + return Ok(None); + } + if let Event::Key(event) = read()? { + if event.kind == KeyEventKind::Press { + return Ok(Some(event)); + } + } } } } diff --git a/crates/nu-explore/src/pager/mod.rs b/crates/nu-explore/src/pager/mod.rs index 4a043476bb..5ea560093d 100644 --- a/crates/nu-explore/src/pager/mod.rs +++ b/crates/nu-explore/src/pager/mod.rs @@ -191,6 +191,12 @@ fn render_ui( })?; } + // Note that this will return within the configured tick_rate of events. In particular this + // means this loop keeps redrawing the UI about 4 times a second, whether it needs to or + // not. That's OK-ish because ratatui will detect real changes and not send unnecessary + // output to the terminal (something that may especially be important with ssh). While not + // needed at the moment, the idea is that this behavior allows for some degree of + // animation (so that the UI can update over time, even without user input). let transition = handle_events( engine_state, stack, @@ -612,33 +618,25 @@ fn handle_events( command: &mut CommandBuf, mut view: Option<&mut V>, ) -> Option { - let key = match events.next() { + // We are only interested in Pressed events; + // It's crucial because there are cases where terminal MIGHT produce false events; + // 2 events 1 for release 1 for press. + // Want to react only on 1 of them so we do. + let mut key = match events.next_key_press() { Ok(Some(key)) => key, - _ => return None, + Ok(None) => return None, + Err(e) => { + log::error!("Failed to read key event: {e}"); + return None; + } }; - let result = handle_event( - engine_state, - stack, - layout, - info, - search, - command, - view.as_deref_mut(), - key, - ); - - if result.is_some() { - return result; - } - // Sometimes we get a BIG list of events; // for example when someone scrolls via a mouse either UP or DOWN. // This MIGHT cause freezes as we have a 400 delay for a next command read. // // To eliminate that we are trying to read all possible commands which we should act upon. - - while let Ok(Some(key)) = events.try_next() { + loop { let result = handle_event( engine_state, stack, @@ -649,13 +647,18 @@ fn handle_events( view.as_deref_mut(), key, ); - if result.is_some() { return result; } + match events.try_next_key_press() { + Ok(Some(next_key)) => key = next_key, + Ok(None) => return None, + Err(e) => { + log::error!("Failed to peek key event: {e}"); + return None; + } + } } - - result } #[allow(clippy::too_many_arguments)]