search

hackerb9 / lsix

Like "ls", but for images. Shows thumbnails in terminal using sixel graphics.
GNU General Public License v3.0
3.99k stars 129 forks source link

Doesn't detect bg colour #20

Open Tanath opened 5 years ago

Tanath commented 5 years ago

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

lsix in xterm bg

Here's my .Xresources.

hackerb9 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

Tanath commented 5 years ago

You could do xrdb -query to check for reverseVideo.

hackerb9 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

I'm aware of that, but:

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:

hackerb9 commented 5 years ago

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:

But, those seem outweighed by the pros:

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 commented 5 years ago

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).