diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e43aff4c6..0523fb62a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ For live updates on Fresh, [follow me on X](https://x.com/TheNoamLewis). * **Highlights taller than the window are drawn again.** * **Input** * **Shift works with "Keyboard Report All Keys As Escape Codes"** - `Shift+A` typed `a` (#2880, reported by @akarinotomoshibi). + * **`Ctrl+/` works outside kitty again** (#2933) - toggle-comment fired under kitty but nowhere else, because the `0x1F` byte every other terminal sends for the chord was read as `Ctrl+_`. It also reaches a terminal panel's child process as `0x1F` now, instead of a literal `/`. * **Pasting works in the New Workspace and Run Agent dialogs**, in daemon mode too. * **Settings & commands** * **Settings toggles actually stick** diff --git a/crates/fresh-editor/src/app/input.rs b/crates/fresh-editor/src/app/input.rs index ff4085def5..aefb708237 100644 --- a/crates/fresh-editor/src/app/input.rs +++ b/crates/fresh-editor/src/app/input.rs @@ -518,6 +518,62 @@ impl Editor { .expect("editor base layer always owns the keyboard") } + /// Handle a key press that a terminal reported, resolving which of its two + /// readings the keymap should see. + /// + /// A chord is both a physical key plus modifiers (`Ctrl+Shift+7`) and the + /// character that key types (`&` on a US layout, `/` on a German one). The + /// parser reports both when they disagree — see + /// [`fresh_input_parser::KeyPress`] — because neither is right on its own: + /// binding the physical chord leaves a German user's `Ctrl+/` firing + /// `set_bookmark` (sinelaw/fresh#2933), and binding the typed character + /// breaks every US `Ctrl+Shift+`. + /// + /// **The keymap decides.** The layout reading is tried first and used only + /// if something is actually bound to it; otherwise the physical chord is + /// handled exactly as before. So a US layout is unaffected — nothing binds + /// `ctrl+&`, so `Ctrl+Shift+7` still reaches `set_bookmark` — while a German + /// layout resolves the same keystroke to `ctrl+/`. + /// + /// One chord can only mean one thing, so this is a precedence, not a + /// merge: where a keymap binds both readings, the layout one wins and the + /// physical chord is unreachable from that key. A user who wants the other + /// way round rebinds it. + pub fn handle_key_press(&mut self, press: fresh_input_parser::KeyPress) -> AnyhowResult<()> { + let (code, modifiers) = self + .layout_reading(&press) + .unwrap_or((press.code, press.modifiers)); + self.handle_key(code, modifiers) + } + + /// The chord built from what the key types on this layout, if the keymap + /// binds it. `None` when there is no distinct layout character, or when + /// nothing is bound to it and the physical chord should be used instead. + fn layout_reading( + &self, + press: &fresh_input_parser::KeyPress, + ) -> Option<(crossterm::event::KeyCode, crossterm::event::KeyModifiers)> { + use crate::input::keybindings::Action; + use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + + let layout_char = press.layout_char?; + // Shift is spent producing the character, so it is not part of the + // chord built from it: the German `/` is `ctrl+/`, not `ctrl+shift+/`. + let modifiers = press.modifiers - KeyModifiers::SHIFT; + let code = KeyCode::Char(layout_char); + let action = self + .keybindings + .read() + .ok()? + .resolve(&KeyEvent::new(code, modifiers), self.get_key_context()); + // `InsertChar` is the resolver's "nothing bound, just type it" answer. + // Typing is the physical key's job, not this reading's. + match action { + Action::None | Action::InsertChar(_) => None, + _ => Some((code, modifiers)), + } + } + /// Handle a key event and return whether it was handled /// This is the central key handling logic used by both main.rs and tests pub fn handle_key( diff --git a/crates/fresh-editor/src/app/lifecycle.rs b/crates/fresh-editor/src/app/lifecycle.rs index 2340e9ea5d..e38cceccd0 100644 --- a/crates/fresh-editor/src/app/lifecycle.rs +++ b/crates/fresh-editor/src/app/lifecycle.rs @@ -303,18 +303,21 @@ impl Editor { /// disturbing the live cursor. /// /// Returns whether the editor wants the next frame redrawn. - pub fn handle_input_event(&mut self, event: crossterm::event::Event) -> anyhow::Result { + pub fn handle_input_event(&mut self, event: fresh_input_parser::Event) -> anyhow::Result { use crate::input::is_keystroke; - use crossterm::event::Event as Ev; + use fresh_input_parser::{Event as Ev, KeyPress}; match event { - Ev::Key(key_event) if is_keystroke(key_event.kind) => { - let key_code = format!("{:?}", key_event.code); - let modifiers = format!("{:?}", key_event.modifiers); + Ev::Key(press) if is_keystroke(press.kind) => { + let key_code = format!("{:?}", press.code); + let modifiers = format!("{:?}", press.modifiers); self.active_window_mut() .log_keystroke(&key_code, &modifiers); - let translated = self.key_translator().translate(key_event); - self.handle_key(translated.code, translated.modifiers)?; + // The calibration translator rewrites the physical chord only; + // the layout character rides along untouched, since it says + // what the key types rather than which chord arrived. + let translated = self.key_translator().translate(press.event); + self.handle_key_press(KeyPress::with_layout_char(translated, press.layout_char))?; // If `paste()` just took the async placeholder path, // skip the otherwise-automatic render for this // keystroke. The placeholder is sitting in the diff --git a/crates/fresh-editor/src/app/toggle_actions.rs b/crates/fresh-editor/src/app/toggle_actions.rs index bd1b19cf76..4812c53b86 100644 --- a/crates/fresh-editor/src/app/toggle_actions.rs +++ b/crates/fresh-editor/src/app/toggle_actions.rs @@ -434,9 +434,9 @@ impl Editor { /// normal input handling for it and re-render. This lives on `Editor` so /// every event loop (the local terminal loop in `main.rs` and the daemon /// server loop) shares one dismissal path rather than duplicating it. - pub fn maybe_dismiss_wave_animation(&mut self, event: &crossterm::event::Event) -> bool { + pub fn maybe_dismiss_wave_animation(&mut self, event: &fresh_input_parser::Event) -> bool { use crate::input::is_keystroke; - use crossterm::event::Event; + use fresh_input_parser::Event; if !self.wave_animation_active() { return false; } diff --git a/crates/fresh-editor/src/main.rs b/crates/fresh-editor/src/main.rs index 66187f0710..97e360678b 100644 --- a/crates/fresh-editor/src/main.rs +++ b/crates/fresh-editor/src/main.rs @@ -7,7 +7,7 @@ use windows_sys::Win32::System::Console::{AttachConsole, ATTACH_PARENT_PROCESS}; use anyhow::{Context, Result as AnyhowResult}; use clap::{CommandFactory, FromArgMatches, Parser}; -use crossterm::event::{poll as event_poll, read as event_read, Event as CrosstermEvent}; +use crossterm::event::{poll as event_poll, read as event_read}; use fresh::input::key_translator::KeyTranslator; #[cfg(target_os = "linux")] use fresh::services::gpm::{gpm_to_crossterm, GpmClient}; @@ -18,6 +18,7 @@ use fresh::{ services::release_checker, services::remote, services::signal_handler, services::tracing_setup::TracingHandles, workspace, }; +use fresh_input_parser::Event as InputEvent; use ratatui::Terminal; use std::{ io::{self, stdout}, @@ -5752,7 +5753,7 @@ fn run_event_loop( let reader = VtInputReader::spawn(); let mut input_parser = InputParser::new(); - let mut event_buffer: std::collections::VecDeque = + let mut event_buffer: std::collections::VecDeque = std::collections::VecDeque::new(); let result = run_event_loop_common( @@ -5761,7 +5762,7 @@ fn run_event_loop( workspace_enabled, key_translator, terminal_modes, - |timeout| -> AnyhowResult> { + |timeout| -> AnyhowResult> { // Return buffered events first if let Some(event) = event_buffer.pop_front() { return Ok(Some(event)); @@ -5787,16 +5788,16 @@ fn run_event_loop( } Some(VtInputEvent::Resize) => { if let Ok((cols, rows)) = crossterm::terminal::size() { - event_buffer.push_back(CrosstermEvent::Resize(cols, rows)); + event_buffer.push_back(InputEvent::Resize(cols, rows)); } got_any = true; } Some(VtInputEvent::FocusGained) => { - event_buffer.push_back(CrosstermEvent::FocusGained); + event_buffer.push_back(InputEvent::FocusGained); got_any = true; } Some(VtInputEvent::FocusLost) => { - event_buffer.push_back(CrosstermEvent::FocusLost); + event_buffer.push_back(InputEvent::FocusLost); got_any = true; } None => break, @@ -5860,7 +5861,7 @@ fn run_event_loop( /// consecutive recovered panics and, past a small threshold, surface an error /// so the caller shuts the loop down cleanly instead of busy-looping. Any /// successful read resets the counter. -fn safe_event_read() -> std::io::Result> { +fn safe_event_read() -> std::io::Result> { use std::panic::{catch_unwind, AssertUnwindSafe}; use std::sync::atomic::{AtomicU32, Ordering}; @@ -5870,7 +5871,7 @@ fn safe_event_read() -> std::io::Result> { match catch_unwind(AssertUnwindSafe(event_read)) { Ok(res) => { CONSECUTIVE_PANICS.store(0, Ordering::Relaxed); - res.map(Some) + res.map(|event| Some(InputEvent::from(event))) } Err(_) => { let n = CONSECUTIVE_PANICS.fetch_add(1, Ordering::Relaxed) + 1; @@ -5899,14 +5900,14 @@ fn run_event_loop_common( mut poll_event: F, ) -> AnyhowResult<()> where - F: FnMut(Duration) -> AnyhowResult>, + F: FnMut(Duration) -> AnyhowResult>, { use std::time::Instant; const FRAME_DURATION: Duration = Duration::from_millis(16); // 60fps let mut last_render = Instant::now(); let mut needs_render = true; - let mut pending_event: Option = None; + let mut pending_event: Option = None; // Time of the last real input event, used to start the wave-animation // screensaver after the configured idle period. Read from the editor's // injected time source so tests can drive idle time deterministically. @@ -6062,7 +6063,7 @@ where // Event debug dialog receives ALL RAW events (before any translation or processing) // This is essential for diagnosing terminal keybinding issues if editor.active_window().is_event_debug_active() { - if let CrosstermEvent::Key(key_event) = event { + if let InputEvent::Key(key_event) = &event { if fresh::input::is_keystroke(key_event.kind) { editor .active_window_mut() @@ -6080,7 +6081,7 @@ where // tracing spans + key-translation that depend on // event-loop-local state. let _span = match &event { - CrosstermEvent::Key(key_event) if fresh::input::is_keystroke(key_event.kind) => Some( + InputEvent::Key(key_event) if fresh::input::is_keystroke(key_event.kind) => Some( tracing::trace_span!( "handle_key", code = ?key_event.code, @@ -6104,7 +6105,7 @@ fn poll_with_gpm( reader: &mut fresh::services::tty_input::TtyReader, gpm_client: Option<&GpmClient>, timeout: Duration, -) -> AnyhowResult> { +) -> AnyhowResult> { use nix::poll::{poll, PollFd, PollFlags, PollTimeout}; use std::os::unix::io::{AsRawFd, BorrowedFd}; @@ -6158,7 +6159,7 @@ fn poll_with_gpm( match gpm.read_event() { Ok(Some(gpm_event)) => { if let Some(mouse_event) = gpm_to_crossterm(&gpm_event) { - return Ok(Some(CrosstermEvent::Mouse(mouse_event))); + return Ok(Some(InputEvent::Mouse(mouse_event))); } else { tracing::debug!("GPM event could not be converted to crossterm event"); } @@ -6183,13 +6184,11 @@ fn poll_with_gpm( /// Skip stale mouse move events, return the latest one. /// If we read a non-move event while draining, return it as pending. -fn coalesce_mouse_moves( - event: CrosstermEvent, -) -> AnyhowResult<(CrosstermEvent, Option)> { +fn coalesce_mouse_moves(event: InputEvent) -> AnyhowResult<(InputEvent, Option)> { use crossterm::event::MouseEventKind; // Only coalesce mouse moves - if !matches!(&event, CrosstermEvent::Mouse(m) if m.kind == MouseEventKind::Moved) { + if !matches!(&event, InputEvent::Mouse(m) if m.kind == MouseEventKind::Moved) { return Ok((event, None)); } @@ -6206,7 +6205,7 @@ fn coalesce_mouse_moves( let Some(next) = safe_event_read()? else { continue; }; - if matches!(&next, CrosstermEvent::Mouse(m) if m.kind == MouseEventKind::Moved) { + if matches!(&next, InputEvent::Mouse(m) if m.kind == MouseEventKind::Moved) { latest = next; // Newer move, skip the old one } else { return Ok((latest, Some(next))); // Hit a click/key, save it diff --git a/crates/fresh-editor/src/server/editor_server.rs b/crates/fresh-editor/src/server/editor_server.rs index 4f090cfd0e..c362c49c09 100644 --- a/crates/fresh-editor/src/server/editor_server.rs +++ b/crates/fresh-editor/src/server/editor_server.rs @@ -12,7 +12,7 @@ use std::sync::mpsc; use std::sync::Arc; use std::time::{Duration, Instant}; -use crossterm::event::Event; +use fresh_input_parser::Event; use ratatui::Terminal; use crate::app::Editor; @@ -1533,10 +1533,10 @@ impl EditorServer { } match event { - Event::Key(key_event) => { - if crate::input::is_keystroke(key_event.kind) { + Event::Key(press) => { + if crate::input::is_keystroke(press.kind) { editor - .handle_key(key_event.code, key_event.modifiers) + .handle_key_press(press) .map_err(|e| io::Error::other(e.to_string()))?; Ok(true) } else { @@ -1684,8 +1684,9 @@ mod wave_dismiss_tests { use crate::config::Config; use crate::config_io::DirectoryContext; use crossterm::event::{ - Event, KeyCode, KeyEvent, KeyModifiers, MouseButton, MouseEvent, MouseEventKind, + KeyCode, KeyEvent, KeyModifiers, MouseButton, MouseEvent, MouseEventKind, }; + use fresh_input_parser::Event; use std::sync::Arc; use std::time::Duration; @@ -1853,7 +1854,7 @@ mod wave_dismiss_tests { ); let consumed = server - .handle_event(Event::Key(KeyEvent::new( + .handle_event(Event::key(KeyEvent::new( KeyCode::Char('a'), KeyModifiers::empty(), ))) diff --git a/crates/fresh-editor/src/server/input_parser.rs b/crates/fresh-editor/src/server/input_parser.rs index b1f6e60a76..e34c34363f 100644 --- a/crates/fresh-editor/src/server/input_parser.rs +++ b/crates/fresh-editor/src/server/input_parser.rs @@ -10,9 +10,7 @@ use std::time::{Duration, Instant}; -use crossterm::event::Event; - -pub use fresh_input_parser::InputParser; +pub use fresh_input_parser::{Event, InputParser, KeyPress}; /// How long a lone `ESC` stays buffered before it is resolved as the Escape /// key. Mirrors the tty reader's grace window: long enough that a control diff --git a/crates/fresh-editor/src/server/runner.rs b/crates/fresh-editor/src/server/runner.rs index d2bf15b4c9..649e75e52f 100644 --- a/crates/fresh-editor/src/server/runner.rs +++ b/crates/fresh-editor/src/server/runner.rs @@ -18,7 +18,7 @@ use crate::server::ipc::{ServerConnection, ServerListener, SocketPaths}; use crate::server::protocol::{ ClientControl, ServerControl, ServerHello, TermSize, VersionMismatch, PROTOCOL_VERSION, }; -use crossterm::event::Event; +use fresh_input_parser::Event; /// Server configuration #[derive(Debug, Clone)] diff --git a/crates/fresh-editor/src/services/terminal/pty.rs b/crates/fresh-editor/src/services/terminal/pty.rs index 2d5f096cd0..f778e8f5aa 100644 --- a/crates/fresh-editor/src/services/terminal/pty.rs +++ b/crates/fresh-editor/src/services/terminal/pty.rs @@ -133,7 +133,11 @@ fn control_byte(c: char) -> Option { '\\' | '4' => 0x1c, ']' | '5' => 0x1d, '^' | '6' => 0x1e, - '_' | '7' => 0x1f, + // Ctrl+/ is the same `US` byte as Ctrl+_ and Ctrl+7. Without the '/' + // arm the chord fell through to the plain-character path and the child + // saw a literal `/` — which is what a kitty-protocol terminal, where + // the chord arrives as Ctrl+/ rather than Ctrl+_, hit. + '_' | '7' | '/' => 0x1f, '@' | '2' => 0x00, // NUL ' ' => 0x00, // Ctrl+Space = NUL '?' => 0x7f, // DEL @@ -373,6 +377,33 @@ mod tests { assert_eq!(seq(KeyCode::Up, alt), "\x1b[1;3A"); } + /// Every spelling of the `US` chord has to reach the child as 0x1F. `/` was + /// missing from the table, so Ctrl+/ — the way the chord arrives from a + /// kitty-protocol terminal, and now from the legacy path too — fell through + /// to the plain-character branch and the child saw a bare `/`. + #[test] + fn ctrl_slash_reaches_the_child_as_us() { + let ctrl = KeyModifiers::CONTROL; + + for key in ['/', '_', '7'] { + assert_eq!( + key_to_pty_bytes(KeyCode::Char(key), ctrl, false), + Some(vec![0x1f]), + "Ctrl+{key} should send 0x1F" + ); + } + + // Ctrl+Alt is deliberately not asserted: it is the one part of this + // encoding that varies by platform, since Windows reports AltGr as + // Ctrl+Alt and routes it to the plain-character path instead. + + // Without Ctrl it is still an ordinary slash. + assert_eq!( + key_to_pty_bytes(KeyCode::Char('/'), KeyModifiers::empty(), false), + Some(vec![b'/']) + ); + } + /// Home/End/PageUp/PageDown/Insert accepted only Ctrl (or nothing at all), /// so every other modifier on them was silently dropped. #[test] diff --git a/crates/fresh-editor/src/services/tty_input.rs b/crates/fresh-editor/src/services/tty_input.rs index f863e14706..9f97f1dee2 100644 --- a/crates/fresh-editor/src/services/tty_input.rs +++ b/crates/fresh-editor/src/services/tty_input.rs @@ -21,15 +21,15 @@ //! *input* side moves here. Focus (`ESC[I`/`O`) and bracketed paste //! (`ESC[200~`…`201~`) arrive in the byte stream and are decoded by //! `InputParser`; terminal resizes do not, so we install our own `SIGWINCH` -//! handler and synthesize [`CrosstermEvent::Resize`]. +//! handler and synthesize [`InputEvent::Resize`]. use std::collections::VecDeque; use std::os::unix::io::{AsRawFd, BorrowedFd, RawFd}; use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; -use crossterm::event::{Event as CrosstermEvent, MouseEventKind}; -use fresh_input_parser::InputParser; +use crossterm::event::MouseEventKind; +use fresh_input_parser::{Event as InputEvent, InputParser}; /// How long a buffered lone `ESC` waits for a continuation before it is /// resolved as the Escape key. This bounds two waits: the in-`drain_stdin` @@ -99,7 +99,7 @@ fn poll_readable(fd: RawFd, timeout: Duration) -> bool { /// Streaming reader that converts raw stdin bytes into crossterm events. pub struct TtyReader { parser: InputParser, - queue: VecDeque, + queue: VecDeque, stdin_fd: RawFd, } @@ -116,18 +116,18 @@ impl TtyReader { } /// Return a pending resize event if a `SIGWINCH` fired since last checked. - pub fn take_resize(&self) -> Option { + pub fn take_resize(&self) -> Option { if SIGWINCH_PENDING.swap(false, Ordering::Relaxed) { crossterm::terminal::size() .ok() - .map(|(cols, rows)| CrosstermEvent::Resize(cols, rows)) + .map(|(cols, rows)| InputEvent::Resize(cols, rows)) } else { None } } /// Pop the next already-decoded event, if any. - pub fn next_buffered(&mut self) -> Option { + pub fn next_buffered(&mut self) -> Option { self.queue.pop_front() } @@ -194,10 +194,10 @@ impl TtyReader { /// Queue an event, collapsing a run of mouse-move events down to the latest /// one (a motion flood produces one Moved event per read batch), matching /// the coalescing the crossterm path did in `coalesce_mouse_moves`. - fn push_coalesced(&mut self, ev: CrosstermEvent) { - if let CrosstermEvent::Mouse(m) = &ev { + fn push_coalesced(&mut self, ev: InputEvent) { + if let InputEvent::Mouse(m) = &ev { if m.kind == MouseEventKind::Moved { - if let Some(CrosstermEvent::Mouse(last)) = self.queue.back() { + if let Some(InputEvent::Mouse(last)) = self.queue.back() { if last.kind == MouseEventKind::Moved { *self.queue.back_mut().expect("back() was Some") = ev; return; @@ -209,7 +209,7 @@ impl TtyReader { } /// Blocking (up to `timeout`) read of the next event, or `None` on timeout. - pub fn poll(&mut self, timeout: Duration) -> anyhow::Result> { + pub fn poll(&mut self, timeout: Duration) -> anyhow::Result> { if let Some(ev) = self.next_buffered() { return Ok(Some(ev)); } @@ -238,7 +238,7 @@ impl TtyReader { /// Non-blocking peek at the next event: drains stdin once if data is already /// pending. Used by mouse-move coalescing to look ahead without blocking. - pub fn try_next(&mut self) -> Option { + pub fn try_next(&mut self) -> Option { if let Some(ev) = self.next_buffered() { return Some(ev); } @@ -302,7 +302,7 @@ mod tests { } } - fn drain_events(r: &mut TtyReader) -> Vec { + fn drain_events(r: &mut TtyReader) -> Vec { let mut out = Vec::new(); while let Some(ev) = r.next_buffered() { out.push(ev); @@ -339,7 +339,7 @@ mod tests { "expected exactly one event, got {events:?}", ); assert!( - matches!(events[0], CrosstermEvent::Mouse(_)), + matches!(events[0], InputEvent::Mouse(_)), "expected a single Mouse event, got {:?}", events[0], ); @@ -366,7 +366,7 @@ mod tests { assert!( matches!( events[0], - CrosstermEvent::Key(k) if k.code == KeyCode::Esc, + InputEvent::Key(k) if k.code == KeyCode::Esc, ), "expected Esc key, got {:?}", events[0], diff --git a/crates/fresh-editor/src/webui/mod.rs b/crates/fresh-editor/src/webui/mod.rs index 43bd16aaaf..f980f18285 100644 --- a/crates/fresh-editor/src/webui/mod.rs +++ b/crates/fresh-editor/src/webui/mod.rs @@ -97,9 +97,10 @@ use std::sync::Arc; use std::time::{Duration, Instant}; use anyhow::Result; -use crossterm::event::{ - Event, KeyCode, KeyEvent, KeyModifiers, MouseButton, MouseEvent, MouseEventKind, -}; +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers, MouseButton, MouseEvent, MouseEventKind}; +// The browser sends a chord, never a keyboard layout's shifted codepoint, so +// the web path builds plain key presses (`Event::key`) with no layout char. +use fresh_input_parser::Event; use ratatui::backend::TestBackend; use ratatui::buffer::Buffer; use ratatui::layout::{Position, Rect}; @@ -2134,7 +2135,7 @@ fn apply_key(editor: &mut Editor, v: &Value) { // the daemon server loop): ANY key press dismisses the interactive wave // and is CONSUMED — it only stops the show, it doesn't also act on the // editor. `KeyEvent::new` sets kind=Press, which the dismissal requires. - if editor.maybe_dismiss_wave_animation(&Event::Key(KeyEvent::new(code, mods))) { + if editor.maybe_dismiss_wave_animation(&Event::key(KeyEvent::new(code, mods))) { return; } if let Err(e) = editor.handle_key(code, mods) { diff --git a/crates/fresh-editor/tests/common/harness.rs b/crates/fresh-editor/tests/common/harness.rs index 5799fc5ca7..6dad28f03f 100644 --- a/crates/fresh-editor/tests/common/harness.rs +++ b/crates/fresh-editor/tests/common/harness.rs @@ -1450,7 +1450,20 @@ impl EditorTestHarness { /// must use this; `send_key` cannot express a release. pub fn send_key_event(&mut self, key_event: crossterm::event::KeyEvent) -> anyhow::Result<()> { self.editor - .handle_input_event(crossterm::event::Event::Key(key_event))?; + .handle_input_event(fresh::server::input_parser::Event::key(key_event))?; + self.drain_async_work(); + self.render()?; + Ok(()) + } + + /// Deliver a full [`KeyPress`], so a test can supply the keyboard-layout + /// character a non-US terminal reports alongside the physical chord. + pub fn send_key_press( + &mut self, + press: fresh::server::input_parser::KeyPress, + ) -> anyhow::Result<()> { + self.editor + .handle_input_event(fresh::server::input_parser::Event::Key(press))?; self.drain_async_work(); self.render()?; Ok(()) @@ -1458,7 +1471,7 @@ impl EditorTestHarness { pub fn send_paste(&mut self, text: &str) -> anyhow::Result<()> { self.editor - .handle_input_event(crossterm::event::Event::Paste(text.to_string()))?; + .handle_input_event(fresh::server::input_parser::Event::Paste(text.to_string()))?; self.drain_async_work(); self.render()?; Ok(()) diff --git a/crates/fresh-editor/tests/e2e/csi_u_session_input.rs b/crates/fresh-editor/tests/e2e/csi_u_session_input.rs index 34d9595d41..d4a19883a0 100644 --- a/crates/fresh-editor/tests/e2e/csi_u_session_input.rs +++ b/crates/fresh-editor/tests/e2e/csi_u_session_input.rs @@ -9,7 +9,8 @@ //! literal text into the editor buffer. use crate::common::harness::EditorTestHarness; -use crossterm::event::{Event, KeyCode, KeyModifiers}; +use crossterm::event::{KeyCode, KeyModifiers}; +use fresh::server::input_parser::Event; use fresh::server::input_parser::InputParser; /// Helper: assert that InputParser produces exactly one Key event matching the diff --git a/crates/fresh-editor/tests/e2e/ctrl_slash_legacy_terminal.rs b/crates/fresh-editor/tests/e2e/ctrl_slash_legacy_terminal.rs new file mode 100644 index 0000000000..18c3e83680 --- /dev/null +++ b/crates/fresh-editor/tests/e2e/ctrl_slash_legacy_terminal.rs @@ -0,0 +1,123 @@ +//! Regression tests for sinelaw/fresh#2933: `Ctrl+/` did nothing on any +//! terminal without the kitty keyboard protocol. +//! +//! Those terminals have no CSI-u form for the chord — they send the bare `US` +//! byte (0x1F), which the parser reported as `Ctrl+_`. Nothing connected that +//! back to the `ctrl+/` binding, so toggle-comment fired under kitty and +//! nowhere else. +//! +//! These drive the raw bytes a real terminal would send through `InputParser` +//! and assert on what ends up rendered, so they cover the whole path +//! (bytes → key event → binding → edit → screen) rather than any one layer. + +use crate::common::harness::{EditorTestHarness, HarnessOptions}; +use fresh::config::Config; +use fresh::server::input_parser::{Event, InputParser}; +use tempfile::TempDir; + +/// Feed raw terminal bytes through the parser into the editor, exactly as +/// session mode does on the server side. +fn send_bytes(harness: &mut EditorTestHarness, bytes: &[u8]) { + let mut parser = InputParser::new(); + for event in parser.parse(bytes) { + if let Event::Key(press) = event { + harness.send_key_press(press).unwrap(); + } + } +} + +/// Open a Rust file (`//` line comments) on its own temp dir, so the tests stay +/// isolated from each other and from the host. +fn harness_with_rust_file() -> (TempDir, EditorTestHarness) { + let temp_dir = TempDir::new().unwrap(); + let file_path = temp_dir.path().join("test.rs"); + std::fs::write(&file_path, "fn main() {}\n").unwrap(); + + let mut config = Config::default(); + // Pin the "default" keymap: `Config::default()` selects `macos` on macOS, + // which binds this chord differently. + config.active_keybinding_map = fresh::config::KeybindingMapName("default".to_string()); + + let mut harness = + EditorTestHarness::create(80, 24, HarnessOptions::new().with_config(config)).unwrap(); + harness.open_file(&file_path).unwrap(); + harness.render().unwrap(); + (temp_dir, harness) +} + +/// The bug as reported: on a terminal with no kitty protocol, pressing Ctrl+/ +/// sends 0x1F and must comment the line. +#[test] +fn ctrl_slash_toggles_comment_from_a_legacy_terminal_byte() { + let (_temp_dir, mut harness) = harness_with_rust_file(); + harness.assert_screen_contains("fn main() {}"); + + send_bytes(&mut harness, &[0x1f]); + harness.render().unwrap(); + + harness.assert_screen_contains("// fn main() {}"); +} + +/// The same chord under the kitty keyboard protocol, which already worked — +/// kept as the control that says both encodings now land on one binding. +#[test] +fn ctrl_slash_toggles_comment_from_the_kitty_encoding() { + let (_temp_dir, mut harness) = harness_with_rust_file(); + + send_bytes(&mut harness, b"\x1b[47;5u"); + harness.render().unwrap(); + + harness.assert_screen_contains("// fn main() {}"); +} + +/// The chord is a toggle, so the same byte twice must comment and then +/// uncomment — proving the second press resolves to the binding as well, not +/// just the first. +#[test] +fn the_legacy_byte_round_trips_the_comment() { + let (_temp_dir, mut harness) = harness_with_rust_file(); + + send_bytes(&mut harness, &[0x1f]); + harness.render().unwrap(); + harness.assert_screen_contains("// fn main() {}"); + + send_bytes(&mut harness, &[0x1f]); + harness.render().unwrap(); + harness.assert_screen_not_contains("// fn main() {}"); + harness.assert_screen_contains("fn main() {}"); +} + +// ---- The non-US half of the same issue ---- +// +// On a layout where `/` needs Shift (German, French, Spanish, …) there is no +// `Ctrl+/` for a terminal to report. kitty sends the physical chord plus the +// character it types — `CSI 55:47;6u`: base 55 (`7`), shifted 47 (`/`), +// Ctrl+Shift — and `default.json` binds `ctrl+shift+7` to `set_bookmark`, so +// the chord users press for "comment this line" silently set a bookmark. + +/// A German keyboard's Ctrl+/ must comment the line, exactly as a US one does. +#[test] +fn ctrl_slash_toggles_comment_on_a_layout_where_slash_needs_shift() { + let (_temp_dir, mut harness) = harness_with_rust_file(); + harness.assert_screen_contains("fn main() {}"); + + send_bytes(&mut harness, b"\x1b[55:47;6u"); + harness.render().unwrap(); + + harness.assert_screen_contains("// fn main() {}"); +} + +/// …and the US reading of the very same physical chord is untouched: Shift+7 +/// types `&` there, nothing binds `ctrl+&`, so `Ctrl+Shift+7` still reaches +/// `set_bookmark` rather than commenting the line. This is the test that says +/// the layout reading is a fallback, not a rewrite. +#[test] +fn the_same_chord_on_a_us_layout_still_reaches_its_digit_binding() { + let (_temp_dir, mut harness) = harness_with_rust_file(); + + send_bytes(&mut harness, b"\x1b[55:38;6u"); + harness.render().unwrap(); + + harness.assert_screen_not_contains("// fn main() {}"); + harness.assert_screen_contains("fn main() {}"); +} diff --git a/crates/fresh-editor/tests/e2e/issue_2796_key_release_duplicates.rs b/crates/fresh-editor/tests/e2e/issue_2796_key_release_duplicates.rs index 31f3d48f41..1a68c90629 100644 --- a/crates/fresh-editor/tests/e2e/issue_2796_key_release_duplicates.rs +++ b/crates/fresh-editor/tests/e2e/issue_2796_key_release_duplicates.rs @@ -13,15 +13,15 @@ //! assert only on what the status bar shows. use crate::common::harness::EditorTestHarness; -use crossterm::event::Event; +use fresh::server::input_parser::Event; use fresh::server::input_parser::InputParser; /// Feed raw terminal bytes through the same parser the event loops use, and /// dispatch whatever events they produce into the editor. fn feed(harness: &mut EditorTestHarness, parser: &mut InputParser, bytes: &[u8]) { for event in parser.parse(bytes) { - if let Event::Key(key_event) = event { - harness.send_key_event(key_event).unwrap(); + if let Event::Key(press) = event { + harness.send_key_press(press).unwrap(); } } } diff --git a/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs b/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs index ee4d56632f..95d1e3f4cd 100644 --- a/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs +++ b/crates/fresh-editor/tests/e2e/issue_2810_session_escape_flush.rs @@ -29,8 +29,8 @@ //! the test fails at the first wait. use crate::common::harness::{copy_plugin, copy_plugin_lib, EditorTestHarness}; -use crossterm::event::Event; use fresh::server::input_parser::ClientInputParser; +use fresh::server::input_parser::Event; use std::fs; use std::time::{Duration, Instant}; diff --git a/crates/fresh-editor/tests/e2e/mod.rs b/crates/fresh-editor/tests/e2e/mod.rs index ca0a40be2e..dfd4c2e107 100644 --- a/crates/fresh-editor/tests/e2e/mod.rs +++ b/crates/fresh-editor/tests/e2e/mod.rs @@ -24,6 +24,7 @@ pub mod copy_buffer_path; pub mod crash_repro; pub mod csharp_language_coherence; pub mod csi_u_session_input; +pub mod ctrl_slash_legacy_terminal; pub mod cursor_style_rendering; pub mod dabbrev_completion; #[cfg(feature = "plugins")] diff --git a/crates/fresh-editor/tests/e2e/mouse_session_input.rs b/crates/fresh-editor/tests/e2e/mouse_session_input.rs index ae48d09091..e069320093 100644 --- a/crates/fresh-editor/tests/e2e/mouse_session_input.rs +++ b/crates/fresh-editor/tests/e2e/mouse_session_input.rs @@ -14,7 +14,8 @@ //! into the editor and check nothing leaked into the buffer. use crate::common::harness::EditorTestHarness; -use crossterm::event::{Event, KeyCode}; +use crossterm::event::KeyCode; +use fresh::server::input_parser::Event; use fresh::server::input_parser::InputParser; /// Structural bytes of a mouse/CSI sequence — the characters that leaked in diff --git a/crates/fresh-input-parser/src/lib.rs b/crates/fresh-input-parser/src/lib.rs index 8ea49ff0ce..2bb1a337f6 100644 --- a/crates/fresh-input-parser/src/lib.rs +++ b/crates/fresh-input-parser/src/lib.rs @@ -1,6 +1,7 @@ //! Incremental terminal input (VT/ANSI) parser. //! -//! Turns a raw byte stream from a terminal into [`crossterm::event::Event`]s. +//! Turns a raw byte stream from a terminal into [`Event`]s (crossterm's, plus +//! the keyboard-layout information a keymap needs — see [`KeyPress`]). //! The editor runs the whole UI server-side and keeps the client ultra-light, //! so all input parsing happens here on a byte stream that arrives in //! arbitrarily-sized chunks (a single escape sequence is regularly split @@ -36,10 +37,131 @@ //! //! [williams]: https://vt100.net/emu/dec_ansi_parser +use std::ops::Deref; + use crossterm::event::{ - Event, KeyCode, KeyEvent, KeyEventKind, KeyModifiers, MouseButton, MouseEvent, MouseEventKind, + KeyCode, KeyEvent, KeyEventKind, KeyModifiers, MouseButton, MouseEvent, MouseEventKind, }; +/// A key press, as the terminal reported it, plus the character the key +/// actually types on the user's keyboard layout when the chord's own spelling +/// hides it. +/// +/// A chord is described by two different things at once, and on a US layout +/// they happen to coincide so nothing forces the distinction: +/// +/// * the **physical** key plus its modifiers — `Ctrl+Shift+7`; and +/// * the **character the key produces** in that state — `&` on US, `/` on +/// German (where `/` is Shift+7). +/// +/// Terminals report both, and neither one alone is enough. The kitty keyboard +/// protocol sends `CSI 55:47;6u` for the German chord: base codepoint 55 +/// (`7`), shifted codepoint 47 (`/`), modifiers Ctrl+Shift. Reporting only the +/// base means a German user pressing what is, to them, `Ctrl+/` gets +/// `Ctrl+Shift+7` — which `default.json` binds to `set_bookmark`, so the chord +/// silently set a bookmark instead of toggling a comment (sinelaw/fresh#2933). +/// Reporting only the shifted character is no better: it would break US +/// `Ctrl+Shift+` bindings, which are keyed on the digit. +/// +/// So the parser reports the physical chord and hangs the layout character off +/// the side, and the *keymap* decides which reading wins — see +/// `Editor::handle_key_press`, which prefers the layout reading only when it is +/// actually bound. `layout_char` is `None` whenever the two readings agree, +/// which is every key on a US layout and most keys everywhere else. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct KeyPress { + /// The chord as the terminal reported it: the physical key and its + /// modifiers, with SHIFT still set. + pub event: KeyEvent, + /// The character the key types on the current layout, when that differs + /// from `event.code`. The modifier that produced it (Shift) is spent on + /// the character and must not be part of a chord built from it. + pub layout_char: Option, +} + +impl KeyPress { + /// A key press whose physical spelling is the only reading there is. + pub fn new(event: KeyEvent) -> Self { + Self { + event, + layout_char: None, + } + } + + /// A key press that also types `layout_char` on the current layout. + pub fn with_layout_char(event: KeyEvent, layout_char: Option) -> Self { + Self { event, layout_char } + } +} + +impl From for KeyPress { + fn from(event: KeyEvent) -> Self { + Self::new(event) + } +} + +/// Reading a `KeyPress` as the key event it wraps keeps every existing +/// `press.code` / `press.modifiers` / `press.kind` call site working: the +/// layout character is extra information about the same press, not a different +/// kind of press. +impl Deref for KeyPress { + type Target = KeyEvent; + + fn deref(&self) -> &KeyEvent { + &self.event + } +} + +/// A terminal input event. +/// +/// Mirrors [`crossterm::event::Event`] variant for variant, except that `Key` +/// carries a [`KeyPress`] rather than a bare `KeyEvent` so the layout character +/// survives the trip to the keymap. Convert with [`Event::into_crossterm`] at +/// any boundary that only speaks crossterm. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Event { + FocusGained, + FocusLost, + Key(KeyPress), + Mouse(MouseEvent), + Paste(String), + Resize(u16, u16), +} + +impl Event { + /// A key event with no separate layout character. + pub fn key(event: KeyEvent) -> Self { + Event::Key(KeyPress::new(event)) + } + + /// Drop the layout character and hand back a plain crossterm event. + pub fn into_crossterm(self) -> crossterm::event::Event { + use crossterm::event::Event as C; + match self { + Event::FocusGained => C::FocusGained, + Event::FocusLost => C::FocusLost, + Event::Key(press) => C::Key(press.event), + Event::Mouse(mouse) => C::Mouse(mouse), + Event::Paste(text) => C::Paste(text), + Event::Resize(w, h) => C::Resize(w, h), + } + } +} + +impl From for Event { + fn from(event: crossterm::event::Event) -> Self { + use crossterm::event::Event as C; + match event { + C::FocusGained => Event::FocusGained, + C::FocusLost => Event::FocusLost, + C::Key(key) => Event::key(key), + C::Mouse(mouse) => Event::Mouse(mouse), + C::Paste(text) => Event::Paste(text), + C::Resize(w, h) => Event::Resize(w, h), + } + } +} + /// Bracketed-paste end marker. const PASTE_END: &[u8] = b"\x1b[201~"; @@ -161,7 +283,7 @@ impl InputParser { return Vec::new(); } self.state = State::Ground; - vec![Event::Key(KeyEvent::new( + vec![Event::key(KeyEvent::new( KeyCode::Esc, KeyModifiers::empty(), ))] @@ -266,7 +388,7 @@ impl InputParser { } 0x1b => { // First ESC was standalone; stay in Escape for the second one. - out.push(Event::Key(KeyEvent::new( + out.push(Event::key(KeyEvent::new( KeyCode::Esc, KeyModifiers::empty(), ))); @@ -291,7 +413,7 @@ impl InputParser { // Dropping the Control bit made the two indistinguishable, so // Alt+Ctrl+P silently ran the (bound) Alt+P command instead of // resolving to an unbound chord — sinelaw/fresh#2810. - out.push(Event::Key(KeyEvent::new( + out.push(Event::key(KeyEvent::new( byte_to_keycode(other), c0_control_modifier(other) | KeyModifiers::ALT, ))); @@ -382,7 +504,7 @@ impl InputParser { _ => None, }; if let Some(code) = keycode { - out.push(Event::Key(KeyEvent::new(code, KeyModifiers::empty()))); + out.push(Event::key(KeyEvent::new(code, KeyModifiers::empty()))); } else { tracing::trace!("InputParser: unknown SS3 final {:#04x}, dropping", byte); } @@ -454,7 +576,7 @@ impl InputParser { match std::str::from_utf8(&self.buffer) { Ok(s) => { if let Some(c) = s.chars().next() { - out.push(Event::Key(KeyEvent::new(KeyCode::Char(c), modifiers))); + out.push(Event::key(KeyEvent::new(KeyCode::Char(c), modifiers))); } } Err(_) => { @@ -608,7 +730,7 @@ impl InputParser { // Shift+Tab. The event type still rides in the modifier field for // emulators that send one here, so decode it the same way; the // modifiers are fixed because the key *is* the shifted Tab. - b'Z' => out.push(Event::Key(KeyEvent::new_with_kind( + b'Z' => out.push(Event::key(KeyEvent::new_with_kind( KeyCode::BackTab, KeyModifiers::SHIFT, kind_of(¶ms), @@ -633,7 +755,7 @@ impl InputParser { let codepoint: u32 = first_subparam(parts[2]).parse().unwrap_or(0); let modifiers = modifiers_from_param(mods_param); if let Some(code) = functional_or_char(codepoint) { - out.push(Event::Key(KeyEvent::new(code, modifiers))); + out.push(Event::key(KeyEvent::new(code, modifiers))); } return; } @@ -654,7 +776,7 @@ impl InputParser { } // Stray paste-end outside paste mode: ignore gracefully. if num == 201 { - out.push(Event::Key(KeyEvent::new( + out.push(Event::key(KeyEvent::new( KeyCode::Null, KeyModifiers::empty(), ))); @@ -687,7 +809,7 @@ impl InputParser { return; } }; - out.push(Event::Key(KeyEvent::new_with_kind( + out.push(Event::key(KeyEvent::new_with_kind( keycode, modifiers, kind, ))); } @@ -710,11 +832,20 @@ impl InputParser { // A shifted key must arrive as the character it types, exactly as // it does over the legacy encoding — the case (or symbol) carries // the shift, so the modifier bit goes with it. + let mut layout_char = None; if let Some(shifted) = shifted_char(key_field, code, modifiers) { code = KeyCode::Char(shifted); modifiers.remove(KeyModifiers::SHIFT); + } else { + // The chord keeps its physical spelling, which on a non-US + // layout can hide the character the user thinks they pressed. + // Carry that character alongside so the keymap can consider it. + layout_char = hidden_layout_char(key_field, code, modifiers); } - out.push(Event::Key(KeyEvent::new_with_kind(code, modifiers, kind))); + out.push(Event::Key(KeyPress::with_layout_char( + KeyEvent::new_with_kind(code, modifiers, kind), + layout_char, + ))); } } } @@ -743,7 +874,7 @@ fn event_kind_of(mods_field: &str) -> KeyEventKind { /// reporting every release as a press) is what made one arrow keypress move the /// cursor twice (sinelaw/fresh#2796). fn csi_key(code: KeyCode, params: &[u8]) -> Event { - Event::Key(KeyEvent::new_with_kind( + Event::key(KeyEvent::new_with_kind( code, modifiers_of(params), kind_of(params), @@ -801,6 +932,40 @@ fn shifted_char(key_field: &str, code: KeyCode, modifiers: KeyModifiers) -> Opti } } +/// The character a key types on the current layout, when the chord's own +/// spelling does not say so — the [`KeyPress::layout_char`] of a CSI-u report. +/// +/// This is the leftover of [`shifted_char`]: the terminal told us what the key +/// types, but the chord has to keep its base spelling anyway because CONTROL is +/// held (folding the shifted character in there would collapse `Ctrl+Shift+A` +/// onto `Ctrl+A`). On a US layout that discarded character is the one the +/// binding never wanted — `Ctrl+Shift+7` types `&`, and the binding is on the +/// digit. On a German layout it is the whole point: `Ctrl+Shift+7` types `/`, +/// and the user pressed it meaning `Ctrl+/`. +/// +/// Both readings are therefore kept and the keymap picks; see [`KeyPress`]. +/// `None` when the terminal reported no shifted codepoint (no +/// `REPORT_ALTERNATE_KEYS`, nothing to disagree about) or when it matches the +/// base, which is the US case for every letter. +fn hidden_layout_char(key_field: &str, code: KeyCode, modifiers: KeyModifiers) -> Option { + if !modifiers.contains(KeyModifiers::SHIFT) { + return None; + } + let KeyCode::Char(base) = code else { + return None; + }; + match functional_or_char(shifted_subparam(key_field)?)? { + // A letter's shifted codepoint is only its uppercase, which the chord's + // own SHIFT bit already says — `ctrl+A` and `ctrl+shift+a` are one + // binding. Reporting it would put every shifted keystroke through a + // second keymap lookup that can only find the same thing. + KeyCode::Char(shifted) if shifted != base && uppercase_letter(base) != Some(shifted) => { + Some(shifted) + } + _ => None, + } +} + /// The shifted codepoint of a kitty `unicode:shifted:base` key field, if the /// terminal reported one. An empty sub-parameter (`97::29`, the way a key field /// carries a base-layout code with no shifted code) is "not reported". @@ -1154,7 +1319,7 @@ fn c0_control_modifier(byte: u8) -> KeyModifiers { /// C0 control characters (except Tab, LF, CR and Esc, which are their own /// keys). fn byte_to_event(byte: u8) -> Event { - Event::Key(KeyEvent::new( + Event::key(KeyEvent::new( byte_to_keycode(byte), c0_control_modifier(byte), )) @@ -1168,7 +1333,16 @@ fn byte_to_keycode(byte: u8) -> KeyCode { 10 | 13 => KeyCode::Enter, // LF or CR 1..=26 => KeyCode::Char((b'a' + byte - 1) as char), // Ctrl+A..Ctrl+Z 27 => KeyCode::Esc, - 28..=31 => KeyCode::Char((b'\\' + byte - 28) as char), + 28..=30 => KeyCode::Char((b'\\' + byte - 28) as char), // Ctrl+\, Ctrl+], Ctrl+^ + // `US`. A legacy terminal sends 0x1F for Ctrl+/, Ctrl+7 and Ctrl+_ + // alike, so the parser has to pick one spelling for the chord. It picks + // `/` because that is the key people actually press for it (no Shift + // needed) and because it is what the same chord reports under the kitty + // protocol (`CSI 47;5u`), so both kinds of terminal resolve to one + // binding. Deriving this arithmetically with 0x1C–0x1E gave `Ctrl+_`, + // which no keymap binds, so `ctrl+/` fired on kitty and nowhere else + // (sinelaw/fresh#2933). + 31 => KeyCode::Char('/'), 32 => KeyCode::Char(' '), 127 => KeyCode::Backspace, b if (32..127).contains(&b) => KeyCode::Char(b as char), diff --git a/crates/fresh-input-parser/src/proptests.rs b/crates/fresh-input-parser/src/proptests.rs index 3e458a85cf..aa0855b25f 100644 --- a/crates/fresh-input-parser/src/proptests.rs +++ b/crates/fresh-input-parser/src/proptests.rs @@ -205,7 +205,7 @@ proptest! { prop_assert_eq!(ev.len(), pre.len() + 1 + post.len(), "events: {:?}", ev); for (e, &b) in ev.iter().zip(pre.iter()) { - prop_assert_eq!(e, &Event::Key(KeyEvent::new(KeyCode::Char(b as char), KeyModifiers::empty()))); + prop_assert_eq!(e, &Event::key(KeyEvent::new(KeyCode::Char(b as char), KeyModifiers::empty()))); } match &ev[pre.len()] { Event::Mouse(me) => { @@ -215,7 +215,7 @@ proptest! { other => prop_assert!(false, "expected mouse at index {}, got {:?}", pre.len(), other), } for (e, &b) in ev[pre.len() + 1..].iter().zip(post.iter()) { - prop_assert_eq!(e, &Event::Key(KeyEvent::new(KeyCode::Char(b as char), KeyModifiers::empty()))); + prop_assert_eq!(e, &Event::key(KeyEvent::new(KeyCode::Char(b as char), KeyModifiers::empty()))); } } diff --git a/crates/fresh-input-parser/src/tests.rs b/crates/fresh-input-parser/src/tests.rs index 10f6a20bb5..c9558a5b7a 100644 --- a/crates/fresh-input-parser/src/tests.rs +++ b/crates/fresh-input-parser/src/tests.rs @@ -17,6 +17,17 @@ fn keys(events: &[Event]) -> Vec<(KeyCode, KeyModifiers)> { .collect() } +/// Collect the `Key` events of a parse together with their layout characters. +fn keys_with_layout(events: &[Event]) -> Vec<(KeyCode, KeyModifiers, Option)> { + events + .iter() + .filter_map(|e| match e { + Event::Key(press) => Some((press.code, press.modifiers, press.layout_char)), + _ => None, + }) + .collect() +} + /// True if any event is a `Key(Char(_))` — used to prove mouse bytes never /// leak into the child as literal characters. fn has_char_key(events: &[Event]) -> bool { @@ -48,6 +59,118 @@ fn control_characters_have_ctrl_modifier() { assert_eq!(keys(&ev), vec![(KeyCode::Char('c'), KeyModifiers::CONTROL)]); } +/// 0x1F is the byte a legacy terminal sends for Ctrl+/ — the chord users +/// actually press — and it must report as Ctrl+/ so a `ctrl+/` binding fires on +/// xterm/iTerm and not only under the kitty protocol. Reporting it as Ctrl+_ +/// left `ctrl+/` dead on every terminal without CSI-u. +#[test] +fn us_byte_is_ctrl_slash() { + let mut p = InputParser::new(); + assert_eq!( + keys(&p.parse(&[0x1f])), + vec![(KeyCode::Char('/'), KeyModifiers::CONTROL)] + ); +} + +/// The kitty encoding of the same chord agrees with the legacy byte, so a +/// binding resolves identically on both kinds of terminal. +#[test] +fn ctrl_slash_agrees_across_protocols() { + let mut p = InputParser::new(); + assert_eq!(keys(&p.parse(&[0x1f])), keys(&p.parse(b"\x1b[47;5u"))); +} + +/// Characterization test (passes before and after the Ctrl+/ fix): pins the +/// neighbouring separators so re-canonicalising 0x1F does not drag them along. +/// Ctrl+\ (0x1C, SIGQUIT), Ctrl+] (0x1D, the telnet/readline escape) and +/// Ctrl+^ (0x1E) are the keys people press for those bytes, and `ctrl+]` / +/// `ctrl+\` are live bindings in the default and macOS keymaps. +#[test] +fn other_separator_bytes_keep_their_keys() { + let mut p = InputParser::new(); + assert_eq!( + keys(&p.parse(&[0x1c, 0x1d, 0x1e])), + vec![ + (KeyCode::Char('\\'), KeyModifiers::CONTROL), + (KeyCode::Char(']'), KeyModifiers::CONTROL), + (KeyCode::Char('^'), KeyModifiers::CONTROL), + ] + ); +} + +// ---- Keyboard layout (`KeyPress::layout_char`) ---- + +/// The non-US half of sinelaw/fresh#2933. On a German layout `/` is Shift+7, so +/// pressing Ctrl+/ reports base 55 (`7`), shifted 47 (`/`), Ctrl+Shift. The +/// chord keeps its physical spelling — folding the `/` in would collapse +/// `Ctrl+Shift+A` onto `Ctrl+A` for letters — but the `/` the user meant has to +/// survive for the keymap to find `ctrl+/`. +#[test] +fn a_control_chord_carries_the_character_the_layout_types() { + let mut p = InputParser::new(); + assert_eq!( + keys_with_layout(&p.parse(b"\x1b[55:47;6u")), + vec![( + KeyCode::Char('7'), + KeyModifiers::CONTROL | KeyModifiers::SHIFT, + Some('/') + )] + ); +} + +/// The same chord on a US layout, where Shift+7 types `&`. The layout character +/// is still reported — the parser does not know or care which layout is in use +/// — and it is the keymap that declines to use it, since nothing binds +/// `ctrl+&`. This is what keeps US `ctrl+shift+7` bindings intact. +#[test] +fn the_us_reading_of_that_chord_is_unchanged() { + let mut p = InputParser::new(); + assert_eq!( + keys_with_layout(&p.parse(b"\x1b[55:38;6u")), + vec![( + KeyCode::Char('7'), + KeyModifiers::CONTROL | KeyModifiers::SHIFT, + Some('&') + )] + ); +} + +/// No layout character when the two readings agree: a plain letter's shifted +/// codepoint is just its uppercase, which the chord's SHIFT bit already says. +/// Keeping this `None` is what stops every shifted keystroke from taking the +/// keymap's second lookup. +#[test] +fn agreeing_readings_report_no_layout_char() { + let mut p = InputParser::new(); + // Ctrl+Shift+A: base 97 (`a`), shifted 65 (`A`) — same key, and the chord + // is spelled `ctrl+shift+a`, so there is nothing extra to say. + assert_eq!( + keys_with_layout(&p.parse(b"\x1b[97:65;6u")), + vec![( + KeyCode::Char('a'), + KeyModifiers::CONTROL | KeyModifiers::SHIFT, + None + )] + ); + // And a chord with no shifted codepoint reported at all. + assert_eq!( + keys_with_layout(&p.parse(b"\x1b[47;5u")), + vec![(KeyCode::Char('/'), KeyModifiers::CONTROL, None)] + ); +} + +/// Shift alone still resolves to the character it types, as before — the +/// shifted reading *is* the chord there, so it is folded in rather than carried +/// alongside. Guards against the new path stealing plain shifted typing. +#[test] +fn shift_without_control_still_folds_the_character_in() { + let mut p = InputParser::new(); + assert_eq!( + keys_with_layout(&p.parse(b"\x1b[55:47;2u")), + vec![(KeyCode::Char('/'), KeyModifiers::empty(), None)] + ); +} + #[test] fn enter_key_cr_and_lf() { let mut p = InputParser::new(); diff --git a/docs/internal/terminal-input-parsing.md b/docs/internal/terminal-input-parsing.md index 74b2f67653..d43be2b755 100644 --- a/docs/internal/terminal-input-parsing.md +++ b/docs/internal/terminal-input-parsing.md @@ -184,6 +184,16 @@ parser exists to prevent — each is covered by a test that fails without the fi byte and merely gain the sub-parameter. Decoding it for CSI-u alone left every arrow keypress moving the cursor twice and every Delete removing two characters (#2796) while typing was unaffected — the asymmetry that hid the gap. +- **`0x1F` reports as `Ctrl+/`, matching the kitty form.** A terminal without + the kitty keyboard protocol has one byte for that chord and sends it for + Ctrl+/, Ctrl+7 and Ctrl+_ alike, so the parser has to pick one spelling. It + picks `/`: the key people actually press for it (no Shift needed), and the + key a kitty terminal reports for the same chord via `CSI 47;5u`. Deriving the + whole `0x1C..=0x1F` range arithmetically instead gave `Ctrl+_`, which no + keymap binds — so `ctrl+/` (toggle-comment in the default keymap, undo in the + emacs one) fired under kitty and nowhere else (#2933). `0x1C`/`0x1D`/`0x1E` + keep `\`, `]`, `^`: those *are* the keys bound to them, and `ctrl+]` and + `ctrl+\` are live bindings. - **Modifier decoding is complete.** Shift/Alt/Ctrl/Super/Hyper/Meta are all mapped, and the modifier field is parsed as `u16` so its maximum legal value (256) no longer overflows and fails closed. Caps Lock / Num Lock have no