Add support for specifying both foreground and background colors. #49
Conversation
for example:
\033[48;5;200;38;5;100mHello
results in a foreground color (100) and a background color (200)
cswank
changed the title
escape_test.go
Outdated
| @@ -0,0 +1,61 @@ | |||
| package gocui | |||
There was a problem hiding this comment.
I think we don't want any tests in this library at this point in time.
Maybe in the further but for now this file is not needed an can be removed.
|
Yes, I'm pretty sure that specifying foreground and background at the same time is supported on major terminals. I tested a few with printf '\033[48;5;130;38;5;40mHi\033[0m\n' |
escape.go
Outdated
| case p == 7: | ||
| ei.curFgColor |= AttrReverse | ||
| } | ||
| func (ei *escapeInterpreter) splitFgBg() [][]string { |
There was a problem hiding this comment.
does this function need to be a method of ei, cant this just be a function we can call like
func splitFgBg() [][]string { ... return out}
There was a problem hiding this comment.
spltFfBg is very specific to the escapeInterpreter, so to me it makes sense to keep it as is.
There was a problem hiding this comment.
I get that, but it does not rely on ei, it just relies on data. It is not driven by the struct
There was a problem hiding this comment.
Not a big deal to me. I'll change it. Just a sec.
escape.go
Outdated
| if err != nil { | ||
| return errCSIParseError | ||
| switch fgbg { | ||
| case 38: |
There was a problem hiding this comment.
can we change this magic number stuff while we are at it
There was a problem hiding this comment.
I pushed a change that I think is what you want.
|
This PR should not be merged yet. I just thought of an edge case and recovered escape_test.go that you asked me to remove and added another test case in which the background color being set is 38.
38 is a valid color but also the code for setting the foreground color. It causes escapeInterpreter to panic with my changes. Would you prefer I close this PR and open another one or just ping you when I think I have this edge case fixed? |
|
I think that latest commit fixes the edge cases I thought of. |
|
I also realized that this only supports 8 bit colors (ei.output256()). I think outputNormal() needs the same behavior. I'll work on that now. |
|
It looks like OutputNormal already supports specifying foreground and background at the same time. So this should be good to go. |
|
Thanks for making the PR |
for example:
results in a foreground color (100) and a background color (200)
Since this also introduces the first test, what would you think about using testify/assert in the tests?