Conversation
This is a start on work to support richer keyboard reporting in the emulator layer. Also a number of the tests have been simplified, and we addressed some spelling issues while here, and supporting code refactors have taken place. The keyboard handling in the emulator got several minor bugs fix, such as Ctrl-Shift should not generate sequences in legacy mode. Initial support for layouts includes the stock US and US International layouts, including support for AltGr and dead keys.
This simplifies things by putting test only functionality in a single package. The MockTerm will move here later as well.
📝 WalkthroughWalkthroughAdds a keyboard subsystem (Key/ScanCode/WinVK/BaseKey), layout-driven key→rune translation with dead‑keys and state, new MockTerm KeyTap/KeyPress/KeyRelease APIs and SetLayout, refactors KeyEvent to use Key/BaseKey/VK/SC, introduces PmWin32Input and AdvancedKeyboard, and relocates many tests into vt/tests while removing legacy vt/* test files. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Demo/Test
participant Mock as MockTerm
participant Layout as Layout Registry
participant Emu as Emulator
participant Backend as Backend
App->>Mock: KeyTap / KeyPress(Key, Modifiers)
activate Mock
Mock->>Layout: KeyToUTF(Key, Modifiers)
activate Layout
Layout-->>Mock: rune / BaseKey
deactivate Layout
Mock->>Mock: handle dead‑keys & repeat state
Mock->>Emu: Emit KeyEvent(Key, BaseKey, VK, SC, Utf)
deactivate Mock
activate Emu
Emu->>Emu: select legacy/Win32/Kitty/Events path
Emu->>Backend: output CSI/OSC sequences
deactivate Emu
Backend-->>App: terminal output / responses
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demos/beep/beep_test.go (1)
63-65: Error message inconsistency.The assertion checks for
cnt != 3but the error message says"!= 2". This appears to be a pre-existing issue, but worth correcting for clarity.Proposed fix
if cnt := mt.Bells(); cnt != 3 { - t.Errorf("incorrect bell count %d != 2", cnt) + t.Errorf("incorrect bell count %d != 3", cnt) }
🤖 Fix all issues with AI agents
In `@vt/backend.go`:
- Around line 129-136: The KeyboardMode constants are declared as bit flags
using "1 << iota", making the zero value invalid; change the declaration to use
plain iota so modes are single-select enums (e.g., declare KeyboardMode byte and
set KeyboardLegacy, KeyboardWin32, KeyboardKitty, KeyboardEvents using iota).
Update the constants block that defines KeyboardLegacy, KeyboardWin32,
KeyboardKitty, KeyboardEvents accordingly so uninitialized (zero) corresponds to
a defined mode; ensure SetKeyboardMode and GetKeyboardMode continue to
accept/return the KeyboardMode type without bitwise semantics.
In `@vt/layout.go`:
- Around line 280-307: KeysUsLower and KeysUsUpper are missing an entry for KeyZ
which causes KeyboardANSI and derived layouts to not map Z to a rune; add
KeyZ:'z' to the KeysUsLower map and KeyZ:'Z' to the KeysUsUpper map in
vt/layout.go (the maps named KeysUsLower and KeysUsUpper) so the ANSI keyboard
and derived layouts correctly return runes for Z.
- Around line 128-135: The release handling for modifier keys currently clears
bits directly from ks.mod causing stuck-clears when the other side of a modifier
remains pressed; instead, when handling a key release in the block that checks
l.Modifiers[k] (and related code around the other occurrence), recompute ks.mod
by iterating over the current set of pressed keys (e.g., the map/structure that
tracks wasPressed or pressed state) and OR-ing the corresponding modifier bits
from l.Modifiers for each still-pressed key, then set event.Mod = ks.mod and
return event; ensure you remove reliance on clearing a single bit on release and
update both places (the shown block and the other occurrence around lines
195-203) to use this recomputation approach.
In `@vt/tests/uslayout_test.go`:
- Around line 169-181: The test data contains a duplicated subtest key
"S-Alt-Ctrl-A" and the first occurrence mistakenly repeats vt.KeyLCtrl in its
modifiers; remove the duplicate row (keep a single "S-Alt-Ctrl-A" entry) and fix
the modifiers array for the remaining "S-Alt-Ctrl-A" entry so it lists the
correct unique modifiers (e.g., vt.KeyLAlt, vt.KeyLCtrl, vt.KeyLShift, vt.KeyA)
instead of repeating vt.KeyLCtrl.
🧹 Nitpick comments (5)
vt/key.go (3)
17-57: Consider hoisting the shifted-key map to avoid per-call allocations.
Shifted()constructs a map on every call. If this is hit per keystroke, a package-level map or aswitchwould reduce allocations.
208-346: Hoist the scan-code lookup table for hot-path efficiency.The map literal is rebuilt on every call. A package-level map/slice would avoid repeated allocations and reduce GC pressure.
495-635: Consider expanding VK mappings beyond A–Z.Digits/punctuation currently return
0; ifVKis relied on for reporting or layout translation, consider filling those mappings or documenting the limitation.vt/tests/util.go (1)
48-56: Addt.Helper()inReadFfor cleaner failure attribution.Keeps error lines pointing at the caller, consistent with the other helpers.
♻️ Suggested tweak
func ReadF(t *testing.T, term MockTerm) string { + t.Helper() buf := make([]byte, 128) n, err := term.Read(buf) if err != nil { t.Errorf("failed read: %v", err) return "" } return string(buf[:n]) }vt/tests/emulate_test.go (1)
190-193: Open TODOs for DECSLRM/RI test coverage.If you want, I can help draft the missing DECSLRMv3/4 and RIv5/6 test cases or open a tracking issue.
Also applies to: 334-335
| type KeyboardMode byte | ||
|
|
||
| const ( | ||
| KeyboardLegacy KeyboardMode = 1 << iota // KeyboardLegacy uses simple reporting in the VT220 fashion | ||
| KeyboardWin32 // Win32 input mode, reports both key release and press, includes scan codes and vk | ||
| KeyboardKitty // Kitty simple protocol, only disambiguation | ||
| KeyboardEvents // Kitty keyboard but reports events (press, release, repeat) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and understand the context
fd -t f "backend.go" | head -20Repository: gdamore/tcell
Length of output: 70
🏁 Script executed:
# Search for KeyboardMode usage throughout the codebase
rg "KeyboardMode" -A 3 -B 3 | head -100Repository: gdamore/tcell
Length of output: 863
🏁 Script executed:
# Look for bitwise operations that would suggest bitmask usage
rg "KeyboardMode.*[&|^]|[&|^].*KeyboardMode" -A 2 -B 2Repository: gdamore/tcell
Length of output: 39
🏁 Script executed:
# Check struct definitions that contain KeyboardMode
rg "KeyboardMode" --type go | grep -E "(struct|type)" -A 10Repository: gdamore/tcell
Length of output: 306
🏁 Script executed:
# Search for AdvancedKeyboard interface implementations and SetKeyboardMode usage
rg "SetKeyboardMode|GetKeyboardMode" -A 3 -B 3 --type goRepository: gdamore/tcell
Length of output: 248
🏁 Script executed:
# Search for any variable initialization or default values for KeyboardMode
rg "KeyboardMode\s*=" -B 2 -A 2 --type goRepository: gdamore/tcell
Length of output: 449
🏁 Script executed:
# Check if there are any zero-value assignments or comparisons
rg "KeyboardMode\(0\)|== 0|== KeyboardMode" --type goRepository: gdamore/tcell
Length of output: 3251
🏁 Script executed:
# Search for implementations of AdvancedKeyboard by looking for SetKeyboardMode method definitions
rg "func.*SetKeyboardMode" -B 5 -A 10 --type goRepository: gdamore/tcell
Length of output: 39
🏁 Script executed:
# Check the vt/ directory structure for where keyboard handling is implemented
fd -t f "\.go$" vt/ | head -20Repository: gdamore/tcell
Length of output: 381
🏁 Script executed:
# Search for any struct that might have keyboard mode field
rg "struct\s*{" -A 30 | grep -B 30 -i keyboard | head -50Repository: gdamore/tcell
Length of output: 140
🏁 Script executed:
# Look at vt/mode.go which likely contains mode-related code
cat -n vt/mode.go | head -100Repository: gdamore/tcell
Length of output: 5348
🏁 Script executed:
# Check vt/vt.go to understand the main structure
cat -n vt/vt.go | head -150Repository: gdamore/tcell
Length of output: 1150
🏁 Script executed:
# Search for TODO or comments about keyboard mode
rg "TODO|FIXME|XXX" vt/ --type go | grep -i keyboardRepository: gdamore/tcell
Length of output: 155
🏁 Script executed:
# Check if AdvancedKeyboard is used anywhere - might be in an export or struct embedding
rg "AdvancedKeyboard" --type goRepository: gdamore/tcell
Length of output: 104
🏁 Script executed:
# Double-check if KeyboardMode is used anywhere else or if there are any related changes
rg "Keyboard" vt/backend.go -B 5 -A 5 --type goRepository: gdamore/tcell
Length of output: 737
🏁 Script executed:
# Look for any comments or documentation about the keyboard mode design
cat -n vt/backend.go | head -150Repository: gdamore/tcell
Length of output: 6831
Change KeyboardMode to use simple iota for consistency with single-select enum pattern.
The 1 << iota pattern creates invalid power-of-two values (1, 2, 4, 8) and makes the zero-value undefined. Since SetKeyboardMode() and GetKeyboardMode() operate on a single mode at a time (not bitwise combinations), this should use simple iota like MouseReporting elsewhere in the same file. This prevents implementors from accidentally creating invalid modes through uninitialized fields.
Suggested change
const (
- KeyboardLegacy KeyboardMode = 1 << iota // KeyboardLegacy uses simple reporting in the VT220 fashion
+ KeyboardLegacy KeyboardMode = iota // KeyboardLegacy uses simple reporting in the VT220 fashion
KeyboardWin32 // Win32 input mode, reports both key release and press, includes scan codes and vk
KeyboardKitty // Kitty simple protocol, only disambiguation
KeyboardEvents // Kitty keyboard but reports events (press, release, repeat)
)📝 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 KeyboardMode byte | |
| const ( | |
| KeyboardLegacy KeyboardMode = 1 << iota // KeyboardLegacy uses simple reporting in the VT220 fashion | |
| KeyboardWin32 // Win32 input mode, reports both key release and press, includes scan codes and vk | |
| KeyboardKitty // Kitty simple protocol, only disambiguation | |
| KeyboardEvents // Kitty keyboard but reports events (press, release, repeat) | |
| ) | |
| type KeyboardMode byte | |
| const ( | |
| KeyboardLegacy KeyboardMode = iota // KeyboardLegacy uses simple reporting in the VT220 fashion | |
| KeyboardWin32 // Win32 input mode, reports both key release and press, includes scan codes and vk | |
| KeyboardKitty // Kitty simple protocol, only disambiguation | |
| KeyboardEvents // Kitty keyboard but reports events (press, release, repeat) | |
| ) |
🤖 Prompt for AI Agents
In `@vt/backend.go` around lines 129 - 136, The KeyboardMode constants are
declared as bit flags using "1 << iota", making the zero value invalid; change
the declaration to use plain iota so modes are single-select enums (e.g.,
declare KeyboardMode byte and set KeyboardLegacy, KeyboardWin32, KeyboardKitty,
KeyboardEvents using iota). Update the constants block that defines
KeyboardLegacy, KeyboardWin32, KeyboardKitty, KeyboardEvents accordingly so
uninitialized (zero) corresponds to a defined mode; ensure SetKeyboardMode and
GetKeyboardMode continue to accept/return the KeyboardMode type without bitwise
semantics.
There was a problem hiding this comment.
I'm not sure that this will be an enumeration and not a mask yet.
There was a problem hiding this comment.
@gdamore, that makes sense! Keeping the bitmask pattern (1 << iota) preserves the flexibility to support combined modes in the future if needed, even though the current API uses a single mode at a time. I'll note this design decision.
✏️ Learnings added
Learnt from: gdamore
Repo: gdamore/tcell PR: 1012
File: vt/backend.go:129-136
Timestamp: 2026-01-25T20:47:46.305Z
Learning: In vt/backend.go, KeyboardMode is intentionally defined using the bitmask pattern (1 << iota) rather than simple iota enumeration, to preserve flexibility for potential future support of combined keyboard modes, even though the current SetKeyboardMode/GetKeyboardMode API operates on a single mode at a time.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: gdamore
Repo: gdamore/tcell PR: 969
File: vt/emulate.go:154-161
Timestamp: 2026-01-04T09:24:25.201Z
Learning: In vt/emulate.go, `PmMouseSgrPixel` is intentionally defined but not initialized in the mouse modes block (around line 154-161) because pixel-level mouse reporting is not yet supported. It's a placeholder for future work that will be implemented when other pixel-level features (sixel, kitty graphics protocol, pixel window sizes) are added. It is not changeable at this time by design.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
==========================================
- Coverage 84.55% 80.26% -4.29%
==========================================
Files 43 47 +4
Lines 4382 5137 +755
==========================================
+ Hits 3705 4123 +418
- Misses 529 850 +321
- Partials 148 164 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Also the VKey() method for Key was incomplete and incorrect semantically as we need to have layout information to properly determine the virtual key. So it is removed now.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@vt/layout.go`:
- Around line 137-162: The assignment event.Utf = string(r) emits a NUL when
KeyToUTF returns 0; change the tail logic in the same block handling KeyToUTF
(the variables/functions: l.KeyToUTF, r, ks.deadKey, event.Utf, ks.lastRune) so
that you only set event.Utf (and ks.lastRune) when r != 0; when r == 0 leave
event.Utf empty (and do not update ks.lastRune) and return event as-is.
- Around line 661-685: The allLayouts map is accessed concurrently by
RegisterLayout, GetLayout, and Layouts and must be guarded to avoid Go map
concurrent access panics; add a package-level sync.RWMutex (e.g., layoutsMu) and
use layoutsMu.Lock()/Unlock() around writes in RegisterLayout and
layoutsMu.RLock()/RUnlock() around reads in GetLayout and Layouts (including the
iteration in Layouts), keeping the existing allLayouts variable and behavior
otherwise.
♻️ Duplicate comments (1)
vt/layout.go (1)
195-201: Modifier release still drops other held modifiers.
Same concern as earlier review: releasing one side clears the modifier bit even if the other side is still pressed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@vt/layout.go`:
- Around line 385-390: The KeysPadOps map (mapping
KeyPadMul/KeyPadAdd/KeyPadSub/KeyPadDiv to '*','+','-','/') is never referenced
by KeyboardANSI.Maps so KeyToUTF won't produce those runes; add a ModifierMap
entry that includes KeysPadOps to the KeyboardANSI.Maps slice (so the keypad
operator keys are looked up), using an appropriate modifier set (e.g., no
modifier or a NumLock-agnostic entry) so KeyPadMul/KeyPadAdd/KeyPadSub/KeyPadDiv
resolve to the expected runes.
🧹 Nitpick comments (1)
vt/layout.go (1)
680-696: Thread safety correctly implemented; considersync.RWMutexfor better read concurrency.The layout registry is now properly protected from concurrent access. For slightly better performance with concurrent reads (which are likely more common than writes),
sync.RWMutexcould be used instead ofsync.Mutex.♻️ Optional: Use RWMutex for read-heavy workloads
-var layoutsLock sync.Mutex +var layoutsLock sync.RWMutex // RegisterLayout registers the given layout. func RegisterLayout(km *Layout) { layoutsLock.Lock() allLayouts[km.Name] = km layoutsLock.Unlock() } // GetLayout returns a keyboard layout for the given name. // The layout must have been previously registered with RegisterLayout. // (Builtin layouts do this as a consequence of importing the layout.) func GetLayout(name string) *Layout { - layoutsLock.Lock() - defer layoutsLock.Unlock() + layoutsLock.RLock() + defer layoutsLock.RUnlock() return allLayouts[name] }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@vt/key.go`:
- Around line 438-455: The KeyIsoBackSlash and KeyIsoSlash entries currently map
to base-layout characters ('\\' and '/'); instead, update their handling to
follow the Kitty keyboard protocol by removing reliance on base-layout-key
values and populating unicode-key-code (and shifted-key if needed) for
KeyIsoBackSlash and KeyIsoSlash so the actual character produced by the physical
ISO key is reported via unicode code points rather than treating them as
US-layout base keys; locate the KeyIsoBackSlash and KeyIsoSlash symbols in
vt/key.go and change their mapping logic to emit unicode-key-code values for
'¥'/'\\'/'/' as appropriate and set shifted-key fields when the shifted
character differs.
🧹 Nitpick comments (1)
vt/key.go (1)
540-543: Add clarifying comment explaining the E0 prefix encoding convention for extended keys.The 0xe0 prefix encoding (e.g.,
0xe046for Pause,0xe052for Insert) consistently represents Windows extended keys by packing the E0 extended prefix into the high byte. This design choice works well for storing multi-byte scan codes in a 16-bit value and is applied consistently across all navigation and extended-key mappings. However, a brief comment explaining this encoding scheme would help maintainers and users of theScanCodetype understand the convention without having to deduce it from the patterns in the map.
gdamore
force-pushed
the
keyboard-layouts
branch
from
ccfda9b to
83f361f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@vt/layout.go`:
- Around line 73-90: The reset path doesn't clear the KeyboardState.pressed map
so changing layout via SetLayout can leave stale pressed entries; update
KeyboardState.reset (or extend clearRepeat) to also clear the pressed state
(e.g., reset or reinitialize the pressed map/slice) so pressing modifiers after
SetLayout isn't suppressed, referencing the methods KeyboardState.reset,
KeyboardState.clearRepeat and the SetLayout caller to make the change.
In `@vt/tests/key_test.go`:
- Around line 154-157: The test's assertion uses a malformed format string in
VerifyF causing a trailing bare '%' — update the format string in the t.Run
block where VerifyF is called (the call referencing VerifyF,
cases[i].key.ScanCode(), and cases[i].code) to include matching verbs for both
values (e.g. "%x did not match %x") so the number of format verbs matches the
provided arguments and the failure message prints both ScanCode() and code
correctly.
♻️ Duplicate comments (1)
vt/key.go (1)
360-455: ISO keys should not rely on Kitty base‑layout keys.
KeyIsoBackSlashandKeyIsoSlashare mapped to US base keys ('\\'and'/'). Kitty’s base‑layout key represents a PC‑101 key, which does not include ISO‑specific keys. These should be represented via unicode‑key‑code (and shifted‑key) instead of base‑layout keys. This also affects the expectations invt/tests/key_test.go.💡 Suggested adjustment (omit ISO base mappings)
- KeyIsoBackSlash: '\\', // TODO: does kitty have another mapping for this? + // KeyIsoBackSlash: omit base‑layout mapping; use unicode‑key‑code instead @@ - KeyIsoSlash: '/', // Kitty cannot discriminate? + // KeyIsoSlash: omit base‑layout mapping; use unicode‑key‑code insteadKitty keyboard protocol base-layout-key ISO backslash HID 0x64 mapping
1 participant
This includes enhanced support for keyboard layouts in the emulator subsystem with US and US International keyboard layouts to start. It has rich testing. Along these lines some tests were refactored and moved into a common tests sub-package of the vt package, and several edge case bugs in the keyboard handling were fixed as well.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.