Minor refactoring for common VT definitions into sub package#918
Conversation
We are starting to think a common VT package that we might use for the mock, is likely to be useful for someone wanting to implement more complete terminal handling or even a full terminal emulator.
WalkthroughAdds a new vt package (coordinate and private-mode types) and refactors input parsing, mock TTY, tests, and screen control to use vt.PrivateMode/ModeStatus and vt.Row/vt.Col instead of primitive ints and bespoke private-mode logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #918 +/- ##
==========================================
- Coverage 29.68% 29.45% -0.23%
==========================================
Files 40 41 +1
Lines 3743 3731 -12
==========================================
- Hits 1111 1099 -12
Misses 2534 2534
Partials 98 98 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
vt/mode.go (1)
57-60: Remove unnecessary parentheses.The outer parentheses around the
fmt.Sprintfcall are redundant.🔎 Proposed fix
func (pm PrivateMode) Reply(status ModeStatus) string { - return (fmt.Sprintf("\x1b[?%d;%d$y", pm, status)) + return fmt.Sprintf("\x1b[?%d;%d$y", pm, status) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
input.go(3 hunks)input_test.go(3 hunks)mock/mock.go(11 hunks)tscreen.go(13 hunks)vt/coord.go(1 hunks)vt/mode.go(1 hunks)vt/vt.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
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:
input.govt/vt.gotscreen.gomock/mock.go
🧬 Code graph analysis (3)
input.go (1)
vt/mode.go (2)
PrivateMode(23-23)ModeStatus(63-63)
tscreen.go (2)
vt/mode.go (10)
PmResizeReports(39-39)PmMouseSgr(35-35)PmShowCursor(30-30)PmSyncOutput(38-38)PmMouseButton(31-31)PmMouseDrag(32-32)PmMouseMotion(33-33)PmBracketedPaste(37-37)PmFocusReports(34-34)PmAutoMargin(27-27)screen.go (3)
MouseButtonEvents(263-263)MouseDragEvents(264-264)MouseMotionEvents(265-265)
input_test.go (1)
vt/mode.go (8)
ModeNA(66-66)ModeOff(68-68)PmAutoMargin(27-27)ModeOn(67-67)PmShowCursor(30-30)ModeOnLocked(69-69)PmBlinkCursor(29-29)ModeOffLocked(70-70)
🔇 Additional comments (28)
vt/vt.go (1)
1-18: LGTM - New vt package foundation.This establishes the
vtpackage namespace for VT-derived terminal definitions. The package documentation clearly explains its purpose.Minor nit: Lines 4-5 have minor inconsistencies with the standard Apache 2.0 license text ("file" vs "this file", lowercase "license" vs "License"), but these appear consistent with other files in the codebase, so likely intentional style.
vt/coord.go (1)
27-31: Good design: Distinct Row/Col types prevent axis confusion.The use of separate
RowandColtypes is a solid approach to catch coordinate mix-ups at compile time.input.go (4)
38-39: LGTM - Import of vt package.Correctly imports the new
vtsubpackage for private mode type definitions.
872-878: LGTM - Private mode response handling uses vt types.The validation
params[1] >= 0 && params[1] <= 4correctly matches thevt.ModeStatusconstants (ModeNA=0, ModeOn=1, ModeOff=2, ModeOnLocked=3, ModeOffLocked=4). The event is only posted for valid status values.
881-884: Good addition: Explicit state reset at CSI handler entry.Adding
ip.state = istInitat the beginning ofhandleCsiensures the state machine is properly reset regardless of the path taken through the function. This is defensive and prevents potential state leakage.
1093-1097: LGTM - eventPrivateMode struct updated to use vt types.The struct now uses
vt.PrivateModefor the mode number andvt.ModeStatusfor the status value, providing type safety and clearer semantics.tscreen.go (6)
36-37: LGTM - vt package import added.The import enables use of the centralized VT private mode definitions.
321-324: LGTM - Uses vt constants and Status.Changeable().The switch cases now use the vt package constants (
vt.PmResizeReports,vt.PmMouseSgr) and theStatus.Changeable()method to determine if the mode can be enabled/disabled, replacing the previoususable()check.
837-853: LGTM - Mouse mode handling refactored to use vt package.All mouse-related private modes are now controlled through the vt package's
Enable()andDisable()methods, providing consistent escape sequence generation.
1150-1151: LGTM - Private mode queries use vt.Query() method.The initialization queries for resize reports and SGR mouse support now use the vt package's
Query()method for generating the DECRQM escape sequences.
1177-1178: LGTM - Terminal engagement uses vt Enable/Disable methods.Cursor hiding and auto-margin disabling during terminal engagement are now handled through the vt package methods.
1222-1232: LGTM - Terminal disengagement restores modes via vt methods.The disengage flow correctly re-enables the cursor and auto-margin using the vt package's
Enable()method, maintaining consistency with the engage flow.input_test.go (3)
24-25: LGTM - vt package import for test constants.Test file correctly imports the vt package to access mode constants.
622-628: LGTM - Test cases updated with vt constants.Test expectations now use the vt package's
ModeStatusconstants (vt.ModeNA,vt.ModeOff,vt.ModeOn,vt.ModeOnLocked,vt.ModeOffLocked) andPrivateModeconstants (vt.PmAutoMargin,vt.PmShowCursor,vt.PmBlinkCursor), providing type-safe and readable test data.
641-642: LGTM - Test assertions use Status.Changeable().The test correctly verifies the
Changeable()method behavior, which replaces the previoususable()check. This aligns with the implementation changes ininput.goandtscreen.go.vt/mode.go (3)
22-40: Well-structured private mode constants.The constants are appropriately named and the values match standard VT private mode numbers. Good inline comment for PmAltScreen explaining the alternative values.
42-55: CSI sequence generation looks correct.The Enable (
h), Disable (l), and Query ($p) methods correctly format the CSI sequences for private modes.
73-76: Changeable() logic is correct.The method correctly identifies that only
ModeOnandModeOffstates are changeable, whileModeNA,ModeOnLocked, andModeOffLockedare not.mock/mock.go (10)
29-31: Import addition looks good.The vt package import is properly added alongside existing imports.
61-61: Type-safe private mode map.Good refactor to use
map[vt.PrivateMode]vt.ModeStatusinstead of raw integers. This provides better type safety and self-documenting code.
107-136: Movement methods correctly typed.The
moveUp,moveDown,moveForward, andmoveBackwardmethods now accept typed parameters (vt.Rowandvt.Col), which aligns with the semantic meaning of each movement direction.
139-144: Proper type conversion for cell indexing.The explicit
int()conversions for the index calculation are necessary since array indexing requires int type. The bounds checking logic correctly uses the typed comparisons.
176-204: CSI cursor movement handling properly refactored.The type conversions from parsed int parameters to
vt.Row/vt.Colare correctly applied for all cursor movement commands (CUU, CUD, CUF, CUB, CNL, CPL, CHA, CUP).
206-246: Erase operations use correct vt types.The ED (erase in display) and EL (erase in line) operations properly use
vt.Rowandvt.Colfor loop variables and therangeexpressions work correctly with the typed dimensions.
270-283: Private mode handling uses new vt API effectively.The set/reset private mode handlers now use
Changeable()to check if a mode can be modified, and the query handler usespm.Reply()to generate the response. This is cleaner and more maintainable than raw integer manipulation.
314-314: Index calculation correctly converts types.The cell index calculation properly converts all typed values to
intfor array indexing.
558-560: WindowSize returns proper int conversions.The conversion from
vt.Row/vt.Coltointfor thetcell.WindowSizestruct is correct.
568-583: Reset initialization uses vt types consistently.The Cells slice allocation correctly converts typed dimensions to int, and the PrivateModes map is initialized with proper vt constants and status values. Using
vt.ModeOffLockedfor AutoMargin ensures it cannot be changed, which matches typical terminal behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mock/mock.go (1)
225-228: Fix go directive format: remove patch version from go.mod.The code correctly uses Go 1.22+ range-over-integer syntax, which is supported. However, the go.mod file specifies
go 1.24.0, which violates the Go directive format. The go directive version must adhere to the format 1.x (e.g., go 1.19, go 1.23). Changego 1.24.0togo 1.24.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mock/mock.go(11 hunks)vt/coord.go(1 hunks)vt/mode.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
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/mock.go
🧬 Code graph analysis (1)
mock/mock.go (5)
vt/coord.go (2)
Row(22-22)Col(25-25)color.go (1)
Color(32-32)attr.go (1)
AttrMask(22-22)vt/mode.go (8)
PrivateMode(23-23)ModeStatus(63-63)ModeOn(67-67)ModeOff(68-68)PmAutoMargin(27-27)ModeOffLocked(70-70)PmFocusReports(34-34)PmResizeReports(39-39)resize.go (1)
WindowSize(46-51)
🔇 Additional comments (17)
vt/mode.go (5)
15-23: LGTM! Package and type foundation is well-structured.The package documentation clearly explains the scope (VT100, XTerm, Kitty, Windows Terminal), and the
PrivateModetype provides a clean abstraction for DEC Private Modes.
25-40: LGTM! Comprehensive set of VT private mode constants.The constants cover the essential terminal modes with clear documentation. The note about alternate screen buffer modes (1049 vs. 47/1047) is helpful context.
42-60: LGTM! Escape sequence generation methods are correct.The methods properly format CSI sequences according to the DEC Private Mode specification:
- Enable:
CSI ? Ps h- Disable:
CSI ? Ps l- Query:
CSI ? Ps $ p- Reply:
CSI ? Ps ; Pn $ y
62-71: Previous typo has been fixed.The past review flagged "hardwared ton" at line 69, which has been corrected to "hardwired on". The
ModeStatustype and constants are now correctly documented.
73-76: LGTM! Changeable() logic is correct.The method correctly identifies that only
ModeOnandModeOffstates can be toggled, while locked states (ModeOnLocked,ModeOffLocked) and unsupported modes (ModeNA) cannot be changed.vt/coord.go (3)
1-18: LGTM! Standard license header and package structure.The license header is consistent with the other files in the PR.
16-22: LGTM! Clear type separation for coordinate axes.The use of separate
RowandColtypes is a good practice to prevent coordinate axis confusion. The documentation clearly explains the zero-based indexing strategy and the distinction from VT's one-based escape sequences.
24-31: Previous typo has been fixed.The past review flagged "indicats" in the
Colcomment, which has been corrected to "indicates". The types andCoordstruct are well-documented and appropriate for representing terminal coordinates and window sizes.mock/mock.go (9)
30-30: LGTM! Import of vt package enables type-safe coordinate handling.
42-61: Previous comment swap issue has been fixed.The past review flagged that X and Y comments were reversed. Lines 49-50 now correctly document:
X vt.Col // cursor horizontal positionY vt.Row // cursor vertical positionThe refactoring to use
vt.Rowandvt.Coltypes provides better type safety and prevents coordinate axis confusion.
107-136: LGTM! Movement methods correctly refactored to use vt types.The
moveUp,moveDown,moveForward, andmoveBackwardmethods now acceptvt.Rowandvt.Colparameters respectively, providing type safety. Boundary checks correctly prevent cursor movement outside the valid range.
139-144: LGTM! Cell erasure handles vt types correctly.The
eraseCellmethod properly convertsvt.Colandvt.Rowtointfor array indexing at line 141, and the boundary checks prevent out-of-bounds access.
206-231: LGTM! Erase display (ED) and erase line (EL) logic correctly refactored.The CSI J and CSI K handlers properly use vt types for loop variables and boundary checks. The type conversions are appropriate for the operations being performed.
270-283: LGTM! Private mode handling correctly uses vt types and Status semantics.Lines 270-283 properly:
- Convert parameters to
vt.PrivateMode- Check
Status.Changeable()before allowing mode changes- Use
vt.ModeOnandvt.ModeOffconstants- Call
pm.Reply()for query responsesThis aligns well with the new vt mode API.
314-314: LGTM! Array index calculation correctly converts vt types to int.The calculation
ix := int(mt.X) + int(mt.Y)*int(mt.Cols)properly converts coordinate types to integers for array indexing.
558-560: LGTM! WindowSize conversion handles vt types correctly.The method properly converts
vt.Rowandvt.Coltointfor thetcell.WindowSizestructure.
563-584: LGTM! Reset() initialization properly uses vt types and constants.The initialization:
- Allocates
Cellsarray with correct size calculation using int conversions (line 568)- Initializes
PrivateModesmap withvt.PrivateModekeys andvt.ModeStatusvalues (lines 579-583)- Uses appropriate vt constants (
vt.PmAutoMargin,vt.PmFocusReports,vt.PmResizeReports)- Sets correct initial statuses (
vt.ModeOffLocked,vt.ModeOff)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
mock/mock.go (1)
201-204: Minor style inconsistency in boundary checks (optional improvement).Line 201 mixes comparison styles:
pos[0] >= 1checks the int directly, whilevt.Col(pos[1]) >= 1converts first. For consistency, consider checking both positions the same way.🔎 Suggested consistency improvement
- if pos := intParams(str, 2, 1); pos[0] >= 1 && vt.Row(pos[0]) <= mt.Rows && vt.Col(pos[1]) >= 1 && vt.Col(pos[1]) <= mt.Cols { + if pos := intParams(str, 2, 1); vt.Row(pos[0]) >= 1 && vt.Row(pos[0]) <= mt.Rows && vt.Col(pos[1]) >= 1 && vt.Col(pos[1]) <= mt.Cols {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mock/mock.go(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
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/mock.go
🧬 Code graph analysis (1)
mock/mock.go (5)
vt/coord.go (2)
Row(22-22)Col(25-25)color.go (1)
Color(32-32)attr.go (1)
AttrMask(22-22)vt/mode.go (8)
PrivateMode(23-23)ModeStatus(63-63)ModeOn(67-67)ModeOff(68-68)PmAutoMargin(27-27)ModeOffLocked(70-70)PmFocusReports(34-34)PmResizeReports(39-39)resize.go (1)
WindowSize(46-51)
🔇 Additional comments (12)
mock/mock.go (12)
30-30: LGTM! Import addition enables vt-based type usage.The vt package import is necessary for the coordinate and private mode types used throughout this refactoring.
44-51: LGTM! Struct fields updated with correct vt types and comments.The coordinate fields now use strongly-typed vt.Row and vt.Col, and the comments correctly identify X as horizontal (column) and Y as vertical (row). The past review concern about swapped comments has been resolved.
61-61: LGTM! Private mode map now uses vt-based types.The PrivateModes field now uses vt.PrivateMode keys and vt.ModeStatus values, providing stronger type safety for mode handling.
107-136: LGTM! Movement functions now use semantically appropriate vt types.The movement functions correctly use vt.Row for vertical movement (moveUp/moveDown) and vt.Col for horizontal movement (moveForward/moveBackward), improving type safety.
139-144: LGTM! Cell erasure uses vt types with correct indexing.The eraseCell function now uses vt.Col and vt.Row parameters, and line 141 correctly converts these to int for linear array indexing.
174-199: LGTM! Cursor movement commands now use correct default values.All cursor movement commands (CUU, CUD, CUF, CUB, CNL, CPL, CHA) now use
intParams(str, 1, 1)with default value 1, correctly implementing VT100 behavior. The past review concern has been resolved.
206-246: LGTM! Erase operations correctly use vt types.The erase display (ED) and erase line (EL) operations properly use vt.Row and vt.Col types in range loops and explicit initialization. Go's range over int-based types produces the correct typed values for eraseCell calls.
270-282: LGTM! Private mode handling uses vt API correctly.The private mode handling correctly uses vt.PrivateMode keys, checks ModeStatus.Changeable() before modifications, and uses PrivateMode.Reply() for query responses. This properly integrates with the vt package's mode management API.
314-314: LGTM! Cell indexing correctly converts vt types to int.The linear index calculation properly converts vt.Col and vt.Row to int for array access using the standard formula.
559-559: LGTM! WindowSize correctly converts vt dimensions to int.The WindowSize method properly converts vt.Row and vt.Col to int values for the tcell.WindowSize struct.
568-568: LGTM! Cell slice allocation uses correct size calculation.The Cells slice allocation correctly converts vt.Cols and vt.Rows to int for size computation.
579-583: LGTM! Private modes initialized with vt constants and appropriate statuses.The PrivateModes map initialization uses vt package constants (PmAutoMargin, PmFocusReports, PmResizeReports) with appropriate ModeStatus values, aligning with the vt-based mode management system.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mock/mock.go(11 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
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/mock.go
🧬 Code graph analysis (1)
mock/mock.go (3)
vt/coord.go (2)
Row(22-22)Col(25-25)vt/mode.go (8)
PrivateMode(23-23)ModeStatus(63-63)ModeOn(67-67)ModeOff(68-68)PmAutoMargin(27-27)ModeOffLocked(70-70)PmFocusReports(34-34)PmResizeReports(39-39)resize.go (1)
WindowSize(46-51)
🔇 Additional comments (8)
mock/mock.go (8)
30-30: LGTM!The vt package import is correctly added to support the coordinate and mode type refactoring throughout this file.
42-61: LGTM!The MockTty fields have been correctly updated to use vt-based types:
- Coordinate types (Rows, Cols, X, Y) now use vt.Row and vt.Col
- PrivateModes now uses vt.PrivateMode and vt.ModeStatus
- Comments for X and Y correctly describe horizontal/vertical positions
107-136: LGTM!Movement functions correctly updated to use vt.Row and vt.Col types. The boundary clamping logic is preserved and works correctly with the new types.
139-144: LGTM!The eraseCell function signature correctly updated to use vt.Col and vt.Row, with proper int conversions for array indexing on line 141.
208-248: LGTM!Erase display and line logic correctly updated to use vt-based coordinate types. Loop variables are properly typed, and range statements work correctly with vt.Row and vt.Col types (Go 1.22+ feature).
272-285: LGTM!Private mode handling correctly refactored to use vt.PrivateMode and vt.ModeStatus types, with appropriate method calls to Changeable() and Reply().
560-562: LGTM!WindowSize correctly converts vt-based dimensions to int values for tcell.WindowSize structure.
565-586: LGTM!Reset function correctly updated:
- Cell allocation uses int conversions from vt types (line 570)
- PrivateModes initialization uses vt.PrivateMode keys and vt.ModeStatus values (lines 581-585)
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.