feat(emulator): Add Titler, and tests for it. #940
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughPre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
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: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
demos/hello/hello.gomock/backend.gomock/term.gomock/term_test.govt/backend.govt/emulate.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:
vt/backend.go
🧬 Code graph analysis (4)
vt/backend.go (2)
tscreen.go (2)
t(1323-1330)t(396-435)sim_test.go (1)
TestTitle(138-146)
demos/hello/hello.go (1)
simulation.go (1)
s(484-486)
mock/term_test.go (2)
mock/term.go (1)
NewMockTerm(133-139)mock/backend.go (1)
MockOptSize(184-184)
vt/emulate.go (1)
vt/backend.go (1)
Titler(109-112)
🔇 Additional comments (10)
demos/hello/hello.go (1)
28-28: LGTM! Nice demonstration of the title-setting capability.The placement is appropriate—setting the title during display initialization demonstrates the new API effectively.
mock/term.go (2)
104-107: LGTM! Clean delegation pattern.The implementation correctly delegates to the underlying MockBackend, consistent with other methods in this struct.
128-129: LGTM! Interface extension is minimal and appropriate.Adding GetTitle() to the MockTerm interface enables test validation of title-setting behavior.
mock/term_test.go (1)
565-586: LGTM! Comprehensive test coverage for OSC title sequences.The test validates all three termination methods:
- Standard ST termination (ESC backslash)
- Legacy BEL termination
- 8-bit sequence termination
This ensures robust handling of different terminal emulator behaviors.
vt/emulate.go (3)
278-291: LGTM! Correct OSC termination handling.Supporting both 0x9c (ST) and 0x07 (BEL) for OSC termination aligns with terminal standards. BEL termination is legacy but widely used in practice.
298-310: LGTM! Consistent string termination handling.The change mirrors the OSC handling, ensuring both 0x9c and 0x07 terminate string sequences consistently.
632-649: LGTM! Solid OSC processing implementation.The implementation correctly:
- Parses the OSC payload format (number;string)
- Handles OSC 2 for window title setting
- Gracefully ignores invalid inputs
- Only calls SetWindowTitle when the backend supports Titler
The TODO regarding UTF-8 validation is reasonable as a future enhancement. For now, invalid UTF-8 will simply pass through to the backend, which is acceptable.
mock/backend.go (3)
41-45: LGTM! Interface additions support title testing.The additions to MockBackend appropriately expose Bells() (which was already implemented) and the new GetTitle() for test validation.
50-50: LGTM! Comment accurately reflects new interface.The comment correctly documents that mockBackend now implements vt.Titler.
63-63: LGTM! Straightforward field addition.The title field appropriately stores the window title state for testing.
| // Titler adds support for setting the window title. (Typically this is OSC2.) | ||
| // Note that for security reasons we only support setting this. | ||
| // We don't bothere with icon titles, since few terminal emulators support it, and it | ||
| // would be hard for us to do this in any portable fashion. | ||
| type Titler interface { | ||
| // SetWindowTitle only changes the window title. | ||
| SetWindowTitle(string) | ||
| } |
There was a problem hiding this comment.
Fix typo in comment.
Line 107 has a typo: "bothere" should be "bother".
🔎 Proposed fix
// Titler adds support for setting the window title. (Typically this is OSC2.)
// Note that for security reasons we only support setting this.
-// We don't bothere with icon titles, since few terminal emulators support it, and it
+// We don't bother with icon titles, since few terminal emulators support it, and it
// would be hard for us to do this in any portable fashion.
type Titler interface {
// SetWindowTitle only changes the window title.
SetWindowTitle(string)
}📝 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.
| // Titler adds support for setting the window title. (Typically this is OSC2.) | |
| // Note that for security reasons we only support setting this. | |
| // We don't bothere with icon titles, since few terminal emulators support it, and it | |
| // would be hard for us to do this in any portable fashion. | |
| type Titler interface { | |
| // SetWindowTitle only changes the window title. | |
| SetWindowTitle(string) | |
| } | |
| // Titler adds support for setting the window title. (Typically this is OSC2.) | |
| // Note that for security reasons we only support setting this. | |
| // We don't bother with icon titles, since few terminal emulators support it, and it | |
| // would be hard for us to do this in any portable fashion. | |
| type Titler interface { | |
| // SetWindowTitle only changes the window title. | |
| SetWindowTitle(string) | |
| } |
🤖 Prompt for AI Agents
In vt/backend.go around lines 105 to 112, there's a typo in the comment on line
107: "bothere" should be "bother"; update the comment text to correct the
spelling to "bother" so the sentence reads "...We don't bother with icon
titles..." while leaving surrounding wording intact.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #940 +/- ##
==========================================
+ Coverage 58.56% 58.99% +0.43%
==========================================
Files 37 37
Lines 3239 3256 +17
==========================================
+ Hits 1897 1921 +24
+ Misses 1185 1176 -9
- Partials 157 159 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.