h/fish-shell
1
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2026-05-22 03:51:15 -03:00
Code Issues Packages Projects Releases Wiki Activity

Match bindings with explicit shift first

Browse Source 
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 50a6e486a5 (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 50a6e486a5, 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
This commit is contained in:
Johannes Altmanninger
2025-05-24 10:42:06 +02:00
parent 3951a858dd
commit 08c8afcb12
3 changed files with 143 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
src/builtins/fish_key_reader.rs
View File

@@ -20,8 +20,8 @@
env::{env_init, EnvStack, Environment},
future_feature_flags,
input_common::{
terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent, InputEventQueue,
InputEventQueuer, KeyEvent, QueryResponseEvent, TerminalQuery,
match_key_event_to_key, terminal_protocol_hacks, terminal_protocols_enable_ifn, CharEvent,
InputEventQueue, InputEventQueuer, KeyEvent, QueryResponseEvent, TerminalQuery,
},
key::{char_to_symbol, Key, Modifiers},
nix::isatty,
@@ -40,19 +40,24 @@
use super::prelude::*;
/// Return true if the recent sequence of characters indicates the user wants to exit the program.
fn should_exit(streams: &mut IoStreams, recent_keys: &mut Vec<KeyEvent>, key: KeyEvent) -> bool {
recent_keys.push(key);
fn should_exit(
streams: &mut IoStreams,
recent_keys: &mut Vec<KeyEvent>,
key_evt: KeyEvent,
) -> bool {
recent_keys.push(key_evt);
for evt in [VINTR, VEOF] {
let modes = shell_modes();
let cc = Key::from_single_byte(modes.c_cc[evt]);
if key == 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;
}

119
src/input.rs
View File

@@ -7,8 +7,8 @@
use crate::future::IsSomeAnd;
use crate::global_safety::RelaxedAtomicBool;
use crate::input_common::{
CharEvent, CharInputStyle, ImplicitEvent, InputData, InputEventQueuer, ReadlineCmd,
TerminalQuery, R_END_INPUT_FUNCTIONS,
match_key_event_to_key, CharEvent, CharInputStyle, ImplicitEvent, InputData, InputEventQueuer,
KeyMatchQuality, ReadlineCmd, TerminalQuery, R_END_INPUT_FUNCTIONS,
};
use crate::key::{self, canonicalize_raw_escapes, ctrl, Key, Modifiers};
use crate::proc::job_reap;
@@ -20,6 +20,7 @@
use crate::wchar::prelude::*;
use once_cell::sync::Lazy;
use std::cell::RefMut;
use std::mem;
use std::sync::{
atomic::{AtomicU32, Ordering},
Mutex, MutexGuard,
@@ -502,14 +503,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<KeyMatchQuality> {
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.
@@ -520,7 +526,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 {
@@ -529,7 +535,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;
}
}
};
@@ -539,9 +545,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;
@@ -549,13 +553,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() {
@@ -575,7 +579,7 @@ fn next_is_char(&mut self, style: &KeyNameStyle, key: Key, escaped: bool) -> boo
actual_seq.len()
)
);
return true;
return Some(KeyMatchQuality::Legacy);
}
if key.modifiers == Modifiers::ALT && seq_char == '\x1b' {
if self.subidx + 1 == actual_seq.len() {
@@ -595,11 +599,11 @@ 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(KeyMatchQuality::Legacy);
}
}
}
false
None
}
/// Consume all events up to the current index.
@@ -627,7 +631,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<KeyMatchQuality>,
) -> bool {
assert!(
!seq.is_empty(),
"Empty sequence passed to try_peek_sequence"
@@ -637,9 +646,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 {
@@ -656,16 +666,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<InputMapping> {
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<KeyMatchQuality>,
idx: usize,
subidx: usize,
}
let mut deferred: Option<MatchedMapping<'a>> = 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;
@@ -673,24 +691,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() {
@@ -699,17 +734,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()
}
}

94
src/input_common.rs
View File

@@ -219,24 +219,33 @@ fn apply_shift(mut key: Key, do_ascii: bool, shifted_codepoint: char) -> Option<
Some(key)
}
impl PartialEq<Key> for KeyEvent {
fn eq(&self, key: &Key) -> bool {
if &self.key == key {
return true;
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) 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(crate) fn match_key_event_to_key(event: &KeyEvent, key: &Key) -> Option<KeyMatchQuality> {
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;
@@ -246,41 +255,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.
@@ -859,7 +875,7 @@ fn try_readch(&mut self, blocking: bool) -> Option<CharEvent> {
}
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));
@@ -925,7 +941,11 @@ fn try_readch(&mut self, blocking: bool) -> Option<CharEvent> {
}
});
let vintr = shell_modes().c_cc[libc::VINTR];
if vintr != 0 && key.is_some_and(|key| key == Key::from_single_byte(vintr))
if vintr != 0
&& key.is_some_and(|key| {
match_key_event_to_key(&key, &Key::from_single_byte(vintr))
.is_some()
})
{
FLOG!(
reader,
@@ -968,7 +988,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;