fix: Fix input handling for ESC-char as a meta prefix.#895
Conversation
This requires the caller of the input processor to handle their own delays, but they can check whether there is unfinished work by calling inputProcessor.Waiting. If that is true they should call it again in a while, otherwise they can sleep until new characters arrive.
WalkthroughInput parsing was renamed and refactored: Changes
Sequence Diagram(s)sequenceDiagram
participant T as tScreen (main loop)
participant P as inputParser
participant Q as eventQ
participant Timer as 100ms timer
T->>P: feed bytes (ScanUTF8 / write)
P->>Q: post immediate events (if any)
alt P.Waiting() == true
T->>Timer: start 100ms timer
Timer-->>T: timer fires
T->>P: t.input.Scan() (timed follow-up)
P->>Q: post delayed events (Escape, Alt+Rune, completed sequences)
else P idle
T-->>Timer: stop/clear timer
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tscreen.go (1)
20-37: Timer-driven rescan usinginput.Waitinglooks correct; optional improvement for timer reuseThe pattern of setting
ta = time.After(100 * time.Millisecond)only whent.input.Waiting()is true and clearing it otherwise correctly deferst.input.Scan()until escape/meta sequences have had time to disambiguate, and the nil-channel case in the select safely disables the timer branch. The only trade-off is that repeated ESC-prefix traffic can create short-livedtime.Aftertimers whose channels are never read oncetais reassigned; if this path ever becomes hot, consider switching to a singletime.Timerinstance withStop/Resetand reusingtimer.Cto avoid extra allocations and stray timers, though it’s not required for correctness here.Also applies to: 923-952
input.go (2)
84-88: Consider documenting the newWaitingcontract for other potential callers
Waiting()currently reports onlyip.state != inpStateInit, which is exactly whattscreen.mainLoopneeds to decide whether to schedule a delayedScan(). Since this is now the public hook that replaces the old internal timer, a short doc comment clarifying that “true” means the caller should arrange to callScan()again after a small delay (and that no internal timer will fire) would help future users ofinputProcessoravoid assuming it is self‑timing.
69-70: escTimeout/timer/expire appear vestigial; consider consolidating or removingWith timeouts now handled inside
scan()viakeyTimeand the caller-managedScan()calls,escTimeout()and thetimer/expirefields no longer seem to be wired up from this file. If there are no remaining external callers ofescTimeout, dropping these fields and the method (or delegating the end-of-scan timeout logic toescTimeoutto keep a single implementation) would reduce confusion around which path actually governs ESC/meta timeouts; otherwise, it’s worth double-checking that any remaining users see behavior consistent with the newescChar‑based rules.Also applies to: 121-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
input.go(9 hunks)input_test.go(5 hunks)tscreen.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
input.go (1)
key.go (6)
NewEventKey(249-308)KeyEsc(516-516)ModNone(325-325)KeyRune(339-339)ModAlt(322-322)KeyEscape(517-517)
input_test.go (1)
key.go (4)
KeyEscape(517-517)ModAlt(322-322)EventKey(45-50)Key(332-332)
🔇 Additional comments (4)
input_test.go (3)
325-328: Explicitip.Scan()after a short delay aligns tests with the new timeout contractUpdating the UTF‑8 and empty-input tests to sleep briefly and then call
ip.Scan()ensures they still pass now that timeout handling is driven by callers instead of an internal timer; this accurately reflects the newWaiting/Scancontract and keeps the assertions meaningful without changing the semantics under test.Also applies to: 354-357
372-384:MultipleNullBytesloop still validates behavior correctlySwitching to
for i := range 3while draining three events and checking each forKeyRune+" "+ModCtrlpreserves the original intent and keeps the test straightforward, with no behavioral regressions.
527-528: Good coverage for ESC‑ESC meta prefix and timeout-driven deliveryAdding the
"Esc-Esc"case expectingKeyEscapewithModAlt, plus the timeout branch that callsip.Scan()and immediately rechecksevch, exercises the new behavior where ESC‑prefixed meta sequences are only emitted after a delay. This pattern correctly handles both immediate and timeout-driven key events and should catch regressions in meta‑ESC handling.Also applies to: 578-603
input.go (1)
62-70: Waiting/Scan API and timeout flush logic look coherent with the new caller-driven modelTracking
escCharandkeyTimeinsidescan(), exposingWaiting()asstate != inpStateInit, and adding theScan()wrapper that takes the mutex give callers (liketScreen.mainLoop) a clear, race-free way to drive timeout processing from outside the processor. The end-of-scan timeout block that, after 50ms of inactivity, emits eitherKeyEscape(forinpStateEsc, relying onip.escapedto addModAltfor ESC‑ESC) or an Alt‑rune based onescChar, then resetsstatetoinpStateInit, matches the intended “ESC as meta prefix” semantics without disturbing unambiguous CSI/SS3 sequences.Also applies to: 417-422, 596-605, 952-956
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tscreen.go (1)
923-952: Timer‑drivenScan()integration withWaiting()is sound; consider a named timeout constantThe
ta <-chan time.Timestate plustime.After(100 * time.Millisecond)whent.input.Waiting()is true correctly defersScan()until ambiguous input (e.g. lone ESC) has aged, and the nil channel disables the case when idle. To avoid a magic number and keep this aligned with the parser/tests, you may want to promote the 100ms delay to a shared constant.input_test.go (1)
306-437: UTF‑8 and edge‑case tests are accurate; time‑based behavior is well coveredThe UTF‑8 checks (
TestInputUTF8Characters) and edge‑case subtests inTestInputEdgeCases(empty input, multiple NULs, 0x1F/0x20 boundaries) correctly exercise the parser’s rune handling and ensure no spurious events are emitted. The short sleeps and timeouts used here are reasonable for CI but do tie the tests slightly to wall‑clock timing; keep that in mind if you ever lengthen the internal timeout inscan().input.go (1)
711-789: Verifyfor rangeover integers against the project’s minimum Go versionThe loop
for range rpt { if key != KeyRune || chr != 0 { ip.post(NewEventKey(key, string(chr), mod)) } }(and similar
for range 3/10usages ininput_test.go) relies on Go’s integerrangesupport, which is only available in Go 1.22+. If tcell is still expected to build on earlier Go releases, these loops will not compile and should be rewritten as traditional index loops (e.g.,for i := 0; i < rpt; i++ { … }). Otherwise, ensurego.modand documentation clearly state Go 1.22+ as the minimum supported version.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
input.go(13 hunks)input_test.go(20 hunks)tscreen.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
input_test.go (2)
event.go (1)
Event(23-26)key.go (4)
KeyEscape(517-517)ModAlt(322-322)EventKey(45-50)Key(332-332)
🔇 Additional comments (11)
tscreen.go (1)
175-176: Switch toinputParserand SetSize wiring looks correctUsing
newInputParser(t.eventQ)and forwarding terminal resize notifications viat.input.SetSize(ws.Width, ws.Height)cleanly plugs the new parser into the existing screen lifecycle; no issues spotted here.Also applies to: 255-258, 848-849
input_test.go (4)
22-171: Control/NULL/printable character tests match the parser’s scan semantics
TestInputNullByte,TestInputControlKeys, andTestInputNullVsOtherControlCharscorrectly reflect howscan()maps 0x00–0x1F intoEventKeyvalues (includingKeyRune " "withModCtrlfor NUL andKeyCtrlA–KeyCtrlZviaNewEventKey), and give good regression coverage around the control/printable boundary.
173-305: Printable, special‑key, and sequential‑input tests look consistent with current behavior
TestInputPrintableCharacters,TestInputSpecialKeys, andTestInputSequentialInputall assert results that line up with the newscan()logic for ASCII runes, backspace/tab/enter, and mixed control/printable sequences; no issues seen.
439-517: Concurrency and state‑transition tests validate mutex and state‑machine behavior
TestInputConcurrentAccessconfirms thatScanUTF8is safe under concurrent callers (thanks to the internal mutex), andTestInputStateTransitionsverifies that the NUL fix doesn’t leave the parser in a non‑initial state. Both tests align with the implementation and provide useful regression coverage.
519-606:TestSpecialKeyscorrectly exercises ESC vs ESC‑ESC meta‑prefix semanticsThe
EscandEsc-Esccases, combined with the “timeout thenip.Scan()” pattern, accurately mirror the new lone‑ESC vs meta‑ESC behavior inscan(). Other CSI and SS3 cases (arrows, function keys, modifiers) match thecsiAllKeys/ss3Keysmappings. No changes needed here.input.go (6)
37-79:inputStateandinputParserrefactor is coherent with downstream usageThe new
inputStateenum (istInit,istEsc,istCsi, etc.) and the expandedinputParserfields (buf,utfBuf,strBuf,csiParams,escChar,keyTime,nested, …) align with howscan(),ScanUTF8(),handleCsi(), andhandleWinKey()operate. The structure reads cleanly and matches the intended state‑machine design.
94-123:SetSizeandpostbehavior remains consistent and safeForwarding
SetSizeto a nested parser when present and using a goroutine to updaterows/colsand postNewEventResizeunder the mutex preserves prior semantics, andpost’s handling ofescaped(Alt meta) and paste start/end keys looks correct in the new parser context.
403-592: ESC/meta and timeout logic inscan()correctly handles lone ESC, ESC‑ESC, and ambiguous prefixesThe reworked
scan():
- Treats a single ESC as entering
istEscwithescChar=0, then, after >50ms with no further input, postsKeyEscapewith no modifiers.- Treats
ESC ESCas settingescaped=trueinistEsc; on timeout it postsKeyEscapeand upgrades it toModAltviapost(), giving the expected Meta‑Escape.- Uses
escCharplus the timeout path to resolve ambiguous prefixes likeESC [orESC ]into Alt+[or Alt+]when no full CSI/OSC sequence arrives.- Resets the state machine after a gap (
ip.state = istInit), preventing stale state from leaking across keystrokes.This matches the PR’s intent and the updated tests; no changes needed here.
594-603:handleOsccorrectly resets state and posts clipboard eventsBy setting
ip.state = istInitup front and decoding OSC 52 clipboard payloads via base64 intoNewEventClipboard,handleOsckeeps the state machine consistent while preserving existing OSC‑52 behavior.
633-709: Mouse handling with SGR (CSI <... M/m) remains correct with bounds clipping
handleMouse’s use ofrows/colsto clip coordinates, plus its treatment of motion vs scroll bits and modifier extraction, is consistent with standard XTerm mouse reporting and with existing tcell semantics. The refactor to a method oninputParserlooks fine.
913-943: Locking strategy forScanUTF8/Scankeeps the state machine race‑freeAccumulating bytes into
utfBuf, decoding intobuf, and then callingscan()underip.linScanUTF8, plus reusing the samescan()under lock inScan(), ensuresstate,escChar, andkeyTimeare updated atomically. This matches howWaiting()readsstateand should prevent subtle races between input ingestion and timeout‑driven scans.
This is just some stylistic cleanups for internal symbols and improved doc comments.
48166e2 to
8a470ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
input.go (2)
582-582: Consider: Document the inter-character timeout threshold.The 50ms threshold here is the inter-character timeout for escape sequences, while tscreen uses a 100ms timer for periodic retries. The interaction is subtle:
- If characters arrive >50ms apart, timeout triggers on next
scan()call- The 100ms external timer ensures
Scan()is eventually called whenWaiting()is trueThis works correctly but the coordination between the two timeouts could be clearer in comments.
913-935: LGTM: UTF-8 handling refactored to use byte buffer.The refactoring accumulates bytes in
utfBufand decodes runes incrementally, correctly handling both complete and partial UTF-8 sequences.Minor: The comment at line 919 says "fast path, basic ascii" but the check
< 0x7Fincludes control characters (0x00-0x1F). Consider "fast path, single-byte characters" for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
input.go(13 hunks)input_test.go(20 hunks)tscreen.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
input_test.go (2)
event.go (1)
Event(23-26)key.go (4)
KeyEscape(517-517)ModAlt(322-322)EventKey(45-50)Key(332-332)
input.go (2)
event.go (1)
Event(23-26)key.go (6)
NewEventKey(249-308)KeyRune(339-339)ModNone(325-325)KeyBacktab(361-361)KeyEscape(517-517)ModAlt(322-322)
🔇 Additional comments (8)
tscreen.go (3)
33-33: LGTM: Time package import added for timer support.The time import is necessary for the new timer-based debounce logic in
mainLoop.
175-175: LGTM: Type rename from inputProcessor to inputParser.The rename is consistently applied across field declaration and initialization.
Also applies to: 257-257
926-952: LGTM: Timer-based debounce correctly implements caller-managed delays.The implementation correctly handles the new API contract:
- Checks
Waiting()after each input batch- Schedules a 100ms timer when parser is mid-sequence
- Disables timer (sets
ta = nil) when parser returns to idle- Calls
Scan()on timeout to finalize incomplete sequencesThe concurrency is safe since both
Waiting()andScan()use internal locking.input_test.go (2)
22-26: LGTM: Test updates reflect the API rename.All tests consistently updated from
newInputProcessortonewInputParserand test names simplified fromTestInputProcessor*toTestInput*.Also applies to: 52-55, 99-99, 131-135, 173-175, 197-197, 223-224, 241-241, 264-267, 306-307, 323-323, 348-352, 368-368, 389-389, 415-415, 439-443, 482-486, 572-572
326-327: LGTM: Tests validate deferred Scan() behavior and ESC-ESC handling.The additions correctly test the new API contract:
- Explicit
Scan()calls after delays validate incomplete sequence handling- Esc-Esc test case (line 528) confirms Alt-modified escape key behavior
- TestSpecialKeys fallback logic mirrors the tscreen timer pattern
Also applies to: 355-356, 528-528, 587-602
input.go (3)
37-79: LGTM: Type rename and new fields support the refactoring.The rename from
inputProcessortoinputParseris consistently applied. New fields (utfBuf,strBuf,csiParams,csiInterm,escChar,keyTime,nested) are appropriately scoped private fields that support the timeout handling and buffer management refactoring.
81-92: LGTM: Waiting() correctly exposes parser state.The method safely returns whether the parser is mid-sequence (state != istInit), enabling callers to implement timeout-based finalization. The mutex ensures thread-safe access.
937-943: LGTM: Scan() correctly exposes deferred processing.The method provides the public API for callers to trigger timeout-based finalization after detecting
Waiting()is true. The lock/unlock pattern matchesScanUTF8.
| if ip.state != istInit && time.Since(ip.keyTime) > time.Millisecond*50 { | ||
| if ip.state == istEsc { | ||
| ip.post(NewEventKey(KeyEscape, "", ModNone)) | ||
| } else if ec := ip.escChar; ec != 0 { | ||
| ip.post(NewEventKey(KeyRune, string(ec), ModAlt)) | ||
| } else { | ||
| } | ||
| // if we take too long between bytes, reset the state machine. | ||
| ip.state = istInit | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the context around lines 582-591 in input.go
head -n 600 input.go | tail -n 50Repository: gdamore/tcell
Length of output: 1417
🏁 Script executed:
# Let's see the full scan() method to understand state transitions
sed -n '380,600p' input.go | head -n 100Repository: gdamore/tcell
Length of output: 2284
🏁 Script executed:
# Look for tests related to escape sequences and timeouts
grep -n "50.*[Mm]illisecond\|timeout\|Escape\|ESC.*\[" input_test.go | head -20Repository: gdamore/tcell
Length of output: 496
🏁 Script executed:
# Search for any documentation comments about escChar or timeout behavior
grep -B5 -A5 "escChar\|keyTime" input.go | head -40Repository: gdamore/tcell
Length of output: 1576
🏁 Script executed:
# Search for tests that specifically test incomplete CSI sequences with timeouts
grep -n "CSI\|escChar\|Alt.*\[" input_test.go | head -30Repository: gdamore/tcell
Length of output: 1542
🏁 Script executed:
# Look for any comments or documentation about the escChar and timeout behavior
grep -B3 -A3 "if ec := ip.escChar" input.goRepository: gdamore/tcell
Length of output: 312
🏁 Script executed:
# Check if there are integration tests or behavior documentation
grep -r "incomplete.*sequence\|timeout.*escape\|CSI.*timeout" . --include="*.go" --include="*.md"Repository: gdamore/tcell
Length of output: 39
🏁 Script executed:
# Look at the actual state machine transitions to understand if timeout behavior is documented
sed -n '380,450p' input.go | grep -A2 -B2 "case istEsc"Repository: gdamore/tcell
Length of output: 109
Document or reconsider timeout behavior for incomplete escape sequences.
The timeout logic (lines 582–591) posts incomplete escape sequences as Alt-modified keys. For example, if a user types ESC [ (CSI introducer) and the sequence doesn't complete within 50ms, the code emits Alt+[ as a single event. While this is internally consistent with treating ESC as a meta/Alt prefix, it is undocumented and has no test coverage.
This behavior could surprise downstream applications that expect either separate ESC and [ events or sequence rejection. Consider:
- Adding test cases for incomplete sequences that timeout
- Adding inline documentation explaining why the escChar is emitted as Alt-modified rather than discarding the sequence or emitting separate events
🤖 Prompt for AI Agents
input.go around lines 582-591: the timeout path currently converts an incomplete
escape sequence into an Alt-modified key (emitting Alt+<char> after 50ms), which
is undocumented and untested; add unit tests that cover timeout cases (e.g.,
feed ESC then '[' with a delay >50ms and assert Alt+'[' is emitted), and add an
inline comment above this block explaining the intentional behavior and
rationale (why we treat ESC as Meta/Alt and choose to emit Alt+char on timeout
rather than splitting or discarding), plus a TODO note suggesting making this
behavior configurable if desired.
This requires the caller of the input processor to handle their own delays, but they can check whether there is unfinished work by calling inputProcessor.Waiting. If that is true they should call it again in a while, otherwise they can sleep until new characters arrive.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.