Conversation
📝 WalkthroughWalkthroughA new keyboard event system is introduced to the VT emulator and mock terminal. The changes add a Changes
Sequence DiagramsequenceDiagram
participant Ext as External Caller
participant MT as MockTerm
participant Em as Emulator
participant Raw as rawSend
Ext->>MT: KeyEvent(KbdEvent)
activate MT
MT->>Em: KeyEvent(KbdEvent)
activate Em
Em->>Em: keyLegacy(KbdEvent)
activate Em
alt Printable Key
Em->>Raw: []byte{ASCII}
else Control Key (Space/Esc/Return/etc)
Em->>Raw: []byte{KeyByte}
else Ctrl+Letter
Em->>Raw: []byte{1..26}
end
deactivate Em
Em-->>MT: (on Escape) 150ms delay
deactivate Em
MT-->>Ext: (complete)
deactivate MT
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vt/emulate.go (1)
863-883: Consider handling Ctrl+uppercase letters.The Ctrl+letter fallback (line 880) only handles lowercase 'a'-'z'. If a user presses Ctrl+Shift+A, the
Codewould be 'A' (uppercase), which wouldn't match the condition and would be silently ignored. Consider whether Ctrl+uppercase should map to the same control codes as Ctrl+lowercase for legacy protocol compatibility.Potential enhancement
// fallback control key handling - if ev.Code >= 'a' && ev.Code <= 'z' && ev.Mod == ModCtrl { + if ((ev.Code >= 'a' && ev.Code <= 'z') || (ev.Code >= 'A' && ev.Code <= 'Z')) && ev.Mod == ModCtrl { + code := ev.Code + if code >= 'A' && code <= 'Z' { + code = code - 'A' + 'a' // normalize to lowercase + } - em.SendRaw([]byte{byte(ev.Code) - 'a' + 1}) + em.SendRaw([]byte{byte(code) - 'a' + 1}) }mock/term.go (1)
95-102: Consider making the Escape delay configurable.The 150ms delay after Escape correctly disambiguates standalone Escape from escape sequences. However, this fixed delay could slow down tests that inject many Escape keys. For future enhancement, consider making this delay configurable via a MockOpt.
Example approach:
type mockTerm struct { mb MockBackend em vt.Emulator escDelay time.Duration // configurable delay, default 150ms }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
mock/term.gomock/term_test.govt/emulate.govt/kbd.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:48.847Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:48.847Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
mock/term.go
🧬 Code graph analysis (3)
mock/term_test.go (3)
mock/term.go (1)
NewMockTerm(125-131)mock/backend.go (2)
MockOptSize(168-168)MockOptColors(173-173)vt/kbd.go (5)
KbdEvent(17-24)ModShift(30-30)KcReturn(62-62)ModCtrl(31-31)KcEsc(63-63)
mock/term.go (1)
vt/kbd.go (2)
KbdEvent(17-24)KcEsc(63-63)
vt/emulate.go (1)
vt/kbd.go (10)
KbdEvent(17-24)KcSpace(64-64)ModNone(29-29)ModShift(30-30)KcEsc(63-63)KcReturn(62-62)KcTab(61-61)KcBackspace(60-60)KcDelete(65-65)ModCtrl(31-31)
🔇 Additional comments (5)
vt/emulate.go (2)
42-44: LGTM! Interface extension for keyboard event support.The new
KeyEventmethod properly extends the Emulator interface to accept keyboard events, which is the foundation for keyboard protocol support.
863-883: Note: Many key types not yet handled.As expected for preliminary work, the legacy keyboard handler currently supports only:
- Printable ASCII characters
- Basic control keys (Space, Esc, Return, Tab, Backspace, Delete)
- Ctrl+letter combinations
Function keys, arrow keys, and other special keys are silently ignored. This is fine for initial implementation.
vt/kbd.go (1)
58-227: LGTM! Comprehensive keyboard code definitions.The KeyCode type and constants provide extensive coverage for various keyboard keys including:
- ASCII control characters
- Navigation keys
- Keypad keys
- Media and browser keys
- Function keys (F1-F34)
- Modifier keys
- IME-related keys
This provides a solid foundation for keyboard protocol support.
mock/term_test.go (1)
412-436: LGTM! Well-structured test for legacy keyboard events.The test properly exercises legacy keyboard event handling, including:
- Regular character keys
- Shift-modified keys
- Ctrl-modified keys (e.g., Ctrl+I producing Tab)
- Special keys (Return, Escape)
- Key release events (correctly ignored in legacy protocol)
The expected output correctly matches the legacy protocol behavior.
mock/term.go (1)
119-121: LGTM! KeyEvent properly exposed in the interface.The KeyEvent method is correctly added to the MockTerm interface, enabling external callers to inject keyboard events for testing.
| type KbdEvent struct { | ||
| Down bool // true if event is for key down event | ||
| Repeat int // if > 1, a repeat count | ||
| Code KeyCode // logical key code (X11 keysm, e.g. 'A') | ||
| Base KeyCode // base key code (physical key, e.g 'a'), may be zero if same as code | ||
| Mod Modifier // modifiers x | ||
| Utf string // if non-empty, the unicode content for this | ||
| } |
There was a problem hiding this comment.
Fix typo in comment.
Line 22: The comment // modifiers x appears to have an extraneous "x". Should be just // modifiers.
Fix
- Mod Modifier // modifiers x
+ Mod Modifier // modifiers📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type KbdEvent struct { | |
| Down bool // true if event is for key down event | |
| Repeat int // if > 1, a repeat count | |
| Code KeyCode // logical key code (X11 keysm, e.g. 'A') | |
| Base KeyCode // base key code (physical key, e.g 'a'), may be zero if same as code | |
| Mod Modifier // modifiers x | |
| Utf string // if non-empty, the unicode content for this | |
| } | |
| type KbdEvent struct { | |
| Down bool // true if event is for key down event | |
| Repeat int // if > 1, a repeat count | |
| Code KeyCode // logical key code (X11 keysm, e.g. 'A') | |
| Base KeyCode // base key code (physical key, e.g 'a'), may be zero if same as code | |
| Mod Modifier // modifiers | |
| Utf string // if non-empty, the unicode content for this | |
| } |
🤖 Prompt for AI Agents
In vt/kbd.go around lines 17 to 24, the field comment for Mod contains an
extraneous "x" ("// modifiers x"); update the comment to read simply "//
modifiers" so the comment is corrected; ensure only the comment text is changed
and no code or formatting is altered.
There was a problem hiding this comment.
Will fix in follow up shortly.
There was a problem hiding this comment.
@gdamore, sounds good! Thank you for confirming you'll address this typo.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #936 +/- ##
==========================================
+ Coverage 35.52% 35.91% +0.39%
==========================================
Files 34 34
Lines 3091 3110 +19
==========================================
+ Hits 1098 1117 +19
Misses 1935 1935
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We do not want to have Shift-Ctrl-A handled the same as Ctrl-A. Existing emulator implementations are a bit split as to whether they treat this as Ctrl-A or not. I prefer not to. (macOS terminal, kitty, do not. iTerm2 and Rio do.) What we might want to do is exclude CapsLock, but right now we aren't sending CapsLock at all. |
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.