From b9d9e7edc65af16a5867bad3c0063cefb56ba530 Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sat, 24 May 2025 10:42:06 +0200 Subject: [PATCH] Match bindings with explicit shift first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new key notation canonicalizes aggressively, e.g. these two bindings clash: bind ctrl-shift-a something bind shift-ctrl-a something else This means that key events generally match at most one active binding that uses the new syntax. The exception -- two coexisting new-syntax binds that match the same key event -- was added by commit 50a6e486a56 (Allow explicit shift modifier for non-ASCII letters, fix capslock behavior, 2025-03-30): bind ctrl-A 'echo A' bind ctrl-shift-a 'echo shift-a' The precedence was determined by definition order. This doesn't seem very useful. A following patch wants to resolve #11520 by matching "ctrl-ц" events against "ctrl-w" bindings. It would be surprising if a "ctrl-w" binding shadowed a "ctrl-ц" one based on something as subtle as definition order. Additionally, definition order semantics (which is an unintended cause of the implementation) is not really obvious. Reverse definition order would make more sense. Remove the ambiguity by always giving precedence to bindings that use explicit shift. Unrelated to this, as established in 50a6e486a56, explicit shift is still recommended for bicameral letters but not typically for others -- e.g. alt-+ is typically preferred over alt-shift-= because the former also works on a German keyboard. See #11520 (cherry picked from commit 08c8afcb1294475bebf324f48b48fc7ffd9dfdd2) --- src/bin/fish_key_reader.rs | 9 +-- src/input.rs | 127 ++++++++++++++++++++++++------------- src/input_common.rs | 90 +++++++++++++++----------- 3 files changed, 141 insertions(+), 85 deletions(-) diff --git a/src/bin/fish_key_reader.rs b/src/bin/fish_key_reader.rs index 405a7b33c..c875e33a3 100644 --- a/src/bin/fish_key_reader.rs +++ b/src/bin/fish_key_reader.rs @@ -20,8 +20,8 @@ eprintf, fprintf, input::input_terminfo_get_name, input_common::{ - terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, InputEventQueue, - InputEventQueuer, KeyEvent, + match_key_event_to_key, terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, + InputEventQueue, InputEventQueuer, KeyEvent, }, key::{char_to_symbol, Key, Modifiers}, panic::panic_handler, @@ -44,12 +44,13 @@ fn should_exit(recent_keys: &mut Vec, key_evt: KeyEvent) -> bool { let modes = shell_modes(); let cc = Key::from_single_byte(modes.c_cc[evt]); - if key_evt == cc { + if match_key_event_to_key(&key_evt, &cc).is_some() { if recent_keys .iter() .rev() .nth(1) - .is_some_and(|&prev| prev == cc) + .and_then(|&prev| match_key_event_to_key(&prev, &cc)) + .is_some() { return true; } diff --git a/src/input.rs b/src/input.rs index cb1d98b65..25ad13ca9 100644 --- a/src/input.rs +++ b/src/input.rs @@ -4,8 +4,8 @@ use crate::event; use crate::flog::FLOG; use crate::input_common::{ - CharEvent, CharInputStyle, InputData, InputEventQueuer, KeyEvent, ReadlineCmd, - R_END_INPUT_FUNCTIONS, + match_key_event_to_key, CharEvent, CharInputStyle, InputData, InputEventQueuer, KeyEvent, + KeyMatchQuality, ReadlineCmd, R_END_INPUT_FUNCTIONS, }; use crate::key::{self, canonicalize_raw_escapes, ctrl, Key, Modifiers}; use crate::proc::job_reap; @@ -17,6 +17,7 @@ use crate::wchar::prelude::*; use once_cell::sync::{Lazy, OnceCell}; use std::ffi::CString; +use std::mem; use std::sync::{ atomic::{AtomicU32, Ordering}, Mutex, MutexGuard, @@ -510,14 +511,19 @@ fn next(&mut self) -> CharEvent { /// Check if the next event is the given character. This advances the index on success only. /// If `escaped` is set, then return false if this (or any other) character had a timeout. - fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> bool { + fn next_is_char( + &mut self, + style: &KeyNameStyle, + key: Key, + escaped: bool, + ) -> Option { assert!( self.idx <= self.peeked.len(), "Index must not be larger than dequeued event count" ); // See if we had a timeout already. if escaped && self.had_timeout { - return false; + return None; } // Grab a new event if we have exhausted what we have already peeked. // Use either readch or readch_timed, per our param. @@ -528,7 +534,7 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo Some(evt) => evt, None => { self.had_timeout = true; - return false; + return None; } } } else { @@ -537,7 +543,7 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo Some(evt) => evt, None => { self.had_timeout = true; - return false; + return None; } } }; @@ -547,9 +553,7 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo // Now we have peeked far enough; check the event. // If it matches the char, then increment the index. let evt = &self.peeked[self.idx]; - let Some(kevt) = evt.get_key() else { - return false; - }; + let kevt = evt.get_key()?; if kevt.seq == L!("\x1b") && key.modifiers == Modifiers::ALT { self.idx += 1; self.subidx = 0; @@ -557,13 +561,13 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo return self.next_is_char(style, Key::from_raw(key.codepoint), true); } if *style == KeyNameStyle::Plain { - if kevt.key == key { + let result = match_key_event_to_key(&kevt.key, &key); + if let Some(key_match) = &result { assert!(self.subidx == 0); self.idx += 1; - FLOG!(reader, "matched full key", key); - return true; + FLOG!(reader, "matched full key", key, "kind", key_match); } - return false; + return result; } let actual_seq = kevt.seq.as_char_slice(); if !actual_seq.is_empty() { @@ -583,7 +587,11 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo actual_seq.len() ) ); - return true; + return Some(if matches!(style, KeyNameStyle::Terminfo(_)) { + KeyMatchQuality::Exact + } else { + KeyMatchQuality::Legacy + }); } if key.modifiers == Modifiers::ALT && seq_char == '\x1b' { if self.subidx + 1 == actual_seq.len() { @@ -603,11 +611,15 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo self.subidx = 0; } FLOG!(reader, format!("matched {key} against raw escape sequence")); - return true; + return Some(if matches!(style, KeyNameStyle::Terminfo(_)) { + KeyMatchQuality::Exact + } else { + KeyMatchQuality::Legacy + }); } } } - false + None } /// Consume all events up to the current index. @@ -634,7 +646,12 @@ pub fn restart(&mut self) { } /// Return true if this `peeker` matches a given sequence of char events given by `str`. - fn try_peek_sequence(&mut self, style: &KeyNameStyle, seq: &[Key]) -> bool { + fn try_peek_sequence( + &mut self, + style: &KeyNameStyle, + seq: &[Key], + quality: &mut Vec, + ) -> bool { assert!( !seq.is_empty(), "Empty sequence passed to try_peek_sequence" @@ -644,9 +661,10 @@ fn try_peek_sequence(&mut self, style: &KeyNameStyle, seq: &[Key]) -> bool { // If we just read an escape, we need to add a timeout for the next char, // to distinguish between the actual escape key and an "alt"-modifier. let escaped = *style != KeyNameStyle::Plain && prev == Key::from_raw(key::Escape); - if !self.next_is_char(style, *key, escaped) { + let Some(spec) = self.next_is_char(style, *key, escaped) else { return false; - } + }; + quality.push(spec); prev = *key; } if self.subidx != 0 { @@ -663,16 +681,24 @@ fn try_peek_sequence(&mut self, style: &KeyNameStyle, seq: &[Key]) -> bool { /// user's mapping list, then the preset list. /// Return none if nothing matches, or if we may have matched a longer sequence but it was /// interrupted by a readline event. - pub fn find_mapping( + pub fn find_mapping<'a>( &mut self, vars: &dyn Environment, - ip: &InputMappingSet, + ip: &'a InputMappingSet, ) -> Option { - let mut generic: Option<&InputMapping> = None; let bind_mode = input_get_bind_mode(vars); - let mut escape: Option<&InputMapping> = None; + + struct MatchedMapping<'a> { + mapping: &'a InputMapping, + quality: Vec, + idx: usize, + subidx: usize, + } + + let mut deferred: Option> = None; let ml = ip.mapping_list.iter().chain(ip.preset_mapping_list.iter()); + let mut quality = vec![]; for m in ml { if m.mode != bind_mode { continue; @@ -680,24 +706,41 @@ pub fn find_mapping( // Defer generic mappings until the end. if m.is_generic() { - if generic.is_none() { - generic = Some(m); + if deferred.is_none() { + deferred = Some(MatchedMapping { + mapping: m, + quality: vec![], + idx: self.idx, + subidx: self.subidx, + }); } continue; } // FLOG!(reader, "trying mapping", format!("{:?}", m)); - if self.try_peek_sequence(&m.key_name_style, &m.seq) { - // A binding for just escape should also be deferred - // so escape sequences take precedence. - if m.seq == vec![Key::from_raw(key::Escape)] { - if escape.is_none() { - escape = Some(m); - } - } else { + if self.try_peek_sequence(&m.key_name_style, &m.seq, &mut quality) { + // // A binding for just escape should also be deferred + // // so escape sequences take precedence. + let is_escape = m.seq == vec![Key::from_raw(key::Escape)]; + let is_perfect_match = quality + .iter() + .all(|key_match| *key_match == KeyMatchQuality::Exact); + if !is_escape && is_perfect_match { return Some(m.clone()); } + if deferred + .as_ref() + .is_none_or(|matched| !is_escape && quality >= matched.quality) + { + deferred = Some(MatchedMapping { + mapping: m, + quality: mem::take(&mut quality), + idx: self.idx, + subidx: self.subidx, + }); + } } + quality.clear(); self.restart(); } if self.char_sequence_interrupted() { @@ -706,17 +749,13 @@ pub fn find_mapping( return None; } - if escape.is_some() { - // We need to reconsume the escape. - self.next(); - return escape.cloned(); - } - - if generic.is_some() { - generic.cloned() - } else { - None - } + deferred + .map(|matched| { + self.idx = matched.idx; + self.subidx = matched.subidx; + matched.mapping + }) + .cloned() } } diff --git a/src/input_common.rs b/src/input_common.rs index b44e3bdcb..5c8c64f58 100644 --- a/src/input_common.rs +++ b/src/input_common.rs @@ -6,7 +6,7 @@ }; use crate::env::{EnvStack, Environment}; use crate::fd_readable_set::FdReadableSet; -use crate::flog::FLOG; +use crate::flog::{FloggableDebug, FLOG}; use crate::fork_exec::flog_safe::FLOG_SAFE; use crate::future_feature_flags::{feature_test, FeatureFlag}; use crate::global_safety::RelaxedAtomicBool; @@ -208,24 +208,33 @@ fn apply_shift(mut key: Key, do_ascii: bool, shifted_codepoint: char) -> Option< Some(key) } -impl PartialEq for KeyEvent { - fn eq(&self, key: &Key) -> bool { - if &self.key == key { - return true; - } +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub enum KeyMatchQuality { + Legacy, + ModuloShift, + Exact, +} - let Some(shifted_evt) = apply_shift(self.key, false, self.shifted_codepoint) else { - return false; - }; - let Some(shifted_key) = apply_shift(*key, true, '\0') else { - return false; - }; - shifted_evt == shifted_key +impl FloggableDebug for KeyMatchQuality {} + +pub fn match_key_event_to_key(event: &KeyEvent, key: &Key) -> Option { + if &event.key == key { + return Some(KeyMatchQuality::Exact); } + + let shifted_evt = apply_shift(event.key, false, event.shifted_codepoint)?; + let shifted_key = apply_shift(*key, true, '\0')?; + (shifted_evt == shifted_key).then_some(KeyMatchQuality::ModuloShift) } #[test] -fn test_key_event_eq() { +fn test_match_key_event_to_key() { + macro_rules! validate { + ($evt:expr, $key:expr, $expected:expr) => { + assert_eq!(match_key_event_to_key(&$evt, &$key), $expected); + }; + } + let none = Modifiers::default(); let shift = Modifiers::SHIFT; let ctrl = Modifiers::CTRL; @@ -235,41 +244,48 @@ fn test_key_event_eq() { ..Default::default() }; - assert_eq!(KeyEvent::new(none, 'a'), Key::new(none, 'a')); - assert_ne!(KeyEvent::new(none, 'a'), Key::new(none, 'A')); - assert_eq!(KeyEvent::new(shift, 'a'), Key::new(shift, 'a')); - assert_ne!(KeyEvent::new(shift, 'a'), Key::new(none, 'A')); - assert_ne!(KeyEvent::new(shift, 'ä'), Key::new(none, 'Ä')); + let exact = KeyMatchQuality::Exact; + let modulo_shift = KeyMatchQuality::ModuloShift; + + validate!(KeyEvent::new(none, 'a'), Key::new(none, 'a'), Some(exact)); + validate!(KeyEvent::new(none, 'a'), Key::new(none, 'A'), None); + validate!(KeyEvent::new(shift, 'a'), Key::new(shift, 'a'), Some(exact)); + validate!(KeyEvent::new(shift, 'a'), Key::new(none, 'A'), None); + validate!(KeyEvent::new(shift, 'ä'), Key::new(none, 'Ä'), None); // For historical reasons we canonicalize notation for ASCII keys like "shift-a" to "A", // but not "shift-a" events - those should send a shifted key. - assert_eq!(KeyEvent::new(none, 'A'), Key::new(shift, 'a')); - assert_ne!(KeyEvent::new(none, 'A'), Key::new(shift, 'A')); - assert_eq!(KeyEvent::new(none, 'Ä'), Key::new(none, 'Ä')); - assert_ne!(KeyEvent::new(none, 'Ä'), Key::new(shift, 'ä')); + validate!( + KeyEvent::new(none, 'A'), + Key::new(shift, 'a'), + Some(modulo_shift) + ); + validate!(KeyEvent::new(none, 'A'), Key::new(shift, 'A'), None); + validate!(KeyEvent::new(none, 'Ä'), Key::new(none, 'Ä'), Some(exact)); + validate!(KeyEvent::new(none, 'Ä'), Key::new(shift, 'ä'), None); // FYI: for codepoints that are not letters with uppercase/lowercase versions, we use // the shifted key in the canonical notation, because the unshifted one may depend on the // keyboard layout. let ctrl_shift_equals = KeyEvent::with_shifted_codepoint(ctrl_shift, '=', Some('+')); - assert_eq!(ctrl_shift_equals, Key::new(ctrl_shift, '=')); - assert_eq!(ctrl_shift_equals, Key::new(ctrl, '+')); // canonical notation - assert_ne!(ctrl_shift_equals, Key::new(ctrl_shift, '+')); - assert_ne!(ctrl_shift_equals, Key::new(ctrl, '=')); + validate!(ctrl_shift_equals, Key::new(ctrl_shift, '='), Some(exact)); + validate!(ctrl_shift_equals, Key::new(ctrl, '+'), Some(modulo_shift)); // canonical notation + validate!(ctrl_shift_equals, Key::new(ctrl_shift, '+'), None); + validate!(ctrl_shift_equals, Key::new(ctrl, '='), None); // A event like capslock-shift-ä may or may not include a shifted codepoint. // // Without a shifted codepoint, we cannot easily match ctrl-Ä. let caps_ctrl_shift_ä = KeyEvent::new(ctrl_shift, 'ä'); - assert_eq!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'ä')); // canonical notation - assert_ne!(caps_ctrl_shift_ä, Key::new(ctrl, 'ä')); - assert_ne!(caps_ctrl_shift_ä, Key::new(ctrl, 'Ä')); // can't match without shifted key - assert_ne!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'Ä')); + validate!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'ä'), Some(exact)); // canonical notation + validate!(caps_ctrl_shift_ä, Key::new(ctrl, 'ä'), None); + validate!(caps_ctrl_shift_ä, Key::new(ctrl, 'Ä'), None); // can't match without shifted key + validate!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'Ä'), None); // With a shifted codepoint, we can match the alternative notation too. let caps_ctrl_shift_ä = KeyEvent::with_shifted_codepoint(ctrl_shift, 'ä', Some('Ä')); - assert_eq!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'ä')); // canonical notation - assert_ne!(caps_ctrl_shift_ä, Key::new(ctrl, 'ä')); - assert_eq!(caps_ctrl_shift_ä, Key::new(ctrl, 'Ä')); // matched via shifted key - assert_ne!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'Ä')); + validate!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'ä'), Some(exact)); // canonical notation + validate!(caps_ctrl_shift_ä, Key::new(ctrl, 'ä'), None); + validate!(caps_ctrl_shift_ä, Key::new(ctrl, 'Ä'), Some(modulo_shift)); // matched via shifted key + validate!(caps_ctrl_shift_ä, Key::new(ctrl_shift, 'Ä'), None); } /// Represents an event on the character input stream. @@ -823,7 +839,7 @@ fn try_readch(&mut self, blocking: bool) -> Option { } let mut seq = WString::new(); let mut key = key_with_escape; - if key.is_some_and(|key| key == Key::from_raw(key::Invalid)) { + if key.is_some_and(|key| key.key == Key::from_raw(key::Invalid)) { continue; } assert!(key.map_or(true, |key| key.codepoint != key::Invalid)); @@ -903,7 +919,7 @@ fn parse_escape_sequence( return Some( match self.parse_escape_sequence(buffer, have_escape_prefix) { Some(mut nested_sequence) => { - if nested_sequence == invalid.key { + if nested_sequence.key == invalid.key { return Some(KeyEvent::from_raw(key::Escape)); } nested_sequence.modifiers.alt = true;