Created attachment 159284 [details]
Screenshot in vte.

Please attach "echo" lines to test these.  Thanks.

I'm not sure I understand you right.  Try these, the 4th out of these 6 is buggy right now.

echo $' \e[33m #3/8: Brown, normal \e[0m '
echo $' \e[33m\e[1m #3/8: Yellow, bold \e[0m '
echo $' \e[38;5;3m #3/256: Brown, normal \e[0m '
echo $' \e[38;5;3m\e[1m #3/256: Brown, bold - at least should be \e[0m '     
echo $' \e[38;5;11m #11/256: Yellow, normal \e[0m '
echo $' \e[38;5;11m\e[1m #11/256: Yellow, bold \e[0m '

Please let me know if this is not what you meant.

Should be easy to patch it up.  Though, I don't think we keep the state of whether 256 mode was enabled.

I started looking at the source, and I can imagine two ways to go. I hope you can advise me which one sounds better. I'm looking at vte-0.17.4 because that's the newest I can compile right now, hope that the solution is easily portable to head.

#1. Adjust the colors when the char cell is written, rather than when displayed on the screen.

Currently vte.c: vte_terminal_determine_colors() L8514 offsets the color when it is getting displayed (I believe). This could be removed from here, and moved to vteseq.c vte_sequence_handler_character_attributes() L2795 and other occurrences: whenever the fg color or the bold attribute is changed, adjust the current color accordingly. This would modify the semantics of vte_charcell_attr's fore, the eight old-fashioned bold characters would get the values 8-15 instead of 0-7. Furthermore, as you've mentioned (if I understood your comment correctly) we'd need a per-terminal bit telling whether the last color was set using an 8-color or 256-color escape, because the behavior of a subsequent bold escape would depend on it.

#2. Make the old and new color spaces distinct.

In vte_charcell_attr the foreground and background colors are 9 bits each. I'm not aware of any terminals supporting more than 256 colors, I believe the reason for yet another bit is the special values such as "default". This means that there's enough room to keep the old-fashioned 8 and the new 256 colors completely separate.

#1 goes towards storing the physical appearance in the charcell, #2 goes towards storing the logical state. #2 seems to be cleaner to me. E.g. it allows to implement a run-time toggle "bold on old-fashioned 8 colors makes them actually brighter" (similarly to the current "bold is actually bold"). Also there are runtime escapes modifying the actual RGB values of all 256 colors, I'm not sure if vte implements them at all, and I'm not sure if they are supposed to modify the old 8 colors too, but keeping them logically separate still allows us to manually keep them in sync, so this seems to be the more flexible solution in the long run.

What do you think?

Created attachment 159707 [details] [review]
patch going for the #1 proposed approach

I ended up going for the first approach for a quick proof of concept. Most of the patch is moving code from vte.c (where the cell is displayed) to vteseq.c (where escape sequences are interpreted). This patch fixes the current bug.

I'm not 100% certain that the patch doesn't break anything, but I hope it doesn't. Note that I've left the handling of the invisible attribute in vte.c; if I moved it to vteseq.c then changing fg color while invisible and then turning off invisibility would result in the incorrect old fg color. I'm not entirely sure that my patch doesn't have a similar problem for weird cases where you print a weird combo of the bold/half escapes. Might be just one more reason to go for the #2 approach instead?

Review of attachment 159707 [details] [review]:

not okay

My patch incorrectly handles this:

echo $'\e[31m red \e[1m bold \e[22m no longer bold \e[0m'

\e[22m should turn off bold and bright, but it only turns of bold while keeps it bright.

Now that I think of this, vteseq is not the right place to handle color adjustments either. I think color adjustment should occur when copying the current attributes to a particular cell.

I will work on this...

Created attachment 159781 [details] [review]
second attempt - this seems to be the right way to go

Ok, here's the second version. I believe this patch is pretty decent and it shows the way to go.

One thing that could be cleaned up: we already have 'defaults', 'color_defaults', 'fill_defaults' and 'basic_defaults' - four versions of the colors and attributes. Their exact semantics and purposes are not completely clear to me. (e.g. what to the non-color bits in color_defaults store?)

I've added a fifth of these, just because I wasn't sure which one I could reuse. Probably color_defaults could be used, maybe with minor modifications elsewhere in the code.

The main point is: 'defaults' contains the unadjusted colors (e.g. legacy colors are always 0-7, even when bold), while the new 'adjusted_colors' contains the adjusted version (e.g. legacy bold colors gets shifted to 8-15). The former one is used for maintaining the internal state modified by the escape sequences. The latter one (calculated from the former and the fg_256 and bg_256 bits) contains the actual color to be stored in the cells.

At this moment I like this approach better than my original #2 recommendation - it integrates quite smoothly with vte already maintaining multiple vte_charcells per screen; and after all we're talking about 256 color support, not 264 or 272 colors ;)

Behdad, I think this patch is ready to be reviewed and committed after some minor cleanups by a vte expert.

(On a side note, I don't think we need the "invisible" bit in vte_charcell any more. Similarly, I think the "reverse" bit could be removed too, and the two colors swapped when copying the colors from "defaults" to "adjusted_defaults". This could save two precious bits in that already pretty packed structure for whatever future use :))

Generally like this approach.  Let me review.

Created attachment 254114 [details] [review]
Fix updated to vte 0.34.7

Review of attachment 254114 [details] [review]:

has a leftover line

Created attachment 254115 [details] [review]
Fix updated to vte 0.34.7, try 2

Friendly ping :) - this issue got forgotten by both of us. Now I've updated the patch.

Reminder: When using SGR extended escapes to set one of the 256 colors, the bold attribute should not change the color. Currently it makes the first 8 colors of the 256 palette brighter. Xterm and Konsole leave the color intact.

Way back I proposed two possible approaches, and I ended up choosing '#1'. Apparently this is also what xterm does, grep for "sgr_extended" in its source, so I'm now quite firm that this is the right approach.

While I believe the current patch is okay (apart from coding style probably), I see some points where the code could be further improved if you agree:

- I don't like that the same structure is being used for semantically different things: "VteCell defaults" for unadjusted colors (the logical value as requestd by escape codes), "VteCell color_defaults, fill_defaults" for adjusted colors (the physical color, taking bold/dim into account). Even though documented, this looks error prone to me and could easily break after some subsequent code change.

- VteCellAttr is now fully packed (32 bits, no room to grow). The current change allows the "half" and "standout" bits to be removed from there, to be moved into "struct _VteScreen" right next to "fg_sgr_extended". This is because we don't need to remember whether a character was drawn "half" or "standout", all we need to remember (and we do already) is their actual final color. Such a change would probably also make it feasible to specify a new structure that's similar to but different from VteCell, the one that accumulates the current state including the "half" and "standout" flags - addressing the previous bullet point.

- Probably the "reverse" flag could also be handled this way. I'm not absolutely sure about this, but I think we don't need to remember if a cell was written in reverse mode, all we have to do is swap the two colors when the cell is filled in. This would free up a 3rd bit in VteCell.

- Probably the "invisible" flag could also be handled this way, although that would modify the terminal's behavior and might not be a good idea. Currently the foreground color is used when the cell is highlighted. If we want to keep the current behavior (paint invisible, but still remember the foreground color, and still remember the character for copy-paste purposes) then we need to keep it this way. However, if either the character is to be forgotten (note: xterm copy-pastes a space, so apparently forgets the actual character and substitutes with space), or if the highlight color is not important, then we can get rid of this bit too in VteCell.

Any comments are appreciated :)

Indeed, I've forgotten it...  Let me restudy.

Patch looks good to me.  Thanks!

ChPe, can you land this?  I'm not sure which branch is what at this point.  Thanks.

Looked ok to me too, so pushed to 0-34.

Originally I'd liked alternative #2 from comment 5 better, but since xterm does it like #1, we shouldn't diverge.

As for the rest of comment 15, I don't think we should get rid of the reverse, half, standout, and invisible bits, since I think having them will make printing better (once we get print support).

Also note if we needed some more attributes, VteCell actually has 10 bits free:  vteunistr only uses 22 bits (21 bits from UTF-32, plus the one for the unistr flag; VTE_UNISTR_START could simply be changed from 0x80000000 to 1<<21, afaict).

Thinking about perhaps adding true color support (bug 704449), I also changed my mind, I think approach #2 would have been better.

Old-fashioned escapes using a palette could not be immediately converted to RGB because then we'd lose the ability to dynamically redefine the palette. So we'd need to remember and distinguish at least 16M + 256 colors, that is, approach #2 would be used here. It's better to use that approach consistently throughout all possible escape sequences, rather than something hybrid.

Well, it's never too late to refactor...

Just for the record: eventually we refactored it (c7a76a9, bug 640040) so it's now approach #2 that's implemented.