Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds ECMA-48 SGR handling to the terminal emulator and tests: expands processSgr to handle additional attribute codes (bold/dim interplay, underline variants, italic, blink, reverse, strikethrough, overline) and basic 8-color FG/BG setting and resets; adds tests verifying SGR attributes and 8-color behavior. Changes
Sequence Diagram(s)(omitted — changes are internal SGR handling and tests without a multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 #935 +/- ##
==========================================
+ Coverage 33.37% 35.52% +2.14%
==========================================
Files 34 34
Lines 3053 3091 +38
==========================================
+ Hits 1019 1098 +79
+ Misses 1976 1935 -41
Partials 58 58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
mock/term_test.govt/emulate.go
🧰 Additional context used
🧬 Code graph analysis (2)
mock/term_test.go (3)
vt/coord.go (1)
Coord(28-31)vt/attr.go (14)
Attr(18-18)Bold(22-22)Dim(25-25)Plain(21-21)Italic(26-26)Underline(28-28)DoubleUnderline(33-33)CurlyUnderline(34-34)DottedUnderline(35-35)DashedUnderline(36-36)StrikeThrough(27-27)Reverse(24-24)Overline(29-29)Blink(23-23)color/color.go (4)
Teal(67-67)Green(63-63)Silver(68-68)Black(61-61)
vt/emulate.go (3)
vt/attr.go (9)
Dim(25-25)Bold(22-22)UnderlineMask(37-37)DoubleUnderline(33-33)Italic(26-26)Blink(23-23)Reverse(24-24)StrikeThrough(27-27)Overline(29-29)vt/backend.go (1)
Colorer(59-69)color/color.go (2)
Black(61-61)Reset(860-860)
🔇 Additional comments (7)
vt/emulate.go (4)
383-388: LGTM on Bold/Dim mutual exclusivity.Clearing the conflicting attribute before setting the new one correctly implements the ECMA-48 behavior where Bold (1) and Dim (2) are mutually exclusive.
414-428: LGTM on attribute clearing codes.The ECMA-48 attribute off codes (21–29) are correctly implemented:
- 21: Doubly underlined
- 22: Normal intensity (clears Bold/Dim)
- 23–29: Clear italic, underline, blink, reverse, strikethrough respectively
380-382: Verify: SGR 0 does not reset colors.With the new color handling (cases 30–49), SGR 0 currently only resets
em.attrbut does not reset foreground/background colors via theColorerinterface. Per ECMA-48, SGR 0 should reset all rendition attributes including colors. Consider whether this is intentional or if colors should also be reset.
448-451: LGTM on Overline handling.Cases 53 (enable) and 55 (disable) for Overline attribute are correctly implemented per ECMA-48.
mock/term_test.go (3)
22-22: LGTM!Import correctly added for the new color-related tests.
412-483: Well-structured attribute tests.Good coverage of:
- Bold/Dim mutual exclusivity
- Individual attribute clearing (22, 23, 24, 25, 27, 29)
- Underline variants (simple, double, curly, dotted, dashed)
- Invalid underline sub-parameter (4:9) falling back to plain underline
- Compound attributes (strikethrough + reverse + overline)
485-512: Good color test coverage.The test correctly verifies:
- Setting foreground (36 = Teal) and background (42 = Green) colors
- Reset foreground (39) restores to default (Silver)
- Reset background (49) restores to default (Black)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
vt/emulate.go (1)
443-446: Comment typo still present.The inline comment on line 443 still reads "simple foreground colors" but should read "simple background colors" since cases 40-47 set background colors.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mock/term_test.govt/emulate.go
🧰 Additional context used
🧬 Code graph analysis (2)
vt/emulate.go (3)
vt/backend.go (1)
Colorer(59-69)color/color.go (2)
Reset(860-860)Black(61-61)vt/attr.go (9)
Dim(25-25)Bold(22-22)UnderlineMask(37-37)DoubleUnderline(33-33)Italic(26-26)Blink(23-23)Reverse(24-24)StrikeThrough(27-27)Overline(29-29)
mock/term_test.go (5)
mock/term.go (1)
NewMockTerm(111-117)mock/backend.go (1)
MockOptSize(168-168)vt/coord.go (1)
Coord(28-31)vt/attr.go (13)
Attr(18-18)Bold(22-22)Dim(25-25)Plain(21-21)Underline(28-28)DoubleUnderline(33-33)CurlyUnderline(34-34)DottedUnderline(35-35)DashedUnderline(36-36)StrikeThrough(27-27)Reverse(24-24)Overline(29-29)Blink(23-23)color/color.go (4)
Teal(67-67)Green(63-63)Silver(68-68)Black(61-61)
🔇 Additional comments (7)
vt/emulate.go (5)
383-386: LGTM! Proper SGR 0 reset behavior.The addition of color reset when processing SGR 0 correctly implements ECMA-48 behavior, which requires resetting both attributes and colors to defaults.
388-392: LGTM! Correct Bold/Dim mutual exclusivity.The implementation properly ensures Bold and Dim are mutually exclusive, aligning with ECMA-48 terminal behavior where setting one clears the other.
418-432: LGTM! Proper ECMA-48 SGR attribute clearing.The implementation of SGR codes 21-29 correctly handles attribute clearing per ECMA-48 specifications. The use of bitwise clear operations is appropriate.
434-442: LGTM! Correct ANSI 8-color foreground implementation.The implementation properly handles SGR codes 30-37 for setting 8-color foreground and 39 for resetting. The color offset calculation is correct.
452-455: LGTM! Proper overline attribute handling.The implementation correctly handles SGR codes 53 (enable overline) and 55 (disable overline) using appropriate bitwise operations.
mock/term_test.go (2)
22-22: LGTM! Necessary import for color testing.The color package import is required for the new color-related test assertions in TestSgrColor8.
493-537: Good test coverage for 8-color SGR codes.The test properly verifies foreground/background color setting (SGR 30-37, 40-47) and reset behavior (SGR 39, 49, 0). The expected default colors (Silver for foreground, Black for background) correctly match the actual defaults configured in the mock terminal backend (
mock/backend.go, lines 183-184).
| // TestSgrAttr tests a variety of combinations of Sgr settings. | ||
| func TestSgrAttr(t *testing.T) { | ||
| trm := NewMockTerm(MockOptSize{X: 80, Y: 24}) | ||
| defer mustClose(t, trm) | ||
| mustStart(t, trm) | ||
|
|
||
| writeF(t, trm, "\x1b[H") | ||
| writeF(t, trm, "\x1b[1mA") // bold | ||
| if attr := trm.GetCell(vt.Coord{X: 0, Y: 0}).Attr; attr != vt.Bold { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[2mB") // dim | ||
| if attr := trm.GetCell(vt.Coord{X: 1, Y: 0}).Attr; attr != vt.Dim { // dim is exclusive of bold | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[22mC") // clear both | ||
| if attr := trm.GetCell(vt.Coord{X: 2, Y: 0}).Attr; attr != vt.Plain { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[3;2mD") // italic, dim | ||
| if attr := trm.GetCell(vt.Coord{X: 3, Y: 0}).Attr; attr != vt.Italic|vt.Dim { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[22mE") // dim, should leave italic | ||
| if attr := trm.GetCell(vt.Coord{X: 4, Y: 0}).Attr; attr != vt.Italic { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[23mF") // clear italic | ||
| if attr := trm.GetCell(vt.Coord{X: 5, Y: 0}).Attr; attr != vt.Plain { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[3;4mG") // simple underline | ||
| if attr := trm.GetCell(vt.Coord{X: 6, Y: 0}).Attr; attr != vt.Italic|vt.Underline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[21mH") // double underline (ECMA) | ||
| if attr := trm.GetCell(vt.Coord{X: 7, Y: 0}).Attr; attr != vt.Italic|vt.DoubleUnderline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4mI") // simple underline | ||
| if attr := trm.GetCell(vt.Coord{X: 8, Y: 0}).Attr; attr != vt.Italic|vt.Underline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4:2mJ") // simple underline | ||
| if attr := trm.GetCell(vt.Coord{X: 9, Y: 0}).Attr; attr != vt.Italic|vt.DoubleUnderline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4:3mI") // curly underline | ||
| if attr := trm.GetCell(vt.Coord{X: 10, Y: 0}).Attr; attr != vt.Italic|vt.CurlyUnderline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4:4mJ") // dotted underline | ||
| if attr := trm.GetCell(vt.Coord{X: 11, Y: 0}).Attr; attr != vt.Italic|vt.DottedUnderline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4:5mK") // dashed underline | ||
| if attr := trm.GetCell(vt.Coord{X: 12, Y: 0}).Attr; attr != vt.Italic|vt.DashedUnderline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4:9mL") // junk treats as plain | ||
| if attr := trm.GetCell(vt.Coord{X: 13, Y: 0}).Attr; attr != vt.Italic|vt.Underline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[4:5;24mM") // clustering, clear it | ||
| if attr := trm.GetCell(vt.Coord{X: 14, Y: 0}).Attr; attr != vt.Italic { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[0;9;7;53mN") // clear, striketrhough, reverse, overlined | ||
| if attr := trm.GetCell(vt.Coord{X: 15, Y: 0}).Attr; attr != vt.StrikeThrough|vt.Reverse|vt.Overline { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[5;27;29;55mO") | ||
| if attr := trm.GetCell(vt.Coord{X: 16, Y: 0}).Attr; attr != vt.Blink { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| writeF(t, trm, "\x1b[25mP") | ||
| if attr := trm.GetCell(vt.Coord{X: 17, Y: 0}).Attr; attr != vt.Plain { | ||
| t.Errorf("wrong attr: %x", attr) | ||
| } | ||
| } |
There was a problem hiding this comment.
Excellent test coverage for SGR attributes.
The test comprehensively verifies the new SGR attribute handling, including mutual exclusivity of bold/dim, various underline styles, and proper clearing of individual attributes.
Minor typo on line 479: "striketrhough" should be "strikethrough" in the comment.
🤖 Prompt for AI Agents
In mock/term_test.go around lines 412 to 491, there is a minor typo in the
comment on line 479 where "striketrhough" should read "strikethrough"; update
that single-word comment to the correct spelling without changing surrounding
code or test logic.
1 participant
…tests.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.