diff --git a/src/editor.rs b/src/editor.rs index cdf4ce7..8f85a33 100644 --- a/src/editor.rs +++ b/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. - /// * `Down` / `C-n` --- next history entry. + /// * `Up` --- previous candidate with a dropdown, else previous history. + /// * `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 diff --git a/src/minibuffer.rs b/src/minibuffer.rs index 7eda07d..889cc2d 100644 --- a/src/minibuffer.rs +++ b/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::Down => return Self::HistoryNext, + KeyCode::Up => return Self::PrevCandidateOrHistory, + 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();