From 01ff831ce9c1c9d3ae98f271014b22401308ec06 Mon Sep 17 00:00:00 2001 From: Benjamin Carlsson Date: Wed, 14 Oct 2026 19:54:54 +0000 Subject: [PATCH] Centralize vertical navigation in TUI list views (#54081) ## What changed Add `ListKeymap::vertical_action_for` and `ScrollState::apply_vertical_action` to share key resolution, selection movement, and visibility handling across the experimental features, hooks, memories, skills, and multi-select views. Preserve navigation priority, search input handling, and item reordering precedence. ## Testing Update scroll-state tests to exercise the shared action handler and picker tests to navigate through key events. Cover overlapping page-down and move-left bindings when item reordering is disabled. GitOrigin-RevId: 29927b0a4de0fbeb4c0dd19c418b8f60fffb8a7d --- .../bottom_pane/experimental_features_view.rs | 63 +++-------- .../tui/src/bottom_pane/hooks_browser_view.rs | 49 ++------- .../src/bottom_pane/memories_settings_view.rs | 65 +++-------- .../src/bottom_pane/multi_select_picker.rs | 102 +++++------------- codex-rs/tui/src/bottom_pane/scroll_state.rs | 43 ++++++-- .../tui/src/bottom_pane/skills_toggle_view.rs | 65 ++--------- codex-rs/tui/src/keymap.rs | 14 +++ 7 files changed, 121 insertions(+), 280 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/experimental_features_view.rs b/codex-rs/tui/src/bottom_pane/experimental_features_view.rs index 63a5bd85e1..805e3eff38 100644 --- a/codex-rs/tui/src/bottom_pane/experimental_features_view.rs +++ b/codex-rs/tui/src/bottom_pane/experimental_features_view.rs @@ -163,48 +163,6 @@ impl ExperimentalFeaturesView { rows } - fn move_up(&mut self) { - let len = self.visible_len(); - if len == 0 { - return; - } - self.state.move_up_wrap(len); - self.state.ensure_visible(len, MAX_POPUP_ROWS.min(len)); - } - - fn move_down(&mut self) { - let len = self.visible_len(); - if len == 0 { - return; - } - self.state.move_down_wrap(len); - self.state.ensure_visible(len, MAX_POPUP_ROWS.min(len)); - } - - fn page_up(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.state.page_up_clamped(len, visible); - } - - fn page_down(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.state.page_down_clamped(len, visible); - } - - fn jump_top(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.state.jump_top(len, visible); - } - - fn jump_bottom(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.state.jump_bottom(len, visible); - } - fn toggle_selected(&mut self) { let Some(selected_idx) = self.state.selected_idx else { return; @@ -375,13 +333,22 @@ impl BottomPaneView for ExperimentalFeaturesView { } fn handle_key_event(&mut self, key_event: KeyEvent) { + if let Some(action) = self.keymap.vertical_action_for(key_event) { + let len = self.visible_len(); + if len == 0 + && matches!( + action, + crate::keymap::ListAction::MoveUp | crate::keymap::ListAction::MoveDown + ) + { + return; + } + self.state + .apply_vertical_action(action, len, MAX_POPUP_ROWS.min(len)); + return; + } + match key_event { - _ if self.keymap.move_up.is_pressed(key_event) => self.move_up(), - _ if self.keymap.move_down.is_pressed(key_event) => self.move_down(), - _ if self.keymap.page_up.is_pressed(key_event) => self.page_up(), - _ if self.keymap.page_down.is_pressed(key_event) => self.page_down(), - _ if self.keymap.jump_top.is_pressed(key_event) => self.jump_top(), - _ if self.keymap.jump_bottom.is_pressed(key_event) => self.jump_bottom(), KeyEvent { code: KeyCode::Char(' '), modifiers: KeyModifiers::NONE, diff --git a/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs b/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs index 1e31fa5a58..b73eca721c 100644 --- a/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs +++ b/codex-rs/tui/src/bottom_pane/hooks_browser_view.rs @@ -165,38 +165,6 @@ impl HooksBrowserView { .and_then(|idx| self.entry.hooks.get(idx)) } - fn move_up(&mut self) { - let len = self.page_len(); - self.state.move_up_wrap(len); - self.state.ensure_visible(len, self.max_visible_rows()); - } - - fn move_down(&mut self) { - let len = self.page_len(); - self.state.move_down_wrap(len); - self.state.ensure_visible(len, self.max_visible_rows()); - } - - fn page_up(&mut self) { - let len = self.page_len(); - self.state.page_up_clamped(len, self.max_visible_rows()); - } - - fn page_down(&mut self) { - let len = self.page_len(); - self.state.page_down_clamped(len, self.max_visible_rows()); - } - - fn jump_top(&mut self) { - let len = self.page_len(); - self.state.jump_top(len, self.max_visible_rows()); - } - - fn jump_bottom(&mut self) { - let len = self.page_len(); - self.state.jump_bottom(len, self.max_visible_rows()); - } - fn page_len(&self) -> usize { match self.page { HooksBrowserPage::Events => codex_protocol::protocol::HookEventName::iter().count(), @@ -204,10 +172,6 @@ impl HooksBrowserView { } } - fn max_visible_rows(&self) -> usize { - MAX_POPUP_ROWS.min(self.page_len().max(1)) - } - fn open_selected_event(&mut self) { let Some(event_name) = self.selected_event() else { return; @@ -613,13 +577,14 @@ impl BottomPaneView for HooksBrowserView { } fn handle_key_event(&mut self, key_event: KeyEvent) { + if let Some(action) = self.keymap.vertical_action_for(key_event) { + let len = self.page_len(); + self.state + .apply_vertical_action(action, len, MAX_POPUP_ROWS.min(len.max(1))); + return; + } + match key_event { - _ if self.keymap.move_up.is_pressed(key_event) => self.move_up(), - _ if self.keymap.move_down.is_pressed(key_event) => self.move_down(), - _ if self.keymap.page_up.is_pressed(key_event) => self.page_up(), - _ if self.keymap.page_down.is_pressed(key_event) => self.page_down(), - _ if self.keymap.jump_top.is_pressed(key_event) => self.jump_top(), - _ if self.keymap.jump_bottom.is_pressed(key_event) => self.jump_bottom(), _ if self.keymap.accept.is_pressed(key_event) && self.page == HooksBrowserPage::Events => { diff --git a/codex-rs/tui/src/bottom_pane/memories_settings_view.rs b/codex-rs/tui/src/bottom_pane/memories_settings_view.rs index cd78eb3994..5cc386266e 100644 --- a/codex-rs/tui/src/bottom_pane/memories_settings_view.rs +++ b/codex-rs/tui/src/bottom_pane/memories_settings_view.rs @@ -223,50 +223,6 @@ impl MemoriesSettingsView { .collect() } - fn move_up(&mut self) { - let len = self.visible_len(); - if len == 0 { - return; - } - let state = self.active_state_mut(); - state.move_up_wrap(len); - state.ensure_visible(len, MAX_POPUP_ROWS.min(len)); - } - - fn move_down(&mut self) { - let len = self.visible_len(); - if len == 0 { - return; - } - let state = self.active_state_mut(); - state.move_down_wrap(len); - state.ensure_visible(len, MAX_POPUP_ROWS.min(len)); - } - - fn page_up(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.active_state_mut().page_up_clamped(len, visible); - } - - fn page_down(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.active_state_mut().page_down_clamped(len, visible); - } - - fn jump_top(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.active_state_mut().jump_top(len, visible); - } - - fn jump_bottom(&mut self) { - let len = self.visible_len(); - let visible = MAX_POPUP_ROWS.min(len); - self.active_state_mut().jump_bottom(len, visible); - } - fn toggle_selected(&mut self) { if self.reset_confirmation.is_some() { return; @@ -321,13 +277,22 @@ impl BottomPaneView for MemoriesSettingsView { } fn handle_key_event(&mut self, key_event: KeyEvent) { + if let Some(action) = self.keymap.vertical_action_for(key_event) { + let len = self.visible_len(); + if len == 0 + && matches!( + action, + crate::keymap::ListAction::MoveUp | crate::keymap::ListAction::MoveDown + ) + { + return; + } + self.active_state_mut() + .apply_vertical_action(action, len, MAX_POPUP_ROWS.min(len)); + return; + } + match key_event { - _ if self.keymap.move_up.is_pressed(key_event) => self.move_up(), - _ if self.keymap.move_down.is_pressed(key_event) => self.move_down(), - _ if self.keymap.page_up.is_pressed(key_event) => self.page_up(), - _ if self.keymap.page_down.is_pressed(key_event) => self.page_down(), - _ if self.keymap.jump_top.is_pressed(key_event) => self.jump_top(), - _ if self.keymap.jump_bottom.is_pressed(key_event) => self.jump_bottom(), KeyEvent { code: KeyCode::Char(' '), modifiers: KeyModifiers::NONE, diff --git a/codex-rs/tui/src/bottom_pane/multi_select_picker.rs b/codex-rs/tui/src/bottom_pane/multi_select_picker.rs index cf0bb3410c..a16920a926 100644 --- a/codex-rs/tui/src/bottom_pane/multi_select_picker.rs +++ b/codex-rs/tui/src/bottom_pane/multi_select_picker.rs @@ -353,46 +353,6 @@ impl MultiSelectPicker { } } - /// Moves the selection cursor up, wrapping to the bottom if at the top. - fn move_up(&mut self) { - let len = self.visible_len(); - self.state.move_up_wrap(len); - let visible = Self::max_visible_rows(len); - self.state.ensure_visible(len, visible); - } - - /// Moves the selection cursor down, wrapping to the top if at the bottom. - fn move_down(&mut self) { - let len = self.visible_len(); - self.state.move_down_wrap(len); - let visible = Self::max_visible_rows(len); - self.state.ensure_visible(len, visible); - } - - fn page_up(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.page_up_clamped(len, visible); - } - - fn page_down(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.page_down_clamped(len, visible); - } - - fn jump_top(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.jump_top(len, visible); - } - - fn jump_bottom(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.jump_bottom(len, visible); - } - /// Toggles the enabled state of the currently selected item. /// /// Updates the preview line and invokes the `on_change` callback if set. @@ -543,37 +503,30 @@ impl BottomPaneView for MultiSelectPicker { // plain j/k only apply through non-text events or modified bindings. let allow_plain_char_navigation = !is_plain_text_key_event(key_event); + if allow_plain_char_navigation + && self.ordering_enabled + && self.keymap.move_left.is_pressed(key_event) + { + self.move_selected_item(Direction::Up); + return; + } + if allow_plain_char_navigation + && self.ordering_enabled + && self.keymap.move_right.is_pressed(key_event) + { + self.move_selected_item(Direction::Down); + return; + } + if allow_plain_char_navigation + && let Some(action) = self.keymap.vertical_action_for(key_event) + { + let len = self.visible_len(); + self.state + .apply_vertical_action(action, len, Self::max_visible_rows(len)); + return; + } + match key_event { - _ if allow_plain_char_navigation - && self.ordering_enabled - && self.keymap.move_left.is_pressed(key_event) => - { - self.move_selected_item(Direction::Up); - } - _ if allow_plain_char_navigation - && self.ordering_enabled - && self.keymap.move_right.is_pressed(key_event) => - { - self.move_selected_item(Direction::Down); - } - _ if allow_plain_char_navigation && self.keymap.move_up.is_pressed(key_event) => { - self.move_up() - } - _ if allow_plain_char_navigation && self.keymap.move_down.is_pressed(key_event) => { - self.move_down() - } - _ if allow_plain_char_navigation && self.keymap.page_up.is_pressed(key_event) => { - self.page_up() - } - _ if allow_plain_char_navigation && self.keymap.page_down.is_pressed(key_event) => { - self.page_down() - } - _ if allow_plain_char_navigation && self.keymap.jump_top.is_pressed(key_event) => { - self.jump_top() - } - _ if allow_plain_char_navigation && self.keymap.jump_bottom.is_pressed(key_event) => { - self.jump_bottom() - } KeyEvent { code: KeyCode::Backspace, .. @@ -958,7 +911,7 @@ mod tests { vec!["theme-colors", "model", "branch"] ); - picker.move_down(); + picker.handle_key_event(KeyEvent::from(KeyCode::Down)); picker.move_selected_item(Direction::Up); assert_eq!( picker @@ -1050,6 +1003,7 @@ mod tests { let (tx, _rx) = unbounded_channel::(); let mut keymap = RuntimeKeymap::defaults().list; keymap.page_down = vec![key_hint::ctrl(KeyCode::Char('d'))]; + keymap.move_left = keymap.page_down.clone(); keymap.page_up = vec![key_hint::ctrl(KeyCode::Char('u'))]; keymap.jump_bottom = vec![key_hint::ctrl(KeyCode::Char('e'))]; keymap.jump_top = vec![key_hint::ctrl(KeyCode::Char('a'))]; @@ -1063,7 +1017,7 @@ mod tests { .map(|idx| { item( &format!("item-{idx}"), - /*orderable*/ true, + /*orderable*/ false, /*section_break_after*/ false, ) }) @@ -1102,7 +1056,7 @@ mod tests { .collect(), ); picker.header = Box::new(()); - picker.jump_bottom(); + picker.handle_key_event(KeyEvent::from(KeyCode::End)); let mut snapshots = Vec::new(); for height in 1..=7 { let area = Rect::new(/*x*/ 0, /*y*/ 0, /*width*/ 80, height); @@ -1183,7 +1137,7 @@ mod tests { buf }; let first = render(&picker); - picker.jump_bottom(); + picker.handle_key_event(KeyEvent::from(KeyCode::End)); picker.toggle_selected(); picker.move_selected_item(Direction::Up); let last = render(&picker); diff --git a/codex-rs/tui/src/bottom_pane/scroll_state.rs b/codex-rs/tui/src/bottom_pane/scroll_state.rs index ce208c7952..b90148c326 100644 --- a/codex-rs/tui/src/bottom_pane/scroll_state.rs +++ b/codex-rs/tui/src/bottom_pane/scroll_state.rs @@ -1,3 +1,5 @@ +use crate::keymap::ListAction; + /// Generic scroll/selection state for a vertical list menu. /// /// Encapsulates the common behavior of a selectable list that supports: @@ -25,6 +27,28 @@ impl ScrollState { } } + /// Apply a vertical list action and keep the resulting selection visible. + pub fn apply_vertical_action(&mut self, action: ListAction, len: usize, visible_rows: usize) { + match action { + ListAction::MoveUp => { + self.move_up_wrap(len); + self.ensure_visible(len, visible_rows); + } + ListAction::MoveDown => { + self.move_down_wrap(len); + self.ensure_visible(len, visible_rows); + } + ListAction::PageUp => self.page_up_clamped(len, visible_rows), + ListAction::PageDown => self.page_down_clamped(len, visible_rows), + ListAction::JumpTop => self.jump_top(len, visible_rows), + ListAction::JumpBottom => self.jump_bottom(len, visible_rows), + ListAction::MoveLeft + | ListAction::MoveRight + | ListAction::Accept + | ListAction::Cancel => {} + } + } + /// Reset selection and scroll. pub fn reset(&mut self) { self.selected_idx = None; @@ -144,6 +168,7 @@ impl ScrollState { #[cfg(test)] mod tests { use super::ScrollState; + use crate::keymap::ListAction; #[test] fn wrap_navigation_and_visibility() { @@ -156,16 +181,14 @@ mod tests { s.ensure_visible(len, vis); assert_eq!(s.scroll_top, 0); - s.move_up_wrap(len); - s.ensure_visible(len, vis); + s.apply_vertical_action(ListAction::MoveUp, len, vis); assert_eq!(s.selected_idx, Some(len - 1)); match s.selected_idx { Some(sel) => assert!(s.scroll_top <= sel), None => panic!("expected Some(selected_idx) after wrap"), } - s.move_down_wrap(len); - s.ensure_visible(len, vis); + s.apply_vertical_action(ListAction::MoveDown, len, vis); assert_eq!(s.selected_idx, Some(0)); assert_eq!(s.scroll_top, 0); } @@ -177,27 +200,27 @@ mod tests { let vis = 4; s.clamp_selection(len); - s.page_down_clamped(len, vis); + s.apply_vertical_action(ListAction::PageDown, len, vis); assert_eq!(s.selected_idx, Some(4)); assert_eq!(s.scroll_top, 1); - s.page_down_clamped(len, vis); + s.apply_vertical_action(ListAction::PageDown, len, vis); assert_eq!(s.selected_idx, Some(8)); assert_eq!(s.scroll_top, 5); - s.page_down_clamped(len, vis); + s.apply_vertical_action(ListAction::PageDown, len, vis); assert_eq!(s.selected_idx, Some(9)); assert_eq!(s.scroll_top, 6); - s.page_up_clamped(len, vis); + s.apply_vertical_action(ListAction::PageUp, len, vis); assert_eq!(s.selected_idx, Some(5)); assert_eq!(s.scroll_top, 5); - s.jump_top(len, vis); + s.apply_vertical_action(ListAction::JumpTop, len, vis); assert_eq!(s.selected_idx, Some(0)); assert_eq!(s.scroll_top, 0); - s.jump_bottom(len, vis); + s.apply_vertical_action(ListAction::JumpBottom, len, vis); assert_eq!(s.selected_idx, Some(9)); assert_eq!(s.scroll_top, 6); } diff --git a/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs b/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs index 9bd9084614..c63a75b5d5 100644 --- a/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs +++ b/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs @@ -162,44 +162,6 @@ impl SkillsToggleView { .collect() } - fn move_up(&mut self) { - let len = self.visible_len(); - self.state.move_up_wrap(len); - let visible = Self::max_visible_rows(len); - self.state.ensure_visible(len, visible); - } - - fn move_down(&mut self) { - let len = self.visible_len(); - self.state.move_down_wrap(len); - let visible = Self::max_visible_rows(len); - self.state.ensure_visible(len, visible); - } - - fn page_up(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.page_up_clamped(len, visible); - } - - fn page_down(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.page_down_clamped(len, visible); - } - - fn jump_top(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.jump_top(len, visible); - } - - fn jump_bottom(&mut self) { - let len = self.visible_len(); - let visible = Self::max_visible_rows(len); - self.state.jump_bottom(len, visible); - } - fn toggle_selected(&mut self) { let Some(idx) = self.state.selected_idx else { return; @@ -241,25 +203,16 @@ impl BottomPaneView for SkillsToggleView { // plain j/k only apply through non-text events or modified bindings. let allow_plain_char_navigation = !is_plain_text_key_event(key_event); + if allow_plain_char_navigation + && let Some(action) = self.keymap.vertical_action_for(key_event) + { + let len = self.visible_len(); + self.state + .apply_vertical_action(action, len, Self::max_visible_rows(len)); + return; + } + match key_event { - _ if allow_plain_char_navigation && self.keymap.move_up.is_pressed(key_event) => { - self.move_up() - } - _ if allow_plain_char_navigation && self.keymap.move_down.is_pressed(key_event) => { - self.move_down() - } - _ if allow_plain_char_navigation && self.keymap.page_up.is_pressed(key_event) => { - self.page_up() - } - _ if allow_plain_char_navigation && self.keymap.page_down.is_pressed(key_event) => { - self.page_down() - } - _ if allow_plain_char_navigation && self.keymap.jump_top.is_pressed(key_event) => { - self.jump_top() - } - _ if allow_plain_char_navigation && self.keymap.jump_bottom.is_pressed(key_event) => { - self.jump_bottom() - } KeyEvent { code: KeyCode::Backspace, .. diff --git a/codex-rs/tui/src/keymap.rs b/codex-rs/tui/src/keymap.rs index a2294dd90f..5a8f3aa93d 100644 --- a/codex-rs/tui/src/keymap.rs +++ b/codex-rs/tui/src/keymap.rs @@ -413,6 +413,20 @@ impl ListKeymap { .find(|action| self.bindings_for(*action).is_pressed(event)) } + /// Resolve only vertical navigation, preserving its historical priority. + pub(crate) fn vertical_action_for(&self, event: KeyEvent) -> Option { + [ + ListAction::MoveUp, + ListAction::MoveDown, + ListAction::PageUp, + ListAction::PageDown, + ListAction::JumpTop, + ListAction::JumpBottom, + ] + .into_iter() + .find(|action| self.bindings_for(*action).is_pressed(event)) + } + pub(crate) fn has_chord_prefix(&self, event: KeyEvent) -> bool { self.chord_hints.bindings.iter().any(|binding| { binding.action.context.overlaps(KeymapContext::List)