Add support for colon-separated values #657
Conversation
Separated SGR within CSI and supported colon-separated SGR. Also supported redundant SGR specifications. The color specification of the SGR is invalidated outside the range, and the invalid string becomes an error.
|
Hey @noborus Can you explicit what the acronyms state for? SGR |
oviewer/convert_es.go
Outdated
| @@ -24,6 +24,18 @@ const ( | |||
| oscURL | |||
| ) | |||
|
|
|||
| // Finalbyte is a character outside the escape sequence. | |||
There was a problem hiding this comment.
| // Finalbyte is a character outside the escape sequence. | |
| // FinalByte is a character outside the escape sequence. |
I could suggest a linter for that
oviewer/convert_es.go
Outdated
| // ColorsRGB is the index of the RGB color. 24-bit colors. r:0-255 g:0-255 b:0-255. | ||
| ColorsRGB = 2 | ||
| ) | ||
|
|
||
| // escape sequence states. |
There was a problem hiding this comment.
| // escape sequence states. | |
| // escapeSequence states. |
oviewer/convert_es.go
Outdated
| return tcell.StyleDefault.Normal(), nil | ||
| // sgrStyle returns tcell.Style from the SGR control sequence. | ||
| func sgrStyle(style tcell.Style, paramStr string) tcell.Style { | ||
| if paramStr == "0" || paramStr == "" || paramStr == ";" { |
There was a problem hiding this comment.
I would suggest using a switch with one case listing the 3 values
It will improve readability
| case 0: | ||
|
|
||
| switch sgr.code { | ||
| case 0: // Reset. |
There was a problem hiding this comment.
Use constants for the numbers, this way code is clearer and you improve readability, because then you don't need comment
| case 0: // Reset. | |
| case sgrCodeReset: |
There was a problem hiding this comment.
I disagree with this.
Writing escape sequences with (uncommon) names makes them difficult to read.
| i, color := csColor(fields[index:]) | ||
| if i == 0 { | ||
| return s, nil | ||
| case 30, 31, 32, 33, 34, 35, 36, 37: // Foreground color |
There was a problem hiding this comment.
See here
colorForegroundBlack, colorForegroundRed ...
oviewer/convert_es.go
Outdated
| func toESCode(str string) (int, error) { | ||
| num, err := strconv.Atoi(str) | ||
| // toSGRCode converts the SGR parameter to a code. | ||
| // Supports both separators (:) and (;). |
There was a problem hiding this comment.
You said it supports both, but I only see a split on : 🤔
There was a problem hiding this comment.
This was not a good comment.
If there was no colon(:) separated string, the paramlist (which was semicolon(;;) separated) parameter would be used.
oviewer/convert_es.go
Outdated
| func parseRGBColor(params []string) (string, error) { | ||
| if params[0] == "" || params[1] == "" || params[2] == "" { |
There was a problem hiding this comment.
Either you use
| func parseRGBColor(params []string) (string, error) { | |
| if params[0] == "" || params[1] == "" || params[2] == "" { | |
| func parseRGBColor(params [3]string) (string, error) { | |
| if params[0] == "" || params[1] == "" || params[2] == "" { |
Either you return an error if len(params) != 3)
Otherwise, your code will panic
There was a problem hiding this comment.
It is checked before, so change the passing by slice.
| } | ||
| } | ||
|
|
||
| func Test_parseSGR2(t *testing.T) { |
There was a problem hiding this comment.
Why 2 ?
You could simply add two t.Run block in Test_parseSGR
There was a problem hiding this comment.
I want to separate them because there are too many, but it is hard to think of names...
| @@ -426,8 +426,8 @@ var ( | |||
| ErrNotAlignMode = errors.New("not an align mode") | |||
| // ErrNoColumnSelected indicates that no column is selected. | |||
| ErrNoColumnSelected = errors.New("no column selected") | |||
| // ErrInvalidCSI indicates that the CSI is invalid. | |||
| ErrInvalidCSI = errors.New("invalid CSI") | |||
There was a problem hiding this comment.
Are you sure no one use you as a lib?
You should set it to ErrInvalidSGR (or keep it as is)
And add a // Deprecated: flag in the godoc
There was a problem hiding this comment.
I added this 2 days ago, so I'm pretty sure no one is using this.
Thank you for your comment. |
Separated SGR within CSI and supported colon-separated SGR. Also supported redundant SGR specifications.
The color specification of the SGR is invalidated outside the range, and the invalid string becomes an error.