openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(tui): return keymap edits to main picker

Browse Source 
Return successful shortcut edits and removals to the main `keymap`
picker with the edited action selected. This keeps the list flow stable
after save instead of reopening the focused binding menu.
This commit is contained in:
Felipe Coury
2026-04-23 10:35:35 -03:00
parent 9baecb04e5
commit 0be4b70d15
5 changed files with 235 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/app/event_dispatch.rs
+2 -2
View File
@@ -1731,7 +1731,7 @@ impl App {
self.chat_widget
.apply_keymap_update(keymap_config, &runtime_keymap);
self.chat_widget
.refresh_keymap_action_menu(&context, &action, &runtime_keymap);
.return_to_keymap_picker(&context, &action, &runtime_keymap);
self.chat_widget.add_info_message(message, /*hint*/ None);
}
Err(err) => {
@@ -1776,7 +1776,7 @@ impl App {
self.chat_widget
.apply_keymap_update(keymap_config, &runtime_keymap);
self.chat_widget
.refresh_keymap_action_menu(&context, &action, &runtime_keymap);
.return_to_keymap_picker(&context, &action, &runtime_keymap);
self.chat_widget.add_info_message(
format!("Removed custom shortcut for `{context}.{action}`."),
/*hint*/ None,
codex-rs/tui/src/bottom_pane/mod.rs
+33
View File
@@ -896,6 +896,39 @@ impl BottomPane {
true
}
/// Replace one or more active views whose IDs are in `view_ids` with a
/// generic list selection view.
pub(crate) fn replace_active_views_with_selection_view(
&mut self,
view_ids: &[&'static str],
params: list_selection_view::SelectionViewParams,
) -> bool {
let is_match = self
.view_stack
.last()
.and_then(|view| view.view_id())
.is_some_and(|view_id| view_ids.contains(&view_id));
if !is_match {
return false;
}
while self
.view_stack
.last()
.and_then(|view| view.view_id())
.is_some_and(|view_id| view_ids.contains(&view_id))
{
self.view_stack.pop();
}
let view = list_selection_view::ListSelectionView::new(
params,
self.app_event_tx.clone(),
self.keymap.list.clone(),
);
self.push_view(Box::new(view));
true
}
pub(crate) fn selected_index_for_active_view(&self, view_id: &'static str) -> Option<usize> {
self.view_stack
.last()
codex-rs/tui/src/chatwidget.rs
+21 -9
View File
@@ -7849,24 +7849,36 @@ impl ChatWidget {
self.request_redraw();
}
pub(crate) fn refresh_keymap_action_menu(
pub(crate) fn return_to_keymap_picker(
&mut self,
context: &str,
action: &str,
runtime_keymap: &RuntimeKeymap,
) {
let params = keymap_setup::build_keymap_action_menu_params(
context.to_string(),
action.to_string(),
let params = keymap_setup::build_keymap_picker_params_for_selected_action(
runtime_keymap,
&self.config.tui_keymap,
context,
action,
);
let replaced = self
.bottom_pane
.replace_selection_view_if_active(keymap_setup::KEYMAP_ACTION_MENU_VIEW_ID, params);
if replaced {
self.request_redraw();
let replaced = self.bottom_pane.replace_active_views_with_selection_view(
&[
keymap_setup::KEYMAP_PICKER_VIEW_ID,
keymap_setup::KEYMAP_ACTION_MENU_VIEW_ID,
keymap_setup::KEYMAP_REPLACE_BINDING_MENU_VIEW_ID,
],
params,
);
if !replaced {
let params = keymap_setup::build_keymap_picker_params_for_selected_action(
runtime_keymap,
&self.config.tui_keymap,
context,
action,
);
self.bottom_pane.show_selection_view(params);
}
self.request_redraw();
}
fn status_line_context_window_size(&self) -> Option<i64> {
codex-rs/tui/src/keymap_setup.rs
+152 -18
View File
@@ -6,7 +6,9 @@
mod actions;
mod picker;
pub(crate) use picker::KEYMAP_PICKER_VIEW_ID;
pub(crate) use picker::build_keymap_picker_params;
pub(crate) use picker::build_keymap_picker_params_for_selected_action;
use codex_config::types::KeybindingSpec;
use codex_config::types::KeybindingsSpec;
@@ -969,13 +971,28 @@ mod tests {
assert_eq!(unbound_tab.items.len(), 1);
assert_eq!(unbound_tab.items[0].name, "Toggle Vim Mode");
assert_eq!(
unbound_tab.items[0].description.as_deref(),
Some("unbound")
);
assert_eq!(unbound_tab.items[0].description.as_deref(), Some("unbound"));
assert!(!unbound_tab.items[0].is_disabled);
}
#[test]
fn picker_selected_action_starts_on_matching_all_tab_row() {
let runtime = RuntimeKeymap::defaults();
let params = build_keymap_picker_params_for_selected_action(
&runtime,
&TuiKeymap::default(),
"composer",
"submit",
);
let all_tab = selection_tab(&params, KEYMAP_ALL_TAB_ID);
assert_eq!(params.initial_tab_id.as_deref(), Some(KEYMAP_ALL_TAB_ID));
assert_eq!(
params.initial_selected_idx,
all_tab.items.iter().position(|item| item.name == "Submit")
);
}
#[test]
fn picker_all_tab_items_remain_searchable() {
let runtime = RuntimeKeymap::defaults();
@@ -1124,10 +1141,7 @@ mod tests {
!add_alternate.dismiss_on_select,
"add alternate should keep the action menu under key capture"
);
assert!(
!remove.dismiss_on_select,
"clear-key should keep the action menu open for refresh"
);
assert!(!remove.dismiss_on_select, "clear-key waits for save result");
assert!(
back.dismiss_on_select,
"back should dismiss the action menu"
@@ -1152,9 +1166,10 @@ mod tests {
}
#[test]
fn capture_completion_returns_to_keymap_action_menu() {
fn capture_completion_returns_to_selected_keymap_picker_row() {
let (mut pane, tx, mut rx) = test_pane();
let runtime = RuntimeKeymap::defaults();
pane.show_selection_view(build_keymap_picker_params(&runtime, &TuiKeymap::default()));
pane.show_selection_view(build_keymap_action_menu_params(
"composer".to_string(),
"submit".to_string(),
@@ -1198,18 +1213,137 @@ mod tests {
let keymap =
keymap_with_replacement(&TuiKeymap::default(), &context, &action, &key).unwrap();
let runtime = RuntimeKeymap::from_config(&keymap).unwrap();
let params = build_keymap_action_menu_params(context, action, &runtime, &keymap);
let params =
build_keymap_picker_params_for_selected_action(&runtime, &keymap, &context, &action);
let selected_idx = params.initial_selected_idx;
assert!(
pane.replace_selection_view_if_active(KEYMAP_ACTION_MENU_VIEW_ID, params),
"active action menu should refresh after a successful assignment"
pane.replace_active_views_with_selection_view(
&[
KEYMAP_PICKER_VIEW_ID,
KEYMAP_ACTION_MENU_VIEW_ID,
KEYMAP_REPLACE_BINDING_MENU_VIEW_ID,
],
params
),
"successful assignment should return to the main picker"
);
let area = Rect::new(0, 0, 80, pane.desired_height(/*width*/ 80));
let mut buf = Buffer::empty(area);
pane.render(area, &mut buf);
let rendered = render_buffer(&buf);
assert_eq!(pane.active_view_id(), Some(KEYMAP_PICKER_VIEW_ID));
assert_eq!(
pane.selected_index_for_active_view(KEYMAP_PICKER_VIEW_ID),
selected_idx
);
pane.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
assert!(
rendered.contains("Current ctrl-shift-k"),
"rendered action menu did not include updated binding:\n{rendered}"
pane.no_modal_or_popup_active(),
"the original picker should not remain behind the refreshed picker"
);
}
#[test]
fn clear_completion_returns_to_selected_keymap_picker_row() {
let (mut pane, _tx, mut rx) = test_pane();
let keymap =
keymap_with_replacement(&TuiKeymap::default(), "composer", "submit", "ctrl-enter")
.unwrap();
let runtime = RuntimeKeymap::from_config(&keymap).unwrap();
pane.show_selection_view(build_keymap_picker_params(&runtime, &keymap));
pane.show_selection_view(build_keymap_action_menu_params(
"composer".to_string(),
"submit".to_string(),
&runtime,
&keymap,
));
pane.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE));
pane.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE));
pane.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
let AppEvent::KeymapCleared { context, action } =
rx.try_recv().expect("clear keymap event")
else {
panic!("expected KeymapCleared event");
};
assert_eq!(context, "composer");
assert_eq!(action, "submit");
assert_eq!(pane.active_view_id(), Some(KEYMAP_ACTION_MENU_VIEW_ID));
let runtime = RuntimeKeymap::defaults();
let params = build_keymap_picker_params_for_selected_action(
&runtime,
&TuiKeymap::default(),
&context,
&action,
);
let selected_idx = params.initial_selected_idx;
assert!(
pane.replace_active_views_with_selection_view(
&[
KEYMAP_PICKER_VIEW_ID,
KEYMAP_ACTION_MENU_VIEW_ID,
KEYMAP_REPLACE_BINDING_MENU_VIEW_ID,
],
params
),
"successful clear should return to the main picker"
);
assert_eq!(pane.active_view_id(), Some(KEYMAP_PICKER_VIEW_ID));
assert_eq!(
pane.selected_index_for_active_view(KEYMAP_PICKER_VIEW_ID),
selected_idx
);
pane.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
assert!(
pane.no_modal_or_popup_active(),
"the original picker should not remain behind the refreshed picker"
);
}
#[test]
fn replace_one_completion_drops_focused_keymap_submenus() {
let (mut pane, _tx, _rx) = test_pane();
let runtime = RuntimeKeymap::defaults();
pane.show_selection_view(build_keymap_picker_params(&runtime, &TuiKeymap::default()));
pane.show_selection_view(build_keymap_action_menu_params(
"composer".to_string(),
"toggle_shortcuts".to_string(),
&runtime,
&TuiKeymap::default(),
));
pane.show_selection_view(build_keymap_replace_binding_menu_params(
"composer".to_string(),
"toggle_shortcuts".to_string(),
&runtime,
));
assert_eq!(
pane.active_view_id(),
Some(KEYMAP_REPLACE_BINDING_MENU_VIEW_ID)
);
let params = build_keymap_picker_params_for_selected_action(
&runtime,
&TuiKeymap::default(),
"composer",
"toggle_shortcuts",
);
assert!(
pane.replace_active_views_with_selection_view(
&[
KEYMAP_PICKER_VIEW_ID,
KEYMAP_ACTION_MENU_VIEW_ID,
KEYMAP_REPLACE_BINDING_MENU_VIEW_ID,
],
params
),
"successful replace-one should return to the main picker"
);
assert_eq!(pane.active_view_id(), Some(KEYMAP_PICKER_VIEW_ID));
pane.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
assert!(
pane.no_modal_or_popup_active(),
"the parent action menu should not remain behind the picker"
);
}
codex-rs/tui/src/keymap_setup/picker.rs
+27 -1
View File
@@ -22,7 +22,7 @@ use super::actions::bindings_for_action;
use super::actions::format_binding_summary;
use super::has_custom_binding;
const KEYMAP_PICKER_VIEW_ID: &str = "keymap-picker";
pub(crate) const KEYMAP_PICKER_VIEW_ID: &str = "keymap-picker";
pub(super) const KEYMAP_ALL_TAB_ID: &str = "all-shortcuts";
pub(super) const KEYMAP_COMMON_TAB_ID: &str = "common-shortcuts";
pub(super) const KEYMAP_CUSTOM_TAB_ID: &str = "custom-shortcuts";
@@ -118,11 +118,36 @@ const KEYMAP_CONTEXT_TABS: &[KeymapContextTab] = &[
pub(crate) fn build_keymap_picker_params(
runtime_keymap: &RuntimeKeymap,
keymap_config: &TuiKeymap,
) -> SelectionViewParams {
build_keymap_picker_params_for_action(
runtime_keymap,
keymap_config,
/*selected_action*/ None,
)
}
pub(crate) fn build_keymap_picker_params_for_selected_action(
runtime_keymap: &RuntimeKeymap,
keymap_config: &TuiKeymap,
context: &str,
action: &str,
) -> SelectionViewParams {
build_keymap_picker_params_for_action(runtime_keymap, keymap_config, Some((context, action)))
}
fn build_keymap_picker_params_for_action(
runtime_keymap: &RuntimeKeymap,
keymap_config: &TuiKeymap,
selected_action: Option<(&str, &str)>,
) -> SelectionViewParams {
let rows = build_keymap_rows(runtime_keymap, keymap_config);
let total = rows.len();
let custom_count = rows.iter().filter(|row| row.custom_binding).count();
let unbound_count = rows.iter().filter(|row| row.is_unbound()).count();
let initial_selected_idx = selected_action.and_then(|(context, action)| {
rows.iter()
.position(|row| row.context == context && row.action == action)
});
let name_column_width = rows
.iter()
.map(|row| KEYMAP_ROW_PREFIX_WIDTH + UnicodeWidthStr::width(row.label.as_str()))
@@ -224,6 +249,7 @@ pub(crate) fn build_keymap_picker_params(
col_width_mode: ColumnWidthMode::AutoAllRows,
row_display: SelectionRowDisplay::SingleLine,
name_column_width,
initial_selected_idx,
..Default::default()
}
}