From f1340ab070897c48dc079f7aa19febf520bf4756 Mon Sep 17 00:00:00 2001 From: Loric ANDRE Date: Sat, 18 Jul 2026 20:36:47 +0200 Subject: [PATCH] coderabbit review --- ARCHITECTURE.md | 8 +++--- src/binds.rs | 15 ++++++++---- src/manpage.rs | 58 ++++++++++++++++++++++++++------------------ src/tui/app_tests.rs | 5 ++++ 4 files changed, 54 insertions(+), 32 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0e207c4c..29f2129f 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -1314,10 +1314,10 @@ The global allocator is `mimalloc` (v3), chosen for its low-latency multi-thread | `check_and_run_popup` | `src/bin/main.rs:131` | Check popup conditions, dispatch to popup::run_with | | `sk_main` | `src/bin/main.rs:144` | CLI orchestration + output printing | | `SkimEvent` | `src/binds.rs:25` | `change`/`start`/`load`/`result`/`focus`/`zero`/`one` synthetic events → reserved `KeyEvent` | -| `parse_key` | `src/binds.rs:213` | `"ctrl-a"` → `KeyEvent` | -| `parse_action_binds` | `src/binds.rs:299` | `"reload:first"`, `"act-up:suppress+down"` → action follow-up map | -| `parse_action_chain` | `src/binds.rs:333` | `"down+select"` → `Vec` | -| `Action::name` | `src/tui/event.rs:316` | `Action` → canonical bind name (reverse of `parse_action`) | +| `parse_key` | `src/binds.rs:218` | `"ctrl-a"` → `KeyEvent` | +| `parse_action_binds` | `src/binds.rs:324` | `"reload:first"`, `"act-up:suppress+down"` → action follow-up map | +| `parse_action_chain` | `src/binds.rs:358` | `"down+select"` → `Vec` | +| `Action::name` | `src/tui/event.rs:320` | `Action` → canonical bind name (reverse of `parse_action`) | | `Matcher::create_engine_factory_with_builder` | `src/matcher.rs:189` | Build engine factory chain from options | | `ExactOrFuzzyEngineFactory::create_engine_with_case` | `src/engine/factory.rs:93` | Parse query prefixes, build engine | | `AndOrEngineFactory::parse_andor` | `src/engine/factory.rs:176` | Split query into AND/OR tree | diff --git a/src/binds.rs b/src/binds.rs index 7445ab62..8f540afd 100644 --- a/src/binds.rs +++ b/src/binds.rs @@ -17,10 +17,11 @@ use crate::tui::event::{self, Action}; /// The keymap is keyed by crossterm's [`KeyEvent`], which cannot express /// "the query changed" or "reading finished" directly. Each variant is /// therefore represented *transparently* as a reserved function-key code in the -/// high-`F` range (`F(253)`–`F(255)`) that no real terminal ever emits. Giving -/// these reserved codes named variants keeps them in one place instead of -/// scattering magic `F(255)` literals across the codebase, and lets -/// [`parse_key`] accept the friendly names `start`, `load` and `change`. +/// high-`F` range (`F(249)`–`F(255)`) that no real terminal ever emits. The +/// seven variants are `change`, `start`, `load`, `result`, `focus`, `zero`, and +/// `one`. Giving these reserved codes named variants keeps them in one place +/// instead of scattering magic function-key literals across the codebase, and +/// lets [`parse_key`] accept every friendly event name. #[derive(Clone, Copy, PartialEq, Eq, Hash, Debug)] pub enum SkimEvent { /// Fired once, when skim has started up and entered its event loop. @@ -210,7 +211,11 @@ pub fn get_default_key_map() -> KeyMap { KeyMap(ret) } -/// Parses a key str into a crossterm `KeyEvent` +/// Parses a key str into a crossterm `KeyEvent`. +/// +/// In addition to keyboard names, accepts all seven names recognized by +/// [`SkimEvent::from_name`]: `change`, `start`, `load`, `result`, `focus`, +/// `zero`, and `one`. /// /// # Errors /// Returns an error if the key string is empty, contains an unknown modifier, diff --git a/src/manpage.rs b/src/manpage.rs index 6e0a189f..94eeacf7 100644 --- a/src/manpage.rs +++ b/src/manpage.rs @@ -121,24 +121,30 @@ const KEYS_SS: &str = " * alt-shift-right * any single character "; -const BINDABLE_EVENTS_SS: &str = " -* change: the query changes -* start: skim enters its event loop; fired once -* load: the reader and matcher finish consuming the current input; fired once per read, including reloads -* result: filtering for the current query completes -* focus: the focused item changes because of cursor movement or a result update -* zero: a completed search has no matches -* one: a completed search has exactly one match -"; +const BINDABLE_EVENTS_SS: &str = concat!( + "\n", + "* change: the query changes\n", + "* start: skim enters its event loop; fired once\n", + "* load: the reader and matcher finish consuming the current input; ", + "fired once per read, including reloads\n", + "* result: filtering for the current query completes\n", + "* focus: the focused item changes because of cursor movement or a result update\n", + "* zero: a completed search has no matches\n", + "* one: a completed search has exactly one match\n", +); -const ACTION_BINDINGS_SS: &str = " -Actions can also be used as binding triggers. A follow-up chain bound to an action name runs immediately after that action. Use the `act-` prefix for action triggers; it is recommended to avoid ambiguity and required when the action name is also a key, for example `act-up:last`. +const ACTION_BINDINGS_SS: &str = concat!( + "\n", + "Actions can also be used as binding triggers. A follow-up chain bound to an action name runs immediately ", + "after that action. Use the `act-` prefix for action triggers; it is recommended to avoid ambiguity and ", + "required when the action name is also a key, for example `act-up:last`.\n\n", + "Follow-up chains use non-recursive (`noremap`) semantics: their actions do not trigger further action ", + "bindings. Add `suppress` to skip the triggering action's default behavior, for example ", + "`act-up:suppress+down`.\n", +); -Follow-up chains use non-recursive (`noremap`) semantics: their actions do not trigger further action bindings. Add `suppress` to skip the triggering action's default behavior, for example `act-up:suppress+down`. -"; - -const ACTIONS_SS: &str = " -* abort: ctrl-c ctrl-q esc +const ACTIONS_SS: &str = concat!( + "\n* abort: ctrl-c ctrl-q esc * accept(...): enter *the argument will be printed when the binding is triggered* * append-and-select * backward-char: ctrl-b left @@ -184,8 +190,10 @@ const ACTIONS_SS: &str = " * select-row * set-preview-cmd(...): *arg will be a expanded expression, see COMMAND EXPANSION for details * set-query(...): *arg will be a expanded expression, see COMMAND EXPANSION for details -* suppress: *if bound to an action (e.g. `act-up:suppress`), suppresses that action's default behavior so the rest of the non-recursive chain runs once in its place; if bound to a key, equivalent to `ignore` -* toggle +", + "* suppress: *if bound to an action (e.g. `act-up:suppress`), suppresses that action's default behavior ", + "so the rest of the non-recursive chain runs once in its place; if bound to a key, equivalent to `ignore`\n", + "* toggle * toggle-all * toggle+down: ctrl-i tab * toggle-in: (--layout=reverse ? toggle+up: toggle+down) @@ -201,7 +209,8 @@ const ACTIONS_SS: &str = " * unix-word-rubout: ctrl-w * up: ctrl-k ctrl-p up * yank: ctrl-y -"; +", +); #[cfg(feature = "listen")] const REMOTE_SECTION: &str = " @@ -301,10 +310,13 @@ Exact search can be enabled by default by the `--exact` command-line flag. In ex section( &mut custom, "KEYBINDS", - " -Bindings can be set by the `--bind` option, which takes a comma-separated list of `:[+action2]` expressions. A trigger can be a key, a finder event, or an action name. -Actions can take arguments, specified either between parentheses `reload(ls)` or after a colon `reload:ls`. -", + concat!( + "\nBindings can be set by the `--bind` option, which takes a comma-separated list of ", + "`:[+action2]` expressions. A trigger can be a key, a finder event, or an action ", + "name.\n", + "Actions can take arguments, specified either between parentheses `reload(ls)` or after a colon ", + "`reload:ls`.\n", + ), ); subsection(&mut custom, "Available keys (aliases in parentheses)", KEYS_SS); subsection(&mut custom, "Bindable finder events", BINDABLE_EVENTS_SS); diff --git a/src/tui/app_tests.rs b/src/tui/app_tests.rs index 16499175..9bb6fb65 100644 --- a/src/tui/app_tests.rs +++ b/src/tui/app_tests.rs @@ -80,6 +80,11 @@ fn add_char_updates_query_and_emits_events() { let events = act(&mut app, Action::AddChar('x')); assert_eq!(app.input.value, "x"); // on_query_changed emits a `change` event key (SkimEvent::Change) and a RunPreview + assert!( + events + .iter() + .any(|event| matches!(event, Event::Key(key) if *key == crate::binds::SkimEvent::Change.key_event())) + ); assert!(events.iter().any(|e| matches!(e, Event::RunPreview))); }