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

@@ -32,38 +32,35 @@ impl UIEvents {
         }
     }
 
-    pub fn next(&self) -> Result<Option<KeyEvent>> {
+    /// Read the next key press event, dropping any other preceding events. Returns None if no
-        let now = Instant::now();
+    /// relevant event is found within the configured tick_rate.
-        match poll(self.tick_rate) {
+    pub fn next_key_press(&self) -> Result<Option<KeyEvent>> {
-            Ok(true) => {
+        let deadline = Instant::now() + self.tick_rate;
-                if let Event::Key(event) = read()? {
+        loop {
-                    // We are only interested in Pressed events;
+            let timeout = deadline.saturating_duration_since(Instant::now());
-                    // It's crucial because there are cases where terminal MIGHT produce false events;
+            if !poll(timeout)? {
-                    // 2 events 1 for release 1 for press.
+                return Ok(None);
-                    // Want to react only on 1 of them so we do.
+            }
-                    if event.kind == KeyEventKind::Press {
+            if let Event::Key(event) = read()? {
-                        return Ok(Some(event));
+                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()
             }
-            Ok(false) => Ok(None),
-            Err(err) => Err(err),
         }
     }
 
-    pub fn try_next(&self) -> Result<Option<KeyEvent>> {
+    /// Read the next key press event, dropping any other preceding events. If no key event is
-        match poll(Duration::default()) {
+    /// available, returns immediately.
-            Ok(true) => match read()? {
+    pub fn try_next_key_press(&self) -> Result<Option<KeyEvent>> {
-                Event::Key(event) if event.kind == KeyEventKind::Press => Ok(Some(event)),
+        loop {
-                _ => Ok(None),
+            if !poll(Duration::ZERO)? {
-            },
+                return Ok(None);
-            Ok(false) => Ok(None),
+            }
-            Err(err) => Err(err),
+            if let Event::Key(event) = read()? {
+                if event.kind == KeyEventKind::Press {
+                    return Ok(Some(event));
+                }
+            }
         }
     }
 }

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<V: View>(
     command: &mut CommandBuf,
     mut view: Option<&mut V>,
 ) -> Option<Transition> {
-    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.
-
+    loop {
-    while let Ok(Some(key)) = events.try_next() {
         let result = handle_event(
             engine_state,
             stack,
@@ -649,13 +647,18 @@ fn handle_events<V: View>(
             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)]