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

Doesn't detect bg colour #20

Open
Tanath opened this issue Feb 25, 2019 · 16 comments
Open

Doesn't detect bg colour #20

Tanath opened this issue Feb 25, 2019 · 16 comments

Comments

@Tanath
Copy link

Tanath commented Feb 25, 2019

Supposedly it should detect bg colour, but this is what I get:

lsix in xterm bg

Here's my .Xresources.

@hackerb9
Copy link
Owner

I believe it is detecting the background color, but because you have reverse video turned on it is swapped. I think this is a bug in xterm, but I'll have to check the specs.

In the meantime, try specifying the colors you want directly in your .Xresources.

@Tanath
Copy link
Author

Tanath commented Feb 27, 2019

That is exactly what I'm trying to avoid by using reverse video. With a single line I get it looking how I want without trying to figure out and maintain a bunch of colour definitions. I don't think there's any way around the fact that with reverse video on the bg & fg colours for lsix are set incorrectly. It would help if your program could detect this and act accordingly.

@hackerb9 hackerb9 reopened this Feb 27, 2019
@hackerb9
Copy link
Owner

I agree. However, I'm not sure there is a way to detect reversed video. I'll check into it again.

@Tanath
Copy link
Author

Tanath commented Feb 27, 2019

You could do xrdb -query to check for reverseVideo.

@hackerb9
Copy link
Owner

Good suggestion. Unfortunately, that won't catch all the ways reverse video can be enabled. For example, the -rv command line switch. Holding down control and clicking the mouse. There's even an escape sequence to toggle reverse video.

What I need is an escape sequence to always get the genuine background color. Failing that, an escape sequence to detect reversed video.

@Tanath
Copy link
Author

Tanath commented Feb 28, 2019

How are you detecting fg & bg now? You can also change fg & bg with -fg and -bg. The ctrl+click menu doesn't provide a way to toggle reverse video for me. Even if it did it would be more understandable to the user why it happened and how to fix it.

I'm not sure where to get all the fancy escape sequences you've found already, so if you can't figure it out I doubt I'll have much luck.

I'm hoping you'll do what you can with what we have so far and deal with edge cases when you can. It would still be improvement.

@hackerb9
Copy link
Owner

hackerb9 commented Mar 1, 2019

I'm detecting the colors using an escape sequence. Try this:

$ IFS=$';\e'  read -a fg -r -s -d "\\" -p $'\e]10;?\e\\'
$ echo ${fg[2]}
rgb:e5e5/e5e5/e5e5
$ IFS=$';\e'  read -a bg -r -s -d "\\" -p $'\e]11;?\e\\'
$ echo ${bg[2]}
rgb:1a1a/1a1a/1a1a

XTerm responds the same if the colors are changed with -fg and -bg as with Xresources, so lsix works fine. For example, I use:

! White on black is easier on eyes of B9 (and works with nethack).
XTerm*vt100.foreground  :       Gray90
XTerm*vt100.background  :       Gray10

Reading the man page for XTerm, it seems like setting reverseVideo has no added benefit over setting the foreground and background colors. Is there some feature of reverseVideo that you use and I don't know about?

To get the XTerm menu for reverse video, hold down control and the middle mouse button. Note that lsix still does not work with reverse video that way. I'm just pointing out that xrdb is not a solution.

You can find a list of control sequences that XTerm understands here: https://invisible-island.net/xterm/ctlseqs/ctlseqs.html

@Tanath
Copy link
Author

Tanath commented Mar 1, 2019

I've set my fg & bg colours now, fixing the issue for me. I wasn't using reverseVideo for any reasons other than what I said (simpler & easier). Except that my previous attempt to set them didn't actually work because I left out the '.vt100' bit. Thanks for the pointers.

@hackerb9
Copy link
Owner

hackerb9 commented Mar 1, 2019

I'm glad it works for you. I'll leave this bug open, though, until I get this resolved with the author of XTerm. You're unlikely to be the only person who will run in to this problem.

@ThomasDickey
Copy link

The xterm manual page section on reverseVideo is a good place to start.
This paragraph is relevant:

Programs running in an xterm can also use control sequences to
enable the VT100 reverse video mode. These are independent of
the reverseVideo resource and the menu entry. Xterm exchanges
the current foreground and background colors when drawing text
affected by these control sequences.

An application can use the DECRQSS control/response for SGR to find if reverse-video is active, and take that into account in deciding what is actually displayed.

@hackerb9
Copy link
Owner

hackerb9 commented May 2, 2019

Hello Mr. Dickey,

It is always good to hear from you.

If, by DECRQSS, you mean DECRQM, yes, that is what lsix already uses to decide if reverse-video is active. I use it to check for DECSCNM, but unfortunately, that is not sufficient for XTerm. Is there some other mode number than DECSCNM I should be checking for? (By the way, I'm not worrying about the SGR inverse character attribute. If somebody has left that on, that's their issue.)

Even ignoring SGR's inverse, there are at least three variables: DECSCNM (DEC's "reverse video"), VT100 fg/bg (same as XTerm's -fg, -bg resources), and reverseVideo (XTerm's "reverse video"). The first two lsix discovers easily using DECRQM and OSC.

However, as the man page mentions, XTerm's reverseVideo resource is independent of control sequences, so it would appear there's no way to probe for it. This seems like a bug given that the colors for sixel are not likewise reversed, and so they will be wrong.

I apologize for not contacting you directly about this earlier, but I was trying to first understand the XTerm source code. In particular, why doesn't reverseVideo simply set the default state of DECSCNM instead of internally swapping foreground and background? Perhaps it was written before DECSCNM was documented? Unfortunately, I found the the reverse video logic surprisingly baroque. It would not be trivial for me to simplify it and be sure that I got it right.

Do you think the reverseVideo resource in XTerm could be implemented using DECSCNM? That would make things a lot easier. If not, could you please outline the pseudocode for how one finds the background color for XTerm? Thanks!

@ThomasDickey
Copy link

I see (actually I was busy here). Tying DECSCNM and reverseVideo together would run into the same sort of problem that I have had with the blinking cursor because the same feature could be set in more than one way.

I think that adding a report for reverseVideo (as I did for private modes 13 and 14) would solve this problem without introducing new ones.

@hackerb9
Copy link
Owner

hackerb9 commented May 3, 2019

A new report would be the easiest way for XTerm to solve this, but it feels like a kludge to patch a kludge and should be marked in the documentation as temporary and not something to be implemented by other terminals who want to be "XTerm compatible".

I say it's a kludge because XTerm currently allows the same feature (reverse video) to be set in more than one way: by specifying fg/bg or by setting reverseVideo. This causes problems and some pretty hairy code as XTerm flips the foreground and background based on both DECSCNM and reverseVideo. Having two ways to do the same thing causes headaches.

I was mistaken when I said reverseVideo should be implemented as a default setting for DECSCNM. There's a potential problem with what the terminal should show when DECSCNM is set or reset after initialization.

I've come up with an easier and I think better way. Since the reverseVideo and fg/bg settings are the two ways of setting the same feature, those are the two that should be merged.

What I'm suggesting could be documented as,

"XTerm.reverseVideo: When this resource is set, the XTerm defaults to VT100.background: black and VT100.foreground: white. If those resources are explicitly set, this setting is ignored. (Colors will not be swapped as was the default in past versions of XTerm)."

@ThomasDickey
Copy link

ThomasDickey commented May 3, 2019

I'm aware of that, but:

  • reverseVideo is set by command-line option-rv
  • reverseVideo is set by menu-toggling
  • reverseVideo (see above) is set by users rather than choosing foreground/background colors

I have a hunch based on my experience with blinking-cursor that immediately after tying DECSCNM to reverseVideo, I'll get interesting bug-reports from people who have the escape sequence pasted into their .bashrc

Continuing:

  • pro: it's much simpler to tie the two together.
  • con: it would change the existing behavior (only "new" versions of xterm would respond with the desired behavior)

@hackerb9
Copy link
Owner

hackerb9 commented May 3, 2019

I had forgotten about the menu toggling. That does change my suggestion, but I think it actually makes it simpler and better.

Instead of tying reverseVideo to DECSCNM, it makes sense to redefine reverseVideo as always swapping the VT100 foreground and background colors. This should work no matter whether it is set in the command line, via X resources, or dynamically using a menu toggle.

I believe this would be cleaner and have the benefit of working exactly the same for all users (even someone who uses xterm -fg blue -bg white -rv for white on blue). Or am I forgetting something?

Only new versions of xterm will respond correctly, of course, but that is going to be the case no matter the solution. The only major cons I can come up with are:

  • con: the current code is messy and hard to understand making it difficult to change.
  • con: the current code is messy possibly because it represents an accumulated knowledge from decades of bug reports and patches. It may look like a kludge, but maybe it's doing something incredibly clever that I don't understand.

But, those seem outweighed by the pros:

  • pro: simplicity of XTerm's code, including for future maintenance.
  • pro: easier programming for user applications (like my lsix) by not having to query a third report.
  • pro: XTerm will not be a "special case". The code that works for other terminals, including hardware, will work on XTerm.
  • pro: terminals which try to mimic XTerm will not copy a kludge.

I guess the big question is: why wasn't reverseVideo implemented as swapping foreground and background in the first place. Could it be simply that reverseVideo was implemented first?

@ThomasDickey
Copy link

It's more complicated than that: reverseVideo, foreground and background are X Toolkit features, and getting those to behave consistently from the standpoint of xterm's users took a fair amount of work (so I'm not much interested in changing how those work).

Let's just focus on whether DECSCNM should be modified to tie it to reverseVideo or whether a specific report for the state of reverseVideo (to fix the problem reported here) is less disruptive.

I suppose one way to answer that would be to see if there's (aside from testing) any real use of DECSCNM (I suspect not much, since I recall some of the other terminals not supporting that).

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

No branches or pull requests

3 participants