*** Bug 700341 has been marked as a duplicate of this bug. ***

I don't think vte will implement this ever.

Doesn't sound hard to implement at all.

The relevant part of the scrollback is stored in the attr_stream file with records of 16 bytes: 8 bytes offset, 4 bytes attr and 4 bytes unused padding (assuming 64 bit architecture). This means that its size would not grow at all. Memory would grow by 4 bytes for each cell, that is, around 8kB (default size) - ~50kB (my full screen) per terminal, I think it's negligible.

What we really need is a set of applications supporting true color that make spending time on this feature worthwhile. Anton: could you please help here, collecting as many apps as possible that support it (either out of the box, or via a 3rd party patch)?

I remember when I added 256 color support to midnight commander and created the first skin supporting it, I had a hard time living off of 256 colors only, I could have created something way better if I had access to the full palette. Unfortunately mc (as most other apps) uses ncurses or slang, and these (as far as I can tell) don't support true color, it would require them to bypass these libraries when it comes to color. (I might be wrong, though.) I'll take a look how easy it would be to add true colors to mc.

I think it's false that XTerm supports true color. Konsole supports it, as well as st (suckless). I couldn't find any other.

I'd be happy to add this feature, but first I'd like to see reasonably good applications that would benefit from this.

This could be a catch 22: hardly any apps support it due to lack in terminal support, and hardly any terminals support it due to the lack of killer apps...

xterm >= 282 recognizes true color escape sequences, but converts them to the nearest entry in the palette.

It also starts accepting ':' instead of ';' as the delimiter after CSI 38/48, something we should do too. The problem with semicolon is that it's supposed to delimit properties that are independent from each other and take no additional parameters. E.g. vte currently doesn't recognize ^[[38;2;1;3;4 as a true color escape sequence (rgb:1/3/4), but instead it switches to bold(1), italic(3) and underline(4) since it has no clue how many parameters 38;2 is supposed to take.

iTerm also has support: https://code.google.com/p/iterm2/issues/detail?id=218

elinks (if configured with --enable-true-color) supports true colors.

This is a one-liner image viewer, great fun: http://www.reddit.com/r/commandline/comments/1e6oev/24bit_aka_true_color_image_viewer_inside_your/

3rd party patch for vim, I haven't tried: https://bitbucket.org/ZyX_I/vim/commits/5e3877f

That's all I've found so far. Not too much. Adding support to ncurses seems hopeless, it uses "short" data type for colors everywhere. Adding support to slang seems to be hard too, at least it relies on terminfo entries (tput setaf/tput setab) which are shipped by ncurses and only contain support for 256 colors.

Also radare2 tool (http://rada.re/) have support for true colors.

Patch for emacs, and link to an ncurses discussion: http://emacs.1067599.n5.nabble.com/RFC-Add-tty-True-Color-support-td299962.html

The work should probably start with the refactoring mentioned in bug 616436 comment 19.

Created attachment 265489 [details]
image viewer for fun

Inspired by the image viewer linked above, here's one that doesn't stretch the image vertically, but rather puts two pixels in each character cell by using U+2580 (upper half block: ▀) characters and setting both the foreground and the background color. Good for fun, good for testing.

Created attachment 265490 [details] [review]
Add true color support

And here's a patch adding true color support. I think it works correctly, but please test thoroughly.

Todo:

- Use more named constants rather than some built-in 256, 8, 16 or alike numbers. The overload of VTE_PALETTE_SIZE having the meaning of offset for old-fashioned code is also ugly.

- Figure out why that attribute packed is required. Input from someone understanding gcc would be appreciated. Apparently a struct consisting of unit64 and uint32 (in this order) is 12 bytes, whereas a struct consisting of a struct of 8 bytes and a uint32 (in this order) is 16 bytes.

- Verify that compiles/works on 32 bit architecture too. (I have 64 bit.)

- Verify that attr_stream's records indeed did not grow.

ChPe, once cleaned up, would you accept this patch/feature for mainstream?

Further todos:

- accept ':' in addition to ';' in the escape sequence (can be a separate bug, not blocking this one, since konsole and st don't support this either)

- parsing params of 38 and 48 duplicates tons of code, factor them out to a helper function

- get rid of the now-obsolete _vte_terminal_map_pango_color() (first understand when/how/why it is called at all)

Created attachment 265504 [details] [review]
Add true color support, v2

Previous version segfaulted when cursor was over a truecolor cell.

Thanks Egmont!

Patch looks good to me.

Really, please request a git access account and list me and ChPe as references.

(In reply to comment #10)
> - Use more named constants rather than some built-in 256, 8, 16 or alike
> numbers. The overload of VTE_PALETTE_SIZE having the meaning of offset for
> old-fashioned code is also ugly.

Yes please .

> - Figure out why that attribute packed is required. Input from someone
> understanding gcc would be appreciated. Apparently a struct consisting of
> unit64 and uint32 (in this order) is 12 bytes, whereas a struct consisting of a
> struct of 8 bytes and a uint32 (in this order) is 16 bytes.

(BTW, bug 512860 would have added a G_GNUC_PACKED macro, but it was rejected.)
 
> - Verify that compiles/works on 32 bit architecture too. (I have 64 bit.)

I can do that.
 
> ChPe, once cleaned up, would you accept this patch/feature for mainstream?

Sure! :-)

(In reply to comment #11)
> Further todos:
> 
> - accept ':' in addition to ';' in the escape sequence (can be a separate bug,
> not blocking this one, since konsole and st don't support this either)

Yes, let's do this. I filed that already as bug , but it's fine to deal with this here.

> - parsing params of 38 and 48 duplicates tons of code, factor them out to a
> helper function

Yes, let's do that. And have some more input validation (e.g. in  VTE_PALETTE_SIZE + param - 90 + VTE_COLOR_BRIGHT_OFFSET check that @param is in the right range / clamp to the right range).

> - get rid of the now-obsolete _vte_terminal_map_pango_color() (first understand
> when/how/why it is called at all)

It's only called inside the preedit string drawing afaict, and now that with this patch we can draw any colour, not just palette colours, it should not be needed anymore.

(The colour sequences bug is bug 685759.)

(In reply to comment #14)

> Yes, let's do that. And have some more input validation (e.g. in 
> VTE_PALETTE_SIZE + param - 90 + VTE_COLOR_BRIGHT_OFFSET check that @param is in
> the right range / clamp to the right range).

This is okay, @param is the one on you switch()ed.

> > - get rid of the now-obsolete _vte_terminal_map_pango_color() (first understand
> > when/how/why it is called at all)
> 
> It's only called inside the preedit string drawing afaict, and now that with
> this patch we can draw any colour, not just palette colours, it should not be
> needed anymore.

That was my guess too, but in order to verify I'd like to understand how to trigger the execution of this code from the UI - I have no clue what that "preedit string" means. If you happen to know off the top of your head, please explain to me so you save some time for me :)

That's input method support, used e.g. by CJK input. You can try it out by adding a CJK input source in gnome control centre keyboard prefs.

attribute(packed) findings on x86_64:

Bitpacking seems to place everything not crossing an alignment boundary of its own data type. E.g. with fields like "uint32 fore:25" (as in my first patch) the field would not cross a 4 byte boundary by default, unless the structure is attribute(packed). It was only by pure accident that fore:25 and back:25 did not cross the 4 byte boundary, hence the size of VteCellAttr was the desired 8 bytes rather than 12.

Using uint32 member fields for some reason has the effect that later on this structure, when it becomes a member of another structure, behaves differently than an uint64, despite its own size being exactly 64 bits too. The right thing to do is to change all its members to uint64, then this problem disappears. VteCell and VteIntCell both become 16 bytes.

If both VteCell and the inlined structured within VteIntCell get an attribute(packed) then they both become 12 bytes and everything remains okay. I have no preference either way (tiny bit more complicated and slower binary vs wasting 4 bytes per cell), if anyone has, please speak up.

Created attachment 265632 [details] [review]
Add true color support, v3

Some progress made

Created attachment 265722 [details] [review]
Add true color support, v4

Please take a look, I'm planning to submit this.

':' as separator is still a pending issue, it's a nontrivial code change so I'll do it separately.

Review of attachment 265722 [details] [review]:

Thanks, looks good! Just a few minor points below.

::: src/vte-private.h
@@ +83,3 @@
 #define VTE_COLOR_BRIGHT_OFFSET		8
 #define VTE_COLOR_DIM_OFFSET		16
+#define VTE_RGB_COLOR			(1 << 24)

I was going to comment on v3 to do this, and here it is in v4 before I said anything. Perfect! :-)

::: src/vterowdata.h
@@ +47,2 @@
 typedef struct _VteCellAttr {
+	guint64 fragment: 1;	/* A continuation cell. */

I presume using guint64 vs guint32 doesn't have any perf implications on 32-bit, right? If so, this looks fine; but I would be ok with using guint32 with packed attrs, too.

@@ +55,3 @@
+	guint64 italic: 1;
+	guint64 fore: 25;	/* Index into color palette, or direct RGB, */
+	guint64 back: 25;	/* see vte-private.h */

Let's put a comment like /* 4-byte boundary */ between them.

@@ +79,3 @@
  */
 
+typedef struct __attribute__((__packed__)) _VteCell {

Maybe #define VTE_GNUC_PACKED for this?

From bug 512860:
+#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 6)
+#define G_GNUC_PACKED \
+  __attribute__((__packed__))
+#else
+#define G_GNUC_PACKED
+#endif  /* !__GNUC__ */
+

::: src/vteseq.c
@@ +2437,3 @@
+					param4 = g_value_get_long(value4);
+					if (G_LIKELY (param2 >= 0 && param2 < 256 && param3 >= 0 && param3 < 256 && param4 >= 0 && param4 < 256)) {
+						long value = (1 << 24) | (param2 << 16) | (param3 << 8) | param4;

gulong, or guint32.

(In reply to comment #21)

> I was going to comment on v3 to do this, and here it is in v4 before I said
> anything. Perfect! :-)

I said as a todo entry that I'd do it :)

> ::: src/vterowdata.h
> @@ +47,2 @@
>  typedef struct _VteCellAttr {
> +    guint64 fragment: 1;    /* A continuation cell. */
> 
> I presume using guint64 vs guint32 doesn't have any perf implications on
> 32-bit, right? If so, this looks fine; but I would be ok with using guint32
> with packed attrs, too.

I don't have access to 32 bit so I can't tell. As long as we have bug 572210 comment 3, it's sure not a big deal. The other approach led to a bigger maze where I couldn't figure out gcc's behavior, so I wouldn't be confident with that.

> @@ +55,3 @@
> +    guint64 italic: 1;
> +    guint64 fore: 25;    /* Index into color palette, or direct RGB, */
> +    guint64 back: 25;    /* see vte-private.h */
> 
> Let's put a comment like /* 4-byte boundary */ between them.

It's not a functional requirement, but probably does good to performance. Will do.

> Maybe #define VTE_GNUC_PACKED for this?
> +#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 6)

2.7 is 6+ years old, do we really care? Or do we support non-glibc systems w/o attrib packed?

The problem is that by omitting the attribute packed, the subsequent static assertion on the structure size would fail. Branching that assertion on glibc version sounds a bit overengineering (but doable of course). And I'd prefer to keep that assertion so things don't accidentally get out of hands.

Or, I can just drop the packed stuff and live with larger structs.

> +                        long value = (1 << 24) | (param2 << 16) | (param3 <<
> 8) | param4;
> 
> gulong, or guint32.

Okay.

Comment on attachment 265489 [details]
image viewer for fun

Would be great if you could put a copyright & licence header comment in and commit this script, too. Not sure where in the tree... a few scripts are under perf/ so let's put it there until we can reorganise things.

(In reply to comment #22)
> > Maybe #define VTE_GNUC_PACKED for this?
> > +#if     __GNUC__ > 2 || (__GNUC__ == 2 && __GNUC_MINOR__ > 6)
> 
> 2.7 is 6+ years old, do we really care? Or do we support non-glibc systems w/o
> attrib packed?

We don't care, no :-)  So it's fine to just #if __GNUC__ this one; everything else will either work or fail the static assert.

Created attachment 265733 [details] [review]
Add true color support, v5

(Why did I think __GNUC__ stood for glibc version rather than gcc? It didn't make any sense :) and of course gcc 2.7 is waaaaaaay older.)

Comment on attachment 265733 [details] [review]
Add true color support, v5

Looks good to go, thanks! :-)

Fixed in vte-0-36. Keeping open for vte-next.

Followup work (colon as separator) in bug 685759...

Looks great!

The only drawback is that it's rather slow. (Not the fault of the script; if I redirect the output and them just cat it, same happens.) It's using 2 escape sequences per cell, but anything using this for displaying images will likely do it the same way...

Indeed. Unfortunately it's also slow without the patch (when it uses weird colors). Vte in general is slow as hell :(

Also, the runlength encoding of identical attributes for the scrollback is really uneffective here. But cat'ing images won't be the typical use case for this feature.

Just for reference:

mlterm bug for truecolor support - http://sourceforge.net/mailarchive/message.php?msg_id=31828705

Here are bugs for common console programs too:

tig - https://github.com/jonas/tig/issues/227

mutt - http://dev.mutt.org/trac/ticket/3674

mcabber - https://bitbucket.org/McKael/mcabber-crew/issue/126/support-for-true-color-16-millions-colors

mc - http://www.midnight-commander.org/ticket/3145

s-lang library - http://mailman.jedsoft.org/pipermail/slang-users-l/2014/000798.html

FYI: This is now being continued/revised in bug 791456.