Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ANSIWriter doesn't support SGR 2;37;0m #320

Closed
flw-cn opened this issue Jul 2, 2019 · 13 comments
Closed

ANSIWriter doesn't support SGR 2;37;0m #320

flw-cn opened this issue Jul 2, 2019 · 13 comments

Comments

@flw-cn
Copy link

flw-cn commented Jul 2, 2019

ANSIWriter doesn't support SGR ESC [2;37;0m. This is a very popular sequence (sorry for can't give the standard which it follows).

@rivo
Copy link
Owner

rivo commented Jul 11, 2019

This sequence is turned into [white::d] which looks correct to me (2 = "faint (decreased intensity)", 37 = "foreground white"). I'm just not sure why the 0 is added as it is a reset code. Do you mean the fact that the background colour is not reset? Can you elaborate what you are expecting here?

@flw-cn
Copy link
Author

flw-cn commented Jul 14, 2019

Thank you for your reply!

Sorry rivo, I have some problems with this GitHub account, I re-describe this issue with another account(dzpao <danzipao@gmail.com>), and another problem I just discovered. If possible, I am going to provide a patch.

@dzpao
Copy link

dzpao commented Jul 14, 2019

In my environment, ESC [2;37;0m resets the foreground and background color to normal color. I use iTerm2 on my MacBook. Here's what I see on my computer:

image

In other environments I have used, like putty/xshell, they have similar display effects.

In fact, tview does not reset the background color when rendering this code, causing some applications that rely on this code to display errors, like this:

image

As can be seen from the above figure, the background color of the character on the right side of the portrait is contaminated. And below is the expected effect:

image

In fact, in order to get the above effect, I not only processed SGR 2;37;0m, but also modified a piece of code related to bold attribute. If I don't process that bold attribute, I actually see something like this:
(some of the text marked as bold black(SGR 1;30m) is invisible):

image

@dzpao
Copy link

dzpao commented Jul 14, 2019

In order to figure out what happened when 2;37;0m, I carefully studied the ANSI code, especially the CSI/SGR related specifications (I mainly rely on google.com and Wikipedia, and www.vt100.net), and iTerm2+zsh did the experiment, and I finally came up with the following points:

  • All properties are composed of only one number, except 38/48
  • Any SGR attribute can appear with other attributes
  • Allows multiple attributes to be specified for text at once (such as blinking, bold, italic underlined, like this foo)
  • Multiple attributes do not care about the order between each other, that is to say there is no fixed format (although it is customary to properties; foreground color; background color order)
  • The final visual effect is determined by the combined values of all the attributes. If a conflict is encountered, the attribute that appears later overrides the attribute that previously conflicted with it.

According to the above point of view, most of the code in ansi.go about SGR processing needs to be modified. include:

  • SGR 1 should be able to highlight the foreground color, similar to the effect of SGR 90~97.
  • SGR 21~27 should be able to properly close the corresponding properties set previously.
  • SGR 38/48 should not terminate FieldLoop, and should not be considered to have any code behind them.
  • Added support for italics.

@rivo
Copy link
Owner

rivo commented Jul 21, 2019

In your example, here is the part that I don't understand:

some of the text marked as bold black(SGR 1;30m) is invisible

You have a black background. Why would you see any text if its foreground colour is also set to "black"? It should be invisible, shouldn't it? Are you sure this is a SGR 1;30m code?

And in fact, tview translates this to [black::b] so it's according to the specification.

@dzpao
Copy link

dzpao commented Jul 31, 2019

Thank you for your reply. I took a vacation last week and I didn't see your reply in time.

I understand what you mean. But in fact I have observed several popular terminal emulators, such as iTerm2 and Terminal.app, which both render bold black to gray.

iTerm2
image

Terminal.app
image

So although I can't find the basis for doing so (except for one Chinese page on zh.wikipedia.org), I believe they do justifiably.

@rivo
Copy link
Owner

rivo commented Aug 29, 2019

So I'm not sure if this was different before. But this sequence appears to be displayed correctly. Check this out:

package main

import (
	"github.com/rivo/tview"
)

func main() {
	box := tview.NewBox().
		SetTitle(tview.TranslateANSI("\x1b[1;30;40mbold black")).
		SetBorder(true)
	if err := tview.NewApplication().SetRoot(box, true).Run(); err != nil {
		panic(err)
	}
}

This is what it looks like in iTerm:

image

If you have some (brief) code that illustrates how it doesn't work in your case, please post it here.

@dzpao
Copy link

dzpao commented Sep 12, 2019

I tried the example you gave with the latest version of the code, and the effect is different from yours. It is not correct.

Below is the process of my testing.

image

@dzpao
Copy link

dzpao commented Sep 12, 2019

You are right, I really should reproduce my problem with shorter code. I will give some other examples below. All examples are based on the latest tview code.

I wrote a piece of code to display my test case in the TextView.

package main

import (
	"io"
	"os"

	"github.com/rivo/tview"
)

func main() {
	textView := tview.NewTextView()
	textView.SetBorder(true)
	textView.SetTitle(" tview.TextView ")
	textView.SetDynamicColors(true)
	colorView := tview.ANSIWriter(textView)
	file, _ := os.Open("color-test.txt")
	io.Copy(colorView, file)
	if err := tview.NewApplication().SetRoot(textView, true).Run(); err != nil {
		panic(err)
	}
}

And here is my test cases (all \x1b are replaced with <ESC>, or you can download the file directly):
color-test.txt

<ESC>[0mCase  0.1: <ESC>[1;30;40mbold black on black<ESC>[0m

<ESC>[0mCase  1.1: <ESC>[1;33;44mbold<ESC>[0m
<ESC>[0mCase  1.2: <ESC>[2;33;44mdim<ESC>[0m
<ESC>[0mCase  1.3: <ESC>[4;33;44munderline<ESC>[0m
<ESC>[0mCase  1.4: <ESC>[5;33;44mblink<ESC>[0m

<ESC>[0mCase  2.1: <ESC>[1;33;44mbold + underline<ESC>[4m = bold + underline<ESC>[0m
<ESC>[0mCase  2.2: <ESC>[4;33;44munderline + bold<ESC>[1m = bold + underline<ESC>[0m
<ESC>[0mCase  2.3: <ESC>[1;33;44mbold + blink<ESC>[5m = bold + blink<ESC>[0m
<ESC>[0mCase  2.4: <ESC>[5;33;44mblink + bold<ESC>[1m = bold + blink<ESC>[0m
<ESC>[0mCase  2.5: <ESC>[2;33;44mdim + underline<ESC>[4m = dim + underline<ESC>[0m
<ESC>[0mCase  2.6: <ESC>[4;33;44munderline + dim<ESC>[2m = dim + underline<ESC>[0m
<ESC>[0mCase  2.7: <ESC>[2;33;44mdim + blink<ESC>[5m = dim + blink<ESC>[0m
<ESC>[0mCase  2.8: <ESC>[5;33;44mblink + dim<ESC>[2m = dim + blink<ESC>[0m
<ESC>[0mCase  2.9: <ESC>[4;33;44munderline + blink<ESC>[5m = underline + blink<ESC>[0m
<ESC>[0mCase 2.10: <ESC>[5;33;44mblink + underline<ESC>[4m = underline + blink<ESC>[0m

<ESC>[0mCase  3.1: <ESC>[4;5;33;44m(underline + blink) + bold<ESC>[1m = bold + underline + blink<ESC>[0m
<ESC>[0mCase  3.2: <ESC>[1;5;33;44m(bold + blink) + underline<ESC>[4m = bold + underline + blink<ESC>[0m
<ESC>[0mCase  3.3: <ESC>[1;4;33;44m(bold + underline) + blink<ESC>[5m = bold + underline + blink<ESC>[0m
<ESC>[0mCase  3.4: <ESC>[1;33;44mbold + (underline + blink)<ESC>[4;5m = bold + underline + blink<ESC>[0m
<ESC>[0mCase  3.5: <ESC>[4;33;44munderline + (bold + blink)<ESC>[1;5m = bold + underline + blink<ESC>[0m
<ESC>[0mCase  3.6: <ESC>[5;33;44mblink + (bold + underline)<ESC>[1;4m = bold + underline + blink<ESC>[0m
<ESC>[0mCase  3.7: <ESC>[4;5;33;44m(underline + blink) + dim<ESC>[2m = dim + underline + blink<ESC>[0m
<ESC>[0mCase  3.8: <ESC>[2;5;33;44m(dim + blink) + underline<ESC>[4m = dim + underline + blink<ESC>[0m
<ESC>[0mCase  3.9: <ESC>[2;4;33;44m(dim + underline) + blink<ESC>[5m = dim + underline + blink<ESC>[0m
<ESC>[0mCase 3.10: <ESC>[2;33;44mdim + (underline + blink)<ESC>[4;5m = dim + underline + blink<ESC>[0m
<ESC>[0mCase 3.11: <ESC>[4;33;44munderline + (dim + blink)<ESC>[2;5m = dim + underline + blink<ESC>[0m
<ESC>[0mCase 3.12: <ESC>[5;33;44mblink + (dim + underline)<ESC>[2;4m = dim + underline + blink<ESC>[0m

<ESC>[0mCase  4.1: <ESC>[1;4;33;44m(bold + underline) - bold<ESC>[22m = underline<ESC>[0m
<ESC>[0mCase  4.2: <ESC>[1;4;33;44m(bold + underline) - underline<ESC>[24m = bold<ESC>[0m
<ESC>[0mCase  4.3: <ESC>[1;5;33;44m(bold + blink) - bold<ESC>[22m = blink<ESC>[0m
<ESC>[0mCase  4.4: <ESC>[1;5;33;44m(bold + blink) - blink<ESC>[25m = bold<ESC>[0m
<ESC>[0mCase  4.5: <ESC>[2;4;33;44m(dim + underline) - dim<ESC>[22m = underline<ESC>[0m
<ESC>[0mCase  4.6: <ESC>[2;4;33;44m(dim + underline) - underline<ESC>[24m = dim<ESC>[0m
<ESC>[0mCase  4.7: <ESC>[2;5;33;44m(dim + blink) - dim<ESC>[22m = blink<ESC>[0m
<ESC>[0mCase  4.8: <ESC>[2;5;33;44m(dim + blink) - blink<ESC>[25m = dim<ESC>[0m
<ESC>[0mCase  4.9: <ESC>[4;5;33;44m(underline + blink) - underline<ESC>[24m = blink<ESC>[0m
<ESC>[0mCase 4.10: <ESC>[4;5;33;44m(underline + blink) - blink<ESC>[25m = underline<ESC>[0m

<ESC>[0mCase  5.1: <ESC>[1;4;5;33;44m(bold + underline + blink) - bold<ESC>[22m = underline + blink<ESC>[0m
<ESC>[0mCase  5.2: <ESC>[1;4;5;33;44m(bold + underline + blink) - underline<ESC>[24m = bold + blink<ESC>[0m
<ESC>[0mCase  5.3: <ESC>[1;4;5;33;44m(bold + underline + blink) - blink<ESC>[25m = bold + underline<ESC>[0m
<ESC>[0mCase  5.4: <ESC>[1;4;5;33;44m(bold + underline + blink) - underline - blink<ESC>[24;25m = bold<ESC>[0m
<ESC>[0mCase  5.5: <ESC>[1;4;5;33;44m(bold + underline + blink) - bold - blink<ESC>[22;25m = underline<ESC>[0m
<ESC>[0mCase  5.6: <ESC>[1;4;5;33;44m(bold + underline + blink) - bold - underline<ESC>[22;24m = blink<ESC>[0m
<ESC>[0mCase  5.7: <ESC>[2;4;5;33;44m(dim + underline + blink) - dim<ESC>[22m = underline + blink<ESC>[0m
<ESC>[0mCase  5.8: <ESC>[2;4;5;33;44m(dim + underline + blink) - underline<ESC>[24m = dim + blink<ESC>[0m
<ESC>[0mCase  5.9: <ESC>[2;4;5;33;44m(dim + underline + blink) - blink<ESC>[25m = dim + underline<ESC>[0m
<ESC>[0mCase 5.10: <ESC>[2;4;5;33;44m(dim + underline + blink) - underline - blink<ESC>[24;25m = dim<ESC>[0m
<ESC>[0mCase 5.11: <ESC>[2;4;5;33;44m(dim + underline + blink) - dim - blink<ESC>[22;25m = underline<ESC>[0m
<ESC>[0mCase 5.12: <ESC>[2;4;5;33;44m(dim + underline + blink) - dim - underline<ESC>[22;24m = blink<ESC>[0m

Then I tested with iTerm2, Terminal.app and tview respectively, the effect is as follows:

(iTerm2, without tmux/screen, TERM=xterm-256color, zsh 5.6.2, all the blinks are correct)
image

(Terminal.app, without tmux/screen, TERM=xterm-256color, zsh 5.6.2, all the blinks are correct)
image

(rivo/tview@master, commit ID=f8bc69b, iTerm2 and Terminal.app have the same effect)
image

I also wrote a patch that can display correctly(all the blinks are correct):
image

dzpao added a commit to dzpao/tview that referenced this issue Sep 12, 2019
* SGR code 21/22/23/24/25/27 should not turn off all attributes.
* turn on/off one of bold/dim/underline/blink should not turn off other attributes.

This should fix rivo#320
dzpao added a commit to dzpao/tview that referenced this issue Sep 12, 2019
* SGR code 21/22/23/24/25/27 should not turn off all attributes.
* turn on/off one of bold/dim/underline/blink should not turn off other attributes.

This should fix rivo#320
@dzpao
Copy link

dzpao commented Sep 12, 2019

The patch above is a phased result of my recent research on this issue. Since I don't know enough about tview, there may still be a lot of deficiencies, you can talk to me.

I hope this patch can help you solve this problem.

@dzpao
Copy link

dzpao commented Sep 13, 2019

Here are two new cases to reveal the two bugs I just discovered:

Case 6.1: <ESC>[32mhello<ESC>[m world
Case 6.2: <ESC>[32mhello
    ...   world

image

In 6.1, <ESC>[m should be able to reset the current color attributes, but it doesn't.
In 6.2, the color attributes should affect the next line, but it doesn't.

@rivo
Copy link
Owner

rivo commented Oct 17, 2019

I haven't had time yet to look at the details of your patch. But I've tried your code example. The "bold black" examples look the same directly in the terminal as well as in the application. The blinks also do. I tried iTerm2 and Terminal. There were differences in how these two terminal apps render the codes, though. Terminal actually renders "bold black" as black. You can hardly see it. And iTerm2 doesn't show blinking text but that's just my setting. Here's an example for iTerm2:

Direct:

image

tview:

image

There are some differences, though. Some of them are due to tcell requiring specific colours (there is no colour such as "use the terminal's default color", see also gdamore/tcell#292).

But the partial attribute reset is actually a bug which I need to fix. I hope to be able to resolve this soon.

dzpao added a commit to dzpao/tview that referenced this issue Jan 22, 2020
* SGR code 21/22/23/24/25/27 should not turn off all attributes.
* turn on/off one of bold/dim/underline/blink should not turn off other attributes.

This should fix rivo#320
dzpao added a commit to dzpao/tview that referenced this issue Jan 22, 2020
* SGR code 21/22/23/24/25/27 should not turn off all attributes.
* turn on/off one of bold/dim/underline/blink should not turn off other attributes.

This should fix rivo#320
@rivo rivo closed this as completed in 8aa2912 May 27, 2020
@rivo
Copy link
Owner

rivo commented May 27, 2020

Sorry it took so long. I believe the latest commit fixes these issues. Your test file looks like it's supposed to in tview. Let me know if there's anything else. (Probably best to open a new issue in that case so that the topic doesn't get lost.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants