From 6a0efbfdba143b89b21124f7688d823c6fd59e54 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 26 Mar 2026 04:31:37 +0000 Subject: [PATCH] Refactor ShortcutTracker.Match() to respect AllowFallback priority order Co-authored-by: kovidgoyal <1308621+kovidgoyal@users.noreply.github.com> Agent-Logs-Url: https://github.com/kovidgoyal/kitty/sessions/85fbf706-4688-4901-9a23-907cebc91da3 --- docs/changelog.rst | 2 + tools/config/utils.go | 51 ++++++++++++++---- tools/config/utils_test.go | 82 +++++++++++++++++++++++++++++ tools/tui/loop/key-encoding.go | 40 +++++++++++--- tools/tui/loop/key-encoding_test.go | 54 +++++++++++++++++++ 5 files changed, 210 insertions(+), 19 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index f17bb0a5b..8441e0a74 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -186,6 +186,8 @@ Detailed list of changes - The :opt:`show_hyperlink_targets` option now allows specifying a keyboard modifier so that target URLs are only shown on hover when the modifier is pressed (:pull:`9741`) +- Shortcut matching: When multiple shortcuts match a key event via different fallback types, the one whose :opt:`allow_fallback ` lists the matching fallback type first wins (higher priority). Direct matches always take priority over fallback matches. + 0.46.2 [2026-03-21] ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/tools/config/utils.go b/tools/config/utils.go index c8480ca54..4dcd2e479 100644 --- a/tools/config/utils.go +++ b/tools/config/utils.go @@ -326,37 +326,66 @@ func ParseMap(val string) (*KeyAction, error) { return &KeyAction{Name: action_name, Args: action_args, Normalized_keys: NormalizeShortcuts(spec), AllowFallback: allow_fallback}, nil } +type partialMatch struct { + action *KeyAction + priority int +} + type ShortcutTracker struct { - partial_matches []*KeyAction + partial_matches []partialMatch partial_num_consumed int } func (self *ShortcutTracker) Match(ev *loop.KeyEvent, all_actions []*KeyAction) *KeyAction { if self.partial_num_consumed > 0 { ev.Handled = true - self.partial_matches = utils.Filter(self.partial_matches, func(ac *KeyAction) bool { - return self.partial_num_consumed < len(ac.Normalized_keys) && ev.MatchesPressOrRepeatWithFallback(ac.Normalized_keys[self.partial_num_consumed], ac.AllowFallback) - }) + new_matches := self.partial_matches[:0] + for _, pm := range self.partial_matches { + if self.partial_num_consumed >= len(pm.action.Normalized_keys) { + continue + } + p := ev.MatchesPressOrRepeatPriorityWithFallback(pm.action.Normalized_keys[self.partial_num_consumed], pm.action.AllowFallback) + if p >= 0 { + if p > pm.priority { + pm.priority = p + } + new_matches = append(new_matches, pm) + } + } + self.partial_matches = new_matches if len(self.partial_matches) == 0 { self.partial_num_consumed = 0 return nil } } else { - self.partial_matches = utils.Filter(all_actions, func(ac *KeyAction) bool { - return ev.MatchesPressOrRepeatWithFallback(ac.Normalized_keys[0], ac.AllowFallback) - }) + new_matches := self.partial_matches[:0] + for _, ac := range all_actions { + p := ev.MatchesPressOrRepeatPriorityWithFallback(ac.Normalized_keys[0], ac.AllowFallback) + if p >= 0 { + new_matches = append(new_matches, partialMatch{action: ac, priority: p}) + } + } + self.partial_matches = new_matches if len(self.partial_matches) == 0 { return nil } ev.Handled = true } self.partial_num_consumed++ - for _, x := range self.partial_matches { - if self.partial_num_consumed >= len(x.Normalized_keys) { - self.partial_num_consumed = 0 - return x + var best *partialMatch + for i := range self.partial_matches { + pm := &self.partial_matches[i] + if self.partial_num_consumed >= len(pm.action.Normalized_keys) { + if best == nil || pm.priority < best.priority { + best = pm + } } } + if best != nil { + self.partial_num_consumed = 0 + self.partial_matches = nil + return best.action + } return nil } diff --git a/tools/config/utils_test.go b/tools/config/utils_test.go index 30cf33324..8e70dbfc3 100644 --- a/tools/config/utils_test.go +++ b/tools/config/utils_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "github.com/kovidgoyal/kitty/tools/tui/loop" ) var _ = fmt.Print @@ -176,3 +177,84 @@ func TestNormalizeShortcuts(t *testing.T) { } } } + +func TestShortcutTrackerMatchPriority(t *testing.T) { + // Helper to create a KeyAction with a given spec and AllowFallback. + makeAction := func(name, spec, allowFallback string) *KeyAction { + return &KeyAction{Name: name, Normalized_keys: NormalizeShortcuts(spec), AllowFallback: allowFallback} + } + // Helper to simulate a key press event. + makeEv := func(key, shiftedKey, alternateKey string, mods loop.KeyModifiers) *loop.KeyEvent { + return &loop.KeyEvent{Type: loop.PRESS, Key: key, ShiftedKey: shiftedKey, AlternateKey: alternateKey, Mods: mods} + } + + // Scenario 1: shifted key event — "shifted,ascii" shortcut wins over "ascii,shifted" + actions := []*KeyAction{ + makeAction("ascii_shifted", "a", "ascii,shifted"), + makeAction("shifted_ascii", "a", "shifted,ascii"), + } + // Shift+A with ShiftedKey="a": matches via shifted fallback for both + tracker := ShortcutTracker{} + ev := makeEv("A", "a", "", loop.SHIFT) + result := tracker.Match(ev, actions) + if result == nil || result.Name != "shifted_ascii" { + name := "" + if result != nil { + name = result.Name + } + t.Fatalf("shifted key: expected 'shifted_ascii' (shifted first), got %q", name) + } + + // Scenario 2: alternate (non-ASCII) key event — "ascii,shifted" shortcut wins over "shifted,ascii" + actions2 := []*KeyAction{ + makeAction("shifted_ascii", "ctrl+c", "shifted,ascii"), + makeAction("ascii_shifted", "ctrl+c", "ascii,shifted"), + } + // Cyrillic "с" with AlternateKey="c": matches via ascii fallback for both + tracker2 := ShortcutTracker{} + ev2 := makeEv("с", "", "c", loop.CTRL) + result2 := tracker2.Match(ev2, actions2) + if result2 == nil || result2.Name != "ascii_shifted" { + name := "" + if result2 != nil { + name = result2.Name + } + t.Fatalf("ascii key: expected 'ascii_shifted' (ascii first), got %q", name) + } + + // Scenario 3: direct match wins over any fallback match + // Event: Cyrillic "с" with ctrl + AlternateKey="c"; two shortcuts: one direct match for Cyrillic key, + // one matching via ascii fallback. + actions3 := []*KeyAction{ + makeAction("fallback", "ctrl+c", "ascii"), + makeAction("direct", "ctrl+с", ""), + } + tracker3 := ShortcutTracker{} + ev3 := makeEv("с", "", "c", loop.CTRL) + result3 := tracker3.Match(ev3, actions3) + if result3 == nil || result3.Name != "direct" { + name := "" + if result3 != nil { + name = result3.Name + } + t.Fatalf("direct match: expected 'direct', got %q", name) + } + + // Scenario 4: single-type AllowFallback has same priority as first position in two-type AllowFallback + // "shifted" only vs "shifted,ascii" — when matching via shifted key, both have priority 1, so first in list wins + actions4 := []*KeyAction{ + makeAction("shifted_only", "a", "shifted"), + makeAction("shifted_ascii", "a", "shifted,ascii"), + } + tracker4 := ShortcutTracker{} + ev4 := makeEv("A", "a", "", loop.SHIFT) + result4 := tracker4.Match(ev4, actions4) + // Both have priority 1 (shifted at position 0); first encountered wins + if result4 == nil || result4.Name != "shifted_only" { + name := "" + if result4 != nil { + name = result4.Name + } + t.Fatalf("single vs two-type (shifted): expected 'shifted_only', got %q", name) + } +} diff --git a/tools/tui/loop/key-encoding.go b/tools/tui/loop/key-encoding.go index 77fbaa5c4..5cd2c60f0 100644 --- a/tools/tui/loop/key-encoding.go +++ b/tools/tui/loop/key-encoding.go @@ -292,20 +292,44 @@ func isNonASCIIKey(key string) bool { } func (self *KeyEvent) MatchesParsedShortcutWithFallback(ps *ParsedShortcut, event_type KeyEventType, allowFallback string) bool { + return self.MatchesParsedShortcutPriorityWithFallback(ps, event_type, allowFallback) >= 0 +} + +// MatchesParsedShortcutPriorityWithFallback returns the match priority for the given shortcut: +// - returns -1 if the event does not match +// - returns 0 for a direct match (no fallback needed) +// - returns the 1-based position of the matching fallback type in allowFallback for a fallback match +// (e.g., "shifted" at position 0 in "shifted,ascii" returns 1; "ascii" at position 1 returns 2) +// +// Lower values indicate higher priority, so callers should prefer matches with smaller return values. +func (self *KeyEvent) MatchesParsedShortcutPriorityWithFallback(ps *ParsedShortcut, event_type KeyEventType, allowFallback string) int { if self.Type&event_type == 0 { - return false + return -1 } mods := self.Mods.WithoutLocks() if mods == ps.Mods && self.Key == ps.KeyName { - return true + return 0 } - if strings.Contains(allowFallback, "shifted") && self.ShiftedKey != "" && mods&SHIFT != 0 && (mods & ^SHIFT) == ps.Mods && self.ShiftedKey == ps.KeyName { - return true + canShifted := self.ShiftedKey != "" && mods&SHIFT != 0 && (mods & ^SHIFT) == ps.Mods && self.ShiftedKey == ps.KeyName + canASCII := self.AlternateKey != "" && isNonASCIIKey(self.Key) && mods == ps.Mods && self.AlternateKey == ps.KeyName + for i, part := range strings.Split(allowFallback, ",") { + switch strings.TrimSpace(part) { + case "shifted": + if canShifted { + return i + 1 + } + case "ascii": + if canASCII { + return i + 1 + } + } } - if strings.Contains(allowFallback, "ascii") && self.AlternateKey != "" && isNonASCIIKey(self.Key) && mods == ps.Mods && self.AlternateKey == ps.KeyName { - return true - } - return false + return -1 +} + +// MatchesPressOrRepeatPriorityWithFallback returns the match priority (see MatchesParsedShortcutPriorityWithFallback). +func (self *KeyEvent) MatchesPressOrRepeatPriorityWithFallback(spec string, allowFallback string) int { + return self.MatchesParsedShortcutPriorityWithFallback(ParseShortcut(spec), PRESS|REPEAT, allowFallback) } func (self *KeyEvent) MatchesParsedShortcut(ps *ParsedShortcut, event_type KeyEventType) bool { diff --git a/tools/tui/loop/key-encoding_test.go b/tools/tui/loop/key-encoding_test.go index 90f625306..657337349 100644 --- a/tools/tui/loop/key-encoding_test.go +++ b/tools/tui/loop/key-encoding_test.go @@ -108,6 +108,60 @@ func TestMatchesParsedShortcutWithFallback(t *testing.T) { } } +func TestMatchesParsedShortcutPriorityWithFallback(t *testing.T) { + psA := ParseShortcut("a") + psCtrlC := ParseShortcut("ctrl+c") + + // Direct match: priority 0 + evDirect := &KeyEvent{Type: PRESS, Key: "a"} + if p := evDirect.MatchesParsedShortcutPriorityWithFallback(psA, PRESS, ""); p != 0 { + t.Fatalf("direct match should have priority 0, got %d", p) + } + + // No match: priority -1 + evNoMatch := &KeyEvent{Type: PRESS, Key: "b"} + if p := evNoMatch.MatchesParsedShortcutPriorityWithFallback(psA, PRESS, "shifted,ascii"); p != -1 { + t.Fatalf("no match should have priority -1, got %d", p) + } + + // Shifted fallback at position 0 in "shifted,ascii": priority 1 + evShifted := &KeyEvent{Type: PRESS, Mods: SHIFT, Key: "A", ShiftedKey: "a"} + if p := evShifted.MatchesParsedShortcutPriorityWithFallback(psA, PRESS, "shifted,ascii"); p != 1 { + t.Fatalf("shifted fallback first in 'shifted,ascii' should have priority 1, got %d", p) + } + + // Shifted fallback at position 1 in "ascii,shifted": priority 2 + if p := evShifted.MatchesParsedShortcutPriorityWithFallback(psA, PRESS, "ascii,shifted"); p != 2 { + t.Fatalf("shifted fallback second in 'ascii,shifted' should have priority 2, got %d", p) + } + + // Shifted fallback only in "shifted": priority 1 (same as first position) + if p := evShifted.MatchesParsedShortcutPriorityWithFallback(psA, PRESS, "shifted"); p != 1 { + t.Fatalf("shifted fallback only in 'shifted' should have priority 1, got %d", p) + } + + // Shifted fallback not allowed: priority -1 + if p := evShifted.MatchesParsedShortcutPriorityWithFallback(psA, PRESS, "ascii"); p != -1 { + t.Fatalf("shifted fallback not in 'ascii' should have priority -1, got %d", p) + } + + // ASCII (alternate key) fallback at position 1 in "shifted,ascii": priority 2 + evASCII := &KeyEvent{Type: PRESS, Mods: CTRL, Key: "с", AlternateKey: "c"} + if p := evASCII.MatchesParsedShortcutPriorityWithFallback(psCtrlC, PRESS, "shifted,ascii"); p != 2 { + t.Fatalf("ascii fallback second in 'shifted,ascii' should have priority 2, got %d", p) + } + + // ASCII fallback at position 0 in "ascii,shifted": priority 1 + if p := evASCII.MatchesParsedShortcutPriorityWithFallback(psCtrlC, PRESS, "ascii,shifted"); p != 1 { + t.Fatalf("ascii fallback first in 'ascii,shifted' should have priority 1, got %d", p) + } + + // ASCII fallback only in "ascii": priority 1 + if p := evASCII.MatchesParsedShortcutPriorityWithFallback(psCtrlC, PRESS, "ascii"); p != 1 { + t.Fatalf("ascii fallback only in 'ascii' should have priority 1, got %d", p) + } +} + func TestMatchesParsedShortcutUnconditionalAlternateKey(t *testing.T) { // Unconditional match via MatchesPressOrRepeat (hardcoded shortcuts) ev := &KeyEvent{Type: PRESS, Mods: CTRL, Key: "с", AlternateKey: "c"}