SGR 2 (Faint) support#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd modern Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Previously, each rune was treated as occupying 1 column, causing incorrect cursor positioning for wide characters like CJK text. Changes: - Add go-runewidth dependency to calculate display width - Add AttrWide and AttrWideDummy flags for wide char tracking - Move cursor by actual character width (2 for wide chars) - Handle wide char wrapping at line end - Clear orphaned cells when overwriting partial wide chars
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
state.go (1)
292-300: Dimming by 50% may be too aggressive for faint text.The
dimColorfunction halves each RGB component to create a faint appearance. While this is simple and effective, dimming by 50% may make text too hard to read in some color schemes, especially for already-dark colors.Consider:
- Using a less aggressive dimming factor (e.g., 33% or 25% reduction) for better readability.
- Making the dimming factor configurable.
- Testing with various color combinations to ensure faint text remains legible.
That said, 50% dimming matches the reference image mentioned in the PR description, so this may be intentional.
Please verify that the 50% dimming matches the intended visual appearance from the reference image and provides acceptable readability across different color schemes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go.modparse.gostate.go
🧰 Additional context used
🧬 Code graph analysis (2)
state.go (1)
color.go (3)
Color(35-35)DefaultFG(27-27)DefaultBG(28-28)
parse.go (1)
state.go (6)
State(86-107)AttrGfx(17-17)ModeWrap(37-37)AttrWrap(20-20)AttrWideDummy(23-23)AttrWide(22-22)
🔇 Additional comments (9)
parse.go (4)
28-31: Treating zero-width as 1 may not handle combining characters correctly.Zero-width characters (e.g., combining marks) are treated as width 1, which causes them to advance the cursor. In proper Unicode rendering, these should combine with the previous character without advancing the cursor.
Is combining character support required for this terminal emulator? If so, this logic should be enhanced to handle width==0 properly by combining with the previous cell rather than treating it as width 1.
34-43: Wide character silently dropped when wrapping is disabled.When a wide character doesn't fit and
ModeWrapis not enabled, the function returns early (line 41), silently dropping the character without writing anything.Please confirm this is the intended behavior for the terminal emulator. Some terminals might instead truncate the wide character or write a replacement character.
45-86: Wide character handling logic is comprehensive and well-bounded.The implementation correctly:
- Clears overlapped wide characters in all scenarios (writing over dummy, main, or placing dummy over another wide char's main).
- Performs proper bounds checking before all array accesses.
- Sets appropriate flags (
AttrWide,AttrWideDummy) for the main and dummy cells.- Moves the cursor by the actual character width.
The complexity is necessary to maintain correct rendering state when wide characters are overwritten or wrap across lines.
3-3: No action needed — the library version is valid and secure.
github.com/mattn/go-runewidth v0.0.19is the latest available version with no known vulnerabilities or security issues.go.mod (1)
5-5: Dependency addition aligns with wide-character support.The addition of
github.com/mattn/go-runewidth v0.0.19is necessary for the wide-character handling implemented inparse.go. Version verification was requested in the parse.go review.state.go (4)
14-24: Exported attribute constants enable external API usage.The promotion of attribute constants to exported names (e.g.,
AttrReverse,AttrBold) and the addition of new constants (AttrFaint,AttrWide,AttrWideDummy) align well with the PR objectives:
AttrFaintimplements SGR 2 (faint/decreased intensity) support.AttrWideandAttrWideDummyenable correct wide-character (CJK) rendering.The comments on lines 22-23 helpfully document the purpose of the wide-character flags.
162-166: Unsynchronized access viaRawLines()could lead to data races.The
RawLines()method exposes the internallinesslice without acquiring the mutex, breaking the thread-safety guarantees typically provided by theStatetype. While the comment documents this, it places a significant burden on callers to ensure proper synchronization.Please confirm:
- Is this API necessary, or could callers use the existing
Cell(x, y)method with proper locking?- Where will this be used, and have the call sites been audited to ensure they properly synchronize access?
- Consider making this a private method (e.g.,
rawLines()) if it's only needed internally.Based on learnings, unsynchronized APIs in concurrent code require careful justification and documentation of the exact use cases.
280-285: Faint and bold attributes are mutually exclusive.The implementation ensures that bold (line 280) and faint (line 283) are mutually exclusive:
- Bold brightening (FG + 8) is only applied if NOT faint.
- Faint dimming is only applied if NOT bold.
This is a reasonable design choice, as applying both simultaneously would be ambiguous. However, this behavior should be documented, as some terminal emulators may handle the combination differently.
Please confirm this mutual exclusion matches the intended terminal behavior and consider documenting it in code comments or external documentation.
636-660: SGR attribute handling correctly implements faint support.The implementation properly handles SGR (Select Graphic Rendition) sequences:
- SGR 0 (line 636): Resets all attributes including the new
AttrFaint.- SGR 2 (lines 641-642): Sets
AttrFaintas per the PR objective.- SGR 21/22 (line 652): Clears both
AttrBoldandAttrFaint, correctly implementing "normal intensity."All changes use the exported attribute constants consistently.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
csi.go (1)
187-196: Minor redundancy in condition, but acceptable for clarity.The condition
c.priv || c.prefix != 0is technically redundant sincec.privcan only be true whenc.prefix == '?'(which is non-zero). Simplifying to justc.prefix != 0would suffice:-if c.priv || c.prefix != 0 { +if c.prefix != 0 {However, keeping both checks is reasonable for clarity and defensive coding, especially if
privsemantics might evolve independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@csi.go` around lines 187 - 196, The condition in the CSI handler for cases 's' and 'u' redundantly checks both c.priv and c.prefix != 0; simplify by removing c.priv and only checking c.prefix != 0 before calling t.saveCursor() and t.restoreCursor() (references: the case 's' and case 'u' branches, c.priv, c.prefix, t.saveCursor, t.restoreCursor) so the guard becomes solely "if c.prefix != 0 { goto unknown }".csi_test.go (1)
46-55: Test covers the positive case; consider adding negative cases.The test correctly verifies that
ESC[cproduces the expected DA response. For completeness, consider adding a test case verifying that prefixed variants likeESC[>cdo not produce a response:func TestSecondaryDANoResponse(t *testing.T) { var reply bytes.Buffer term := New(WithWriter(&reply)) if _, err := term.Write([]byte("\033[>c")); err != nil { t.Fatal(err) } if reply.Len() != 0 { t.Fatalf("expected no response for secondary DA, got %q", reply.String()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@csi_test.go` around lines 46 - 55, Add a negative test that verifies prefixed DA requests don't elicit a response: create a TestSecondaryDANoResponse similar to TestXtermStylePrimaryDAResponse, instantiate the terminal with New(WithWriter(&reply)), write the sequence "\033[>c" and assert reply.Len() == 0 (or reply.String() == "") so that secondary/extended DA (prefixed with '>') does not produce any output; reference the existing TestXtermStylePrimaryDAResponse, New, and WithWriter helpers to implement the test.state.go (1)
167-171: Consider the risk of exposing raw lines without synchronization.The
RawLines()method exposes internal state without locking, which could lead to data races if the caller doesn't properly synchronize. While the comment warns about this, an exported method like this may be misused.Consider whether this method needs to be exported, or if callers should use the existing
Cell()method withinLock()/Unlock()pairs instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@state.go` around lines 167 - 171, The RawLines method exposes the internal slice t.lines without synchronization which can cause data races; either make it unexported (rename RawLines to rawLines) and restrict callers to use State.Cell() within Lock()/Unlock() pairs, or implement safe access by acquiring a read lock inside RawLines (e.g., t.mu.RLock(); defer t.mu.RUnlock()) and returning a copy of the slice (append([]line(nil), t.lines...)) so callers cannot mutate internal state; update callers accordingly to use the safe API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@csi_test.go`:
- Around line 46-55: Add a negative test that verifies prefixed DA requests
don't elicit a response: create a TestSecondaryDANoResponse similar to
TestXtermStylePrimaryDAResponse, instantiate the terminal with
New(WithWriter(&reply)), write the sequence "\033[>c" and assert reply.Len() ==
0 (or reply.String() == "") so that secondary/extended DA (prefixed with '>')
does not produce any output; reference the existing
TestXtermStylePrimaryDAResponse, New, and WithWriter helpers to implement the
test.
In `@csi.go`:
- Around line 187-196: The condition in the CSI handler for cases 's' and 'u'
redundantly checks both c.priv and c.prefix != 0; simplify by removing c.priv
and only checking c.prefix != 0 before calling t.saveCursor() and
t.restoreCursor() (references: the case 's' and case 'u' branches, c.priv,
c.prefix, t.saveCursor, t.restoreCursor) so the guard becomes solely "if
c.prefix != 0 { goto unknown }".
In `@state.go`:
- Around line 167-171: The RawLines method exposes the internal slice t.lines
without synchronization which can cause data races; either make it unexported
(rename RawLines to rawLines) and restrict callers to use State.Cell() within
Lock()/Unlock() pairs, or implement safe access by acquiring a read lock inside
RawLines (e.g., t.mu.RLock(); defer t.mu.RUnlock()) and returning a copy of the
slice (append([]line(nil), t.lines...)) so callers cannot mutate internal state;
update callers accordingly to use the safe API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cc6d30b-365d-4d51-a598-156c2c031437
📒 Files selected for processing (4)
alt_screen_test.gocsi.gocsi_test.gostate.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vt.go (1)
7-7: Consider updating deprecated ioutil usage.
ioutil.Discardhas been deprecated since Go 1.16 in favor ofio.Discard. While this is pre-existing code, consider updating it as part of this change.♻️ Update deprecated import
import ( "bufio" "fmt" "io" - "io/ioutil" )And in
New():info := TerminalInfo{ - w: ioutil.Discard, + w: io.Discard,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vt.go` at line 7, Replace deprecated ioutil usage: update the import of "io/ioutil" to "io" and change any uses of ioutil.Discard to io.Discard (e.g., inside the New() function where ioutil.Discard is referenced). Ensure there are no remaining references to ioutil.Discard and run `go vet`/`go build` to verify imports and usage are correct.str.go (1)
185-196: Redundant range check in validColorSlot.The final
between(j, 0, 1<<24-1)check (line 195) already includes thebetween(j, 0, 255)check (line 186), making the first check redundant. Consider simplifying:♻️ Suggested simplification
func (t *State) validColorSlot(j int) bool { - if between(j, 0, 255) { - return true - } - switch Color(j) { case DefaultFG, DefaultBG, DefaultCursor: return true } return between(j, 0, 1<<24-1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@str.go` around lines 185 - 196, The initial between(j, 0, 255) check in function validColorSlot is redundant because the later between(j, 0, 1<<24-1) already covers that range; remove the first between call and simplify validColorSlot to only check the special Color cases (DefaultFG, DefaultBG, DefaultCursor) and then the broader between(j, 0, 1<<24-1) range check so the logic remains correct and concise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@str_test.go`:
- Around line 9-12: Tests in str_test.go call the helper writeAll but that
helper is defined in alt_screen_test.go, so add or centralize the helper: either
define a local writeAll(t *testing.T, term *Term, data ...string) function in
str_test.go matching the signature used in lines ~181-219, or move writeAll into
a shared test helper file (e.g., test_helpers_test.go) and import/keep it
package-private so both str_test.go and alt_screen_test.go can call it; ensure
the implementation uses the same behavior expected by the tests (writing the
provided strings to the terminal test harness) and reference the writeAll symbol
in your change so callers compile.
---
Nitpick comments:
In `@str.go`:
- Around line 185-196: The initial between(j, 0, 255) check in function
validColorSlot is redundant because the later between(j, 0, 1<<24-1) already
covers that range; remove the first between call and simplify validColorSlot to
only check the special Color cases (DefaultFG, DefaultBG, DefaultCursor) and
then the broader between(j, 0, 1<<24-1) range check so the logic remains correct
and concise.
In `@vt.go`:
- Line 7: Replace deprecated ioutil usage: update the import of "io/ioutil" to
"io" and change any uses of ioutil.Discard to io.Discard (e.g., inside the New()
function where ioutil.Discard is referenced). Ensure there are no remaining
references to ioutil.Discard and run `go vet`/`go build` to verify imports and
usage are correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 071fdbc6-c86b-47b4-99b4-675a13f5db52
📒 Files selected for processing (11)
README.mdcsi.gocsi_test.goparse.gostate.gostr.gostr_test.govt.govt_other.govt_posix.govt_test.go
✅ Files skipped from review due to trivial changes (2)
- vt_other.go
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- vt_posix.go
- parse.go
- csi_test.go
| func oscColorReply(num int, c Color, term string) string { | ||
| r, g, b := rgb(int(c)) | ||
| return fmt.Sprintf("\033]%d;rgb:%02x%02x/%02x%02x/%02x%02x%s", num, r, r, g, g, b, b, term) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the writeAll helper function definition
ast-grep --pattern 'func writeAll($$$) {
$$$
}'Repository: tuzig/vt10x
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Check if str_test.go exists and examine its structure
fd -t f "str_test.go" && wc -l str_test.goRepository: tuzig/vt10x
Length of output: 82
🏁 Script executed:
#!/bin/bash
# Examine lines 181-219 where writeAll is likely being used
sed -n '181,219p' str_test.goRepository: tuzig/vt10x
Length of output: 1195
🏁 Script executed:
#!/bin/bash
# Search for writeAll definition in str_test.go
rg "func writeAll" str_test.goRepository: tuzig/vt10x
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Check if there's a common test utilities file
fd -t f "test" | grep -E "(util|helper|common)" | head -20Repository: tuzig/vt10x
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Search for all definitions of writeAll in the repository
rg "func writeAll" --type goRepository: tuzig/vt10x
Length of output: 130
🏁 Script executed:
#!/bin/bash
# Check what files import what in this repo to understand test structure
fd -t f "_test.go" | head -10Repository: tuzig/vt10x
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Check the full content of str_test.go to see if writeAll is defined
cat -n str_test.go | head -50Repository: tuzig/vt10x
Length of output: 1236
🏁 Script executed:
#!/bin/bash
# Check if writeAll is referenced/imported in str_test.go
rg "writeAll" str_test.go -B2 -A2Repository: tuzig/vt10x
Length of output: 625
Add writeAll helper function to str_test.go or refactor test utilities.
The tests at lines 181-219 use writeAll(t, term, ...) which is defined in alt_screen_test.go, not in this file. Either define writeAll locally in str_test.go or move shared test helpers to a dedicated test utilities file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@str_test.go` around lines 9 - 12, Tests in str_test.go call the helper
writeAll but that helper is defined in alt_screen_test.go, so add or centralize
the helper: either define a local writeAll(t *testing.T, term *Term, data
...string) function in str_test.go matching the signature used in lines
~181-219, or move writeAll into a shared test helper file (e.g.,
test_helpers_test.go) and import/keep it package-private so both str_test.go and
alt_screen_test.go can call it; ensure the implementation uses the same behavior
expected by the tests (writing the provided strings to the terminal test
harness) and reference the writeAll symbol in your change so callers compile.
1 participant
https://xtermjs.org/docs/api/vtfeatures/> SGR 2: Faint, decreased intensity.
Made it for gray text from codex:
Summary by CodeRabbit
New Features
Improvements
Tests
API Changes