Merge pull request #89 from levineuwirth/fix-minibuffer-arrow-completion-nav

fix(minibuffer): arrow keys navigate the completion dropdown

src/editor.rs

@@ -592,8 +592,10 @@ impl EditorState {
     /// * `RET` / `C-m`           --- accept (invoke `on_accept`).
     /// * `C-g`                   --- cancel (invoke `on_cancel`).
     /// * `TAB` / `C-i`           --- complete to selected candidate.
-    /// * `Up` / `C-p`            --- previous history entry.
+    /// * `Up`                    --- previous candidate with a dropdown, else previous history.
-    /// * `Down` / `C-n`          --- next history entry.
+    /// * `Down`                  --- next candidate with a dropdown, else next history.
+    /// * `C-p`                   --- previous history entry (always).
+    /// * `C-n`                   --- next history entry (always).
     /// * `BS`                    --- delete codepoint left of cursor.
     /// * `DEL` / `C-d`           --- delete codepoint at cursor.
     /// * `Left`  / `C-b`         --- cursor left.
@@ -620,6 +622,22 @@ impl EditorState {
             MinibufferAction::HistoryNext => self.with_minibuffer(Minibuffer::history_next),
             MinibufferAction::ScrollNext => self.with_minibuffer(|m| m.scroll_candidate(1)),
             MinibufferAction::ScrollPrev => self.with_minibuffer(|m| m.scroll_candidate(-1)),
+            // Arrows navigate the completion dropdown when one is showing
+            // (the intuitive default), else step through history.
+            MinibufferAction::PrevCandidateOrHistory => {
+                if self.core.borrow().minibuffer.has_candidates() {
+                    self.with_minibuffer(|m| m.scroll_candidate(-1));
+                } else {
+                    self.with_minibuffer(Minibuffer::history_prev);
+                }
+            }
+            MinibufferAction::NextCandidateOrHistory => {
+                if self.core.borrow().minibuffer.has_candidates() {
+                    self.with_minibuffer(|m| m.scroll_candidate(1));
+                } else {
+                    self.with_minibuffer(Minibuffer::history_next);
+                }
+            }
             MinibufferAction::Backspace => {
                 self.with_minibuffer(Minibuffer::backspace);
                 self.recompute_minibuffer_candidates();
@@ -6268,6 +6286,40 @@ mod tests {
         assert_eq!(cursor.unwrap().row, 23);
     }
 
+    #[test]
+    fn arrow_keys_navigate_the_completion_dropdown() {
+        // Regression: Up/Down used to run command HISTORY even with a
+        // completion dropdown showing, so the highlight never moved. Now
+        // they navigate the dropdown when one is present.
+        let selected = |s: &EditorState| {
+            s.core
+                .borrow()
+                .minibuffer
+                .session
+                .as_ref()
+                .expect("session")
+                .selected
+        };
+        let mut s = fresh_with(b"");
+        s.dispatch_key(FrontendId::LOCAL, alt('x'));
+        assert!(
+            s.core.borrow().minibuffer.has_candidates(),
+            "M-x populates a completion dropdown"
+        );
+        let sel0 = selected(&s);
+
+        s.dispatch_key(FrontendId::LOCAL, plain(KeyCode::Down));
+        let sel1 = selected(&s);
+        assert_ne!(sel0, sel1, "Down must move the completion selection");
+
+        s.dispatch_key(FrontendId::LOCAL, plain(KeyCode::Up));
+        assert_eq!(
+            selected(&s),
+            sel0,
+            "Up must move the completion selection back"
+        );
+    }
+
     #[test]
     fn render_clears_grid_between_frames() {
         // Frame 1 renders some text; frame 2 with shorter content

src/minibuffer.rs

@@ -244,6 +244,16 @@ impl Minibuffer {
         s.selected = Some(usize::try_from(next).unwrap_or(0));
     }
 
+    /// Whether a completion dropdown is currently showing (the active
+    /// session has at least one candidate). Drives whether the Up/Down
+    /// arrows navigate the dropdown or step through command history.
+    #[must_use]
+    pub fn has_candidates(&self) -> bool {
+        self.session
+            .as_ref()
+            .is_some_and(|s| !s.candidates.is_empty())
+    }
+
     /// Replace the buffer contents with the currently-selected
     /// candidate, leaving the session active so the user can continue
     /// editing or accept. No-op when nothing is selected.
@@ -422,14 +432,21 @@ pub enum MinibufferAction {
     Cancel,
     /// Replace the buffer with the selected candidate (TAB / C-i).
     Complete,
-    /// Step backward through history (Up / C-p).
+    /// Step backward through history (C-p).
     HistoryPrev,
-    /// Step forward through history (Down / C-n).
+    /// Step forward through history (C-n).
     HistoryNext,
     /// Cycle the selected candidate forward (M-n).
     ScrollNext,
     /// Cycle the selected candidate backward (M-p).
     ScrollPrev,
+    /// Up arrow: move to the previous completion candidate when a
+    /// dropdown is showing, else step back through history. Resolved in
+    /// the dispatcher, which has the session state `from_chord` lacks.
+    PrevCandidateOrHistory,
+    /// Down arrow: move to the next completion candidate when a dropdown
+    /// is showing, else step forward through history.
+    NextCandidateOrHistory,
     /// Backspace.
     Backspace,
     /// Forward delete (DEL / C-d).
@@ -465,8 +482,8 @@ impl MinibufferAction {
                 KeyCode::Enter => return Self::Accept,
                 KeyCode::Esc => return Self::Cancel,
                 KeyCode::Tab => return Self::Complete,
-                KeyCode::Up => return Self::HistoryPrev,
+                KeyCode::Up => return Self::PrevCandidateOrHistory,
-                KeyCode::Down => return Self::HistoryNext,
+                KeyCode::Down => return Self::NextCandidateOrHistory,
                 KeyCode::Left => return Self::Left,
                 KeyCode::Right => return Self::Right,
                 KeyCode::Home => return Self::LineStart,
@@ -908,6 +925,28 @@ mod tests {
         }
     }
 
+    #[test]
+    fn from_chord_arrows_are_candidate_or_history() {
+        use crossterm::event::{KeyCode, KeyModifiers};
+        // Up/Down resolve to dropdown-or-history in the dispatcher; the
+        // chord decode just tags them (was HistoryPrev/HistoryNext, which
+        // ignored the completion dropdown entirely).
+        assert!(matches!(
+            MinibufferAction::from_chord(Chord {
+                code: KeyCode::Up,
+                modifiers: KeyModifiers::NONE,
+            }),
+            MinibufferAction::PrevCandidateOrHistory
+        ));
+        assert!(matches!(
+            MinibufferAction::from_chord(Chord {
+                code: KeyCode::Down,
+                modifiers: KeyModifiers::NONE,
+            }),
+            MinibufferAction::NextCandidateOrHistory
+        ));
+    }
+
     #[test]
     fn insert_and_backspace_round_trip() {
         let mut mb = Minibuffer::new();