Feature request: additional color support via common SGR escape sequences

[Ben Longbons]

8 colors: already supported by terminator
16 colors: 90-97 and 100-107 correspond to brighter versions of 30-37
and 40-47. Supported almost everywhere, except in the linux vt's.
256 colors: supported by xterm, gnome-terminal, and konsole use 38 or
48 followed by a 5 and then n in 0-255
0-7, 8-15 correspond to standard colors and bright standard colors.
Oh, just look at the script at the end.
24-bit colors: only supplied by konsole, to my knowledge. Simply \e
[38/48;2;r;g;bm.

test 256 colors in supported terminal:
#!/bin/bash
CHARS=" "
PRE="\e[48;5;"
echo System Colors:
for COLOR in {0..7}; do echo -ne "$PRE${COLOR}m$CHARS";done
echo -e '\e[0m'
for COLOR in {8..15}; do echo -ne "$PRE${COLOR}m$CHARS";done
echo -e '\e[0m'

echo Color Cube, 6x6x6:
for R in {0..5};do
for G in {0..5}; do
for B in {0..5}; do
echo -ne "$PRE$((16+(36*R)+(6*G)+B))m$CHARS"
done
echo -ne '\e[0m '
done
echo
done

echo Grayscale Ramp:
for COLOR in {232..255}; do
echo -ne "$PRE${COLOR}m$CHARS"
done
echo -e '\e[0m'

###end test script

[Ben Longbons]

Hmmm... is there some reason that
terminator.model.TerminalModel.currentStyle is a short? I guess space
considerations. The Java way would be to use an object, but if for
efficiency, you can still fit two 24-bit colors in a long with room to
spare.

[Martin Dorey]

Space was (probably) the reason, but this is plausible functionality - one that we've already said yes to in principle - and functionality costs space. I'm quite keen to spend some space like this. That way, we're perhaps more likely to knuckle down and fix the unbounded memory usage from our unbounded scrollback.

If someone came up with a patch...

[Elliott Hughes]

it's been like that since the beginning, and i too assume it was for space
reasons. that's obsolete, though, because although the original
implementation kept the styles array the same length as the line's text, i
fixed that so lines using the default style don't even have a styles
array. so 99% of lines pay nothing.

you can even see that at _render_ time, the short[] styles are converted
into runs of StyledText, each of which has a Style, and that class _does_
use two java.awt.Colors. if you were keeping the Styles around, you'd
probably want to avoid having a unique Style object for every use of the
same style.

--
Elliott Hughes, http://www.jessies.org/~enh/

[Ben Longbons]

I've edited the source to replace all instance of short/int style with Style objects, and int color with AWT colors, but now the text doesn't render. The only keypress with any visible effect is enter, which advances the line (leaving the old cursor showing). Commands still work, however, best tested by:
echo hello>/dev/pts/2
or whatever tty you already have open.

I really can't see what's wrong with this, unless it's something that was expecting == to work that now needs .equals()...
(Note: some redundancy remains in the patch, particular the static methods taking ints now point to the member methods of the objects)

I have not actually added the new color support, but at this point it'd be trivial if the important stuff worked.

[Ben Longbons]

Note: next time check the logs first. There was a really stupid
accidental tail recursion, and an NPE for the 'default' I left... fix
that, works, in TerminalView.paintStyledText.
... No, that's not working right, either, \e[0m does nothing (noticed
while using a color prompt), it needs to be directly in getDefault(),
which should be a global constant, now that it's not primitive. But
how would changing the defaults apply to an already existing window.
That might not be a worry, since the delta style is applied on render,
right?

Where's the real user's default fg/bg stored, anyway? I can't find it
programatically, I just used constants.

>  not-working.diff
> 18KViewDownload

[Martin Dorey]

> how would changing the defaults apply to an already existing window.

 

If you're storing an explicit Color, changing the color to which color index n applies to wouldn't have any effect.  The defaults might change, depending on whether you store an explicit Color or a special value, like null.  Not applying changed defaults to existing text in an existing window would be a change in behavior but perhaps not a worrying one.  Storing a color index and translating it to a color on the fly would be the alternative.  Like you've already done, not all values in the "color index" need be looked up in an array - the first few could be special, leaving the remainder as translated rgb triples.

 

> Where's the real user's default fg/bg stored, anyway?

 

This line, from your patch?

 

-                 result = Terminator.getPreferences().getColor(isForeground ? TerminatorPreferences.FOREGROUND_COLOR : TerminatorPreferences.BACKGROUND_COLOR);

 

-----Original Message-----
From: terminat...@googlegroups.com [mailto:terminat...@googlegroups.com] On Behalf Of Ben Longbons
Sent: Tuesday, August 11, 2009 13:41
To: terminator-users
Subject: [terminator-users] Re: Feature request: additional color support via common SGR escape sequences

 

 

Note: next time check the logs first. There was a really stupid

[Elliott Hughes]

yes, perhaps what you're missing is that Style doesn't fully specify
appearance; it's basically a "diff". so "underlined" could be a style, or
"red background" could be a style, or "white text on red background with
underline" could be a style, and so on. anything not explicitly specified
is taken from the context (which may be the styles already applied, or may
be the user's preferences).

note also that the selection highlight is drawn using a Style.

as for "are style shorts compared?", yes they are, in numerous places.
search for something like "style.*[!=]".

even if you're not caching common Style instances from the beginning, i
think the API should assume that doing so will be necessary at some point.
i think Style's constructors should become private.

--elliott

[Ben Longbons]

On Aug 11, 2:34 pm, "Martin Dorey" <mdo...@bluearc.com> wrote:
> > how would changing the defaults apply to an already existing window.

> <snip>
Let me rephrase that: how should changing defaults be applied? Color
has public constructors and no final methods, this approach is taken
for it's SystemColor subclass ("For systems which support the dynamic
update of the system colors (when the user changes the colors) the
actual RGB values of these symbolic colors will also change
dynamically.").

> > Where's the real user's default fg/bg stored, anyway?
>
> This line, from your patch?
>
> -                 result =
> Terminator.getPreferences().getColor(isForeground ?
> TerminatorPreferences.FOREGROUND_COLOR :
> TerminatorPreferences.BACKGROUND_COLOR);
>

No wonder I couldn't find it. I must have thought there was nothing
important in that method.
Well, I'm still getting the reset-not-working thing... and even more
things, like nano, don't work, that don't even use colors, except to
invert.
At least the 16 colors now work.

[Martin Dorey]

> For systems which support the dynamic
> update of the system colors (when the user changes the colors) the
> actual RGB values of these symbolic colors will also change
> dynamically.

Something like that sounds ideal. Using objects rather than bit twiddling gets bonus points.

-----Original Message-----
From: terminat...@googlegroups.com [mailto:terminat...@googlegroups.com] On Behalf Of Ben Longbons
Sent: Tuesday, August 11, 2009 15:59
To: terminator-users
Subject: [terminator-users] Re: Feature request: additional color support via common SGR escape sequences

[Ben Longbons]

Fixed all problems (no idea what made nano fail to render), did not implement caching styles or implement dynamic colors (which'd need to be for the basic color scheme, too). I did not even look at the terminfo file(s).

24-bit color tests
for R in {0..255};do echo -ne "\e[48;2;$R0;0m  \e[m";[ $((I%16)) == 15 ] &&echo; done
for G in {0..255};do echo -ne "\e[48;2;0;$G;0m  \e[m";[ $((I%16)) == 15 ] &&echo; done
for B in {0..255};do echo -ne "\e[48;2;0;0;$Bm  \e[m";[ $((I%16)) == 15 ] &&echo; done

One bit of behaviour that I wrote differently than konsole for the 24-bits - I assume 0 if there aren't enough arguments, whereas konsole falls back to parsing the default behaviours. I think my way is more consistent, since no one is likely to use 38/48 unless they know about the capabilities.

In case it needs to be explicit, I hereby release these changes under the GNU General Public License, version 2 or later. Use at your own risk.

[Ben Longbons]

On Aug 11, 2:52 pm, "Elliott Hughes" <e...@jessies.org> wrote:
> even if you're not caching common Style instances from the beginning, i
> think the API should assume that doing so will be necessary at some point.

I've been running this (a case where caching would have nothing to do)
for a while, and then ^C ing it, and performance does not appear
slowed. So, I don't think caching is so much a necessity as a .
sgr() { local IFS=\;; echo -ne "\e[$*m";}
clear;for R in {0..255}; do echo R is now $R;for G in {0..255}; do for
B in {0..255};do sgr 48 2 $R $G $B;echo -n ' ';done;echo;done;sgr
0;done

Also - I was thinking maybe instead of dynamic colors just do dynamic
getDefaultStyle(). Just register a preferencesChangeListener to update
the static field. It's not marked final.

[Martin Dorey]

He won't have been worried about performance. He'll have been worried about the memory usage.

----- Original Message -----
From: terminat...@googlegroups.com <terminat...@googlegroups.com>
To: terminator-users <terminat...@googlegroups.com>

Sent: Tue Aug 11 22:24:39 2009
Subject: [terminator-users] Re: Feature request: additional color support via common SGR escape sequences

[Elliott Hughes]

(i'll get round to looking at this eventually, but if anyone wanted to
speed things up, the first thing would be to compare heaps, as martin
said. jvisualvm should be up to the job.)

--elliott

[Ben Longbons]

What would maek a good test suite? I'm thinking do three runs, one
with old build, one with new build but old colors, and one with new
colors - but I'm not sure. Maybe $LS_COLORS as set by dircolors. I
guess it just doesn't matter whether they look decent at all.
I've installed and run jvisualvm, but how do I use it to profile
terminator?

[Elliott Hughes]

On Thu, August 27, 2009 14:08, Ben Longbons wrote:
> What would maek a good test suite? I'm thinking do three runs, one
> with old build, one with new build but old colors, and one with new colors
> - but I'm not sure. Maybe $LS_COLORS as set by dircolors. I
> guess it just doesn't matter whether they look decent at all.

sounds reasonable. i use "man bash" as my representative of normal text,
and vim on some source file (holding down 'j' until i reach the end) for
styled text. not terribly scientific, but i'm not usually looking for
subtle problems ;-)

> I've
> installed and run jvisualvm, but how do I use it to profile terminator?

choose the "terminator.Terminator" application in the pane on the left,
then choose the "Monitor" tab, and click the "Heap Dump" button. i expect
significant changes will be obvious from the histogram.

Help > Debugging Tools > Show Heap Usage, and then "Histogram" in the
dialog that appears might be sufficient too.

--elliott

> On Aug 27, 8:59 am, "Elliott Hughes" <e...@jessies.org> wrote:
>
>> (i'll get round to looking at this eventually, but if anyone wanted to
>> speed things up, the first thing would be to compare heaps, as martin
>> said. jvisualvm should be up to the job.)
>>
>>  --elliott
>>
>>
>>
>>
>> On Tue, August 11, 2009 22:24, Ben Longbons wrote:
>>
>>> On Aug 11, 2:52 pm, "Elliott Hughes" <e...@jessies.org> wrote:
>>>
>>
>>>> even if you're not caching common Style instances from the
>>>> beginning, i  think the API should assume that doing so will be
>>>> necessary at some point.
>>
>>> I've been running this (a case where caching would have nothing to
>>> do) for a while, and then ^C ing it, and performance does not appear
>>> slowed. So, I don't think caching is so much a necessity as a .
>>> sgr() { local IFS=\;; echo -ne "\e[$*m";} clear;for R in {0..255}; do
>>> echo R is now $R;for G in {0..255}; do for
>>> B in {0..255};do sgr 48 2 $R $G $B;echo -n '  ';done;echo;done;sgr
>>> 0;done
>>>
>>
>>> Also - I was thinking maybe instead of dynamic colors just do dynamic
>>> getDefaultStyle(). Just register a preferencesChangeListener to
>>> update the static field. It's not marked final.
>>
>> --
>> Elliott Hughes,http://www.jessies.org/~enh/
>>
> >
>
>

[Ben Longbons]

Okay, what I didn't understand that you were supposed to just run the
app simultaneously with jvisualvm and it would automatically detect
it, but anyway:
OpenJDK's version of jvisualvm (the one that is symlinked directly
from /usr/bin) is broken, but the sun jdk version hidden in /usr/lib/
jvm/java-6-sun-1.6.0.14/bin/ works, even for things running on
openjdk. Now that I've filed a bug report for that, maybe I'll
actually do some profiling tomorrow since I don't have any classes.

[Elliott Hughes]

+ private static Style defaultStyle = getStyle
(Terminator.getPreferences().getColor
(TerminatorPreferences.FOREGROUND_COLOR), Terminator.getPreference
s().getColor(TerminatorPreferences.BACKGROUND_COLOR), false, false,
false);

i don't think you want to hard-wire the default foreground and
background. i think null is the proper equivalent of the old code's
"0".


+ if(cache[index]==null) {

(i think you may as well initialize the cache in a static
initializer.)


Color foreground = style.getForeground();
+ if(foreground == null)
foreground=Terminator.getPreferences().getColor
(TerminatorPreferences.FOREGROUND_COLOR);
Color background = style.getBackground();
+ if(background == null)
background=Terminator.getPreferences().getColor
(TerminatorPreferences.BACKGROUND_COLOR);

i think this was better as it was, where Style.get(Background|
Foreground) would just do the right thing if the color was null.
Style's callers shouldn't have to know how it represents "default".

(it might make sense to do this here if we were optimizing away
repeated lookups of the preferences, but (a) we're not, (b) i haven't
seen a profile suggesting we should, and (c) even if we _do_ do that,
we should probably just let Style know when the preferences change,
and keep the defaults cached there.)


+ //nonstandard: multi argument
foreground for extra colors

i think it's only submode 2 that's not widespread, no? xterm seems to
support 38 and 48. "Set foreground color (256-color or 24-bit)." might
be clearer. (likewise for background.)


+ chunk = chunks.length>i+1?chunks[+
+i]:"";
+ value = (chunk.length() == 0) ? 0 :
Integer.parseInt(chunk);
+ switch(value) {
+ case 5://256 color
+ chunk = chunks.length>i+1?
chunks[++i]:"";
+ value = (chunk.length() ==
0) ? 0 : Integer.parseInt(chunk);
+ foreground = Palettes.getColor
(value);
+ break;
+ case 2://24 bit color - konsole only
+ chunk = chunks.length>i+1?
chunks[++i]:"";
+ int red = (chunk.length() ==
0) ? 0 : Integer.parseInt(chunk);
+ chunk = chunks.length>i+1?
chunks[++i]:"";
+ int green = (chunk.length() ==
0) ? 0 : Integer.parseInt(chunk);
+ chunk = chunks.length>i+1?
chunks[++i]:"";
+ int blue = (chunk.length() ==
0) ? 0 : Integer.parseInt(chunk);
+ foreground = new Color
(red,green,blue);
+ break;
+ default:
+ Log.warn("Unknown subattribute
(38;n) " + value + " in [" + StringUtilities.escapeForJava(sequence));
+ }

we'll have to switch to an iterator to make this code much nicer, and
any of us can do that in a separate patch later, but we can at least
collapse the duplication between cases 38 and 48 right now (assign to
a temporary and after the inner switch test 'value' to decide which
local to assign to --- i was going to suggest an "int submode" or
something rather than overwriting 'value' anyway).


+ case 90:
+ case 91:
+ case 92:
+ case 93:
+ case 94:
+ case 95:
+ case 96:
+ case 97:
+ foreground = Palettes.getColor(value -
82);
+ break;
+ case 100:
+ case 101:
+ case 102:
+ case 103:
+ case 104:
+ case 105:
+ case 106:
+ case 107:
+ background = Palettes.getColor
(value-92);
+ break;

could you add a comment for each of these groups, explaining what they
handle? (one line should be fine. extra points for using the exact
wording from whichever specification you found them in.)


other than those things (and some slightly random spacing, which i can
fix when i commit), this looks good to me.

care to give a couple of examples of command-lines/configuration
people would need to see these colors in practice? i haven't tried
anything, but it seems to me like this part of our terminfo is going
to discourage applications from trying to use this new functionality:

# We offer 8 colors, in any foreground/background combination.
colors#8,
pairs#64,

--elliott

>  working.diff
> 24KViewDownload

[Elliott Hughes]

since this seems to have stalled thanks to my slowness in responding,
and since i actually had a bit of free time today, i've made all the
changes i mentioned, including the ones i said could wait. (i also
moved a few things out of StyledText that really belonged in Style,
but i didn't spot before.) i've also updated the terminfo so vim will
work if you load a 256-color colorscheme.

i'll reply to this post with a patch.

--elliott

[Elliott Hughes]

if no-one sees anything wrong with the attached patch, i'll commit it. the
terminfo change will make curses applications started in any
already-running Terminator go monochrome (and start logging unknown escape
sequences). so we might want to commit everything but the terminfo change
and wait. (but how long? it's not obvious that would really help.)

an alternative would be to switch to "terminator-256color" as our TERMINAL
and leave the old "terminator" in place (and eventually unused). i'm not
sure it's worth the bother though.

--elliott

[Elliott Hughes]

two of the three tests here are broken. here's a new version of the
256-color script from earlier in the thread with fixed versions of
these 24-bit tests appended:

#!/bin/bash
CHARS=" "
PRE="\e[48;5;"
echo System Colors:
for COLOR in {0..7}; do echo -ne "$PRE${COLOR}m$CHARS";done
echo -e '\e[0m'
for COLOR in {8..15}; do echo -ne "$PRE${COLOR}m$CHARS";done
echo -e '\e[0m'
echo Color Cube, 6x6x6:
for R in {0..5};do
for G in {0..5}; do
for B in {0..5}; do
echo -ne "$PRE$((16+(36*R)+(6*G)+B))m$CHARS"
done
echo -ne '\e[0m '
done
echo
done
echo Grayscale Ramp:
for COLOR in {232..255}; do
echo -ne "$PRE${COLOR}m$CHARS"
done
echo -e '\e[0m'

echo Color Ramps:
for R in {0..255};do echo -ne "\e[48;2;${R};0;0m \e[m";[ $((I%16)) ==
15 ] &&echo; done; echo
for G in {0..255};do echo -ne "\e[48;2;0;${G};0m \e[m";[ $((I%16)) ==
15 ] &&echo; done; echo
for B in {0..255};do echo -ne "\e[48;2;0;0;${B}m \e[m";[ $((I%16)) ==
15 ] &&echo; done; echo

--elliott

On Aug 11, 5:52 pm, Ben Longbons <brlongb...@gmail.com> wrote:

>  working.diff
> 24KViewDownload

[Ben Longbons]

+ for (int i = 0; i < cache.length; ++i) {
+ if (i < 216) {
+ // There's a 6x6x6 color cube...
+ int r = i/36;
+ int g = (i/6) % 6;
+ int b = i % 6;
+ cache[i] = new Color(r*51, g*51, b*51);
+ } else {
+ // ...followed by a grayscale ramp.
+ int gray = (i - 215)*256/25;
+ cache[i] = new Color(gray, gray, gray);
+ }
+ }

Just a nitpick, but it'd be better to separate this into two separate
for loops than having to calculate an if statement for every
iteration.


More my fault for not replying. I've had some time but always found
something else to waste it on.

On Oct 3, 8:02 pm, "Elliott Hughes" <e...@jessies.org> wrote:
> if no-one sees anything wrong with the attached patch, i'll commit it.

[Ben Longbons]

You left one of my bugs in that. the I in I%16 should be R,G,B to make
the output for each a 16x16 square.
If you're adding it to the script, it no longer fits within 24 lines.
Of course, it should probably also use $CHARS instead of hardcoding
the spaces - the reason I used that variable in the first place was so
that I could replace it with something like `/usr/bin/printf
'\u2580\u2584'`.

[Elliott Hughes]

On Sat, October 3, 2009 20:16, Ben Longbons wrote:
>

> + for (int i = 0; i < cache.length; ++i) {
> + if (i < 216) {
> + // There's a 6x6x6 color cube...
> + int r = i/36;
> + int g = (i/6) % 6;
> + int b = i % 6;
> + cache[i] = new Color(r*51, g*51, b*51);
> + } else {
> + // ...followed by a grayscale ramp.
> + int gray = (i - 215)*256/25;
> + cache[i] = new Color(gray, gray, gray);
> + }
> + }
>
>
> Just a nitpick, but it'd be better to separate this into two separate
> for loops than having to calculate an if statement for every iteration.

looking at it more closely, i see we weren't exactly matching the xterm
colors. here's new code:

// Above the 16 system colors (which come from one of the arrays
above), the
re are 240 more colors.
// The magic numbers below are to exactly match xterm's colors.
private static final Color[] cache = new Color[240];
static {
// First is a 6x6x6 color cube.
for (int r = 0; r < 6; ++r) {
for (int g = 0; g < 6; ++g) {
for (int b = 0; b < 6; ++b) {
int index = 36*r + 6*g + b;
cache[index] =
new Color(r > 0 ? (40*r + 55) : 0,
g > 0 ? (40*g + 55) : 0,
b > 0 ? (40*b + 55) : 0);
}
}
}
// Then a grayscale ramp, intentionally leaving out black
and white.
for (int i = 0; i < 24; ++i) {
int gray = 10*i + 8;
cache[6*6*6 + i] = new Color(gray, gray, gray);
}
}

--elliott

> More my fault for not replying. I've had some time but always found
> something else to waste it on.
>
>
> On Oct 3, 8:02 pm, "Elliott Hughes" <e...@jessies.org> wrote:
>
>> if no-one sees anything wrong with the attached patch, i'll commit it.
>> On Sat, October 3, 2009 19:45, Elliott Hughes wrote:
>>
>>> since this seems to have stalled thanks to my slowness in responding,
>>> and since i actually had a bit of free time today, i've made all the
>>> changes i
>
> >
>
>

[Elliott Hughes]

On Sat, October 3, 2009 20:53, Ben Longbons wrote:
> You left one of my bugs in that. the I in I%16 should be R,G,B to make
> the output for each a 16x16 square.

i actually thoughts you'd done that on purpose to show off the horizontal
scrollbar ;-)

> If you're adding it to the script, it
> no longer fits within 24 lines. Of course, it should probably also use
> $CHARS instead of hardcoding
> the spaces - the reason I used that variable in the first place was so that
> I could replace it with something like `/usr/bin/printf
> '\u2580\u2584'`.

i hadn't thought of that; i'd added a '|' either side of the grayscale
ramp to make it clear where it begins and ends regardless of background
color. but the checkerboard works quite well (though i actually prefer
plain old '[]' because it lets more of the color show through).

i wasn't actually going to check this in, but there's no real reason not
to. though if it's meant as a test, we should probably test the legacy
sequences too.

does dickey's vttest already test the color stuff?

--elliott

[Elliott Hughes]

committed as revision 1579.

--elliott

[Martin Dorey]

> if no-one sees anything wrong with the attached patch

(14 hours might seem plenty of time to review a patch but I for one spent most of it asleep.)

> the terminfo change will make curses applications started in any
> already-running Terminator go monochrome

What happens with the new Java running against the old terminfo? I see Chris is already whining, presumably about the pain of distributing new terminfo around various machines which don't have the latest, or perhaps any, Terminator installed. If that's not actually necessary, except in the case where the new feature is required, then he needn't bother or worry about it.

I'll try to answer that question myself from the patch...

-----Original Message-----
From: terminat...@googlegroups.com [mailto:terminat...@googlegroups.com] On Behalf Of Elliott Hughes
Sent: Saturday, October 03, 2009 20:02
To: terminat...@googlegroups.com
Subject: [terminator-users] Re: Feature request: additional color support via common SGR escape sequences

[Elliott Hughes]

On Oct 4, 12:07 pm, "Martin Dorey" <mdo...@bluearc.com> wrote:
> > if no-one sees anything wrong with the attached patch
>
> (14 hours might seem plenty of time to review a patch but I for one spent most of it asleep.)

i thought your comment to chris was on this thread, and all you had to
say. otherwise i'd have waited longer.

> > the terminfo change will make curses applications started in any
> > already-running Terminator go monochrome
>
> What happens with the new Java running against the old terminfo?  I see Chris is already whining, presumably about the pain of distributing new terminfo around various machines which don't have the latest, or perhaps any, Terminator installed.  If that's not actually necessary, except in the case where the new feature is required, then he needn't bother or worry about it.
>
> I'll try to answer that question myself from the patch...

should be fine. we only add support for new sequences. nothing is
taken away.

the reason the terminfo causes trouble in the other direction is that
it says "use these new sequences for setting foreground/background
colors", and those new sequences aren't supported by the old code.

--elliott

[Chris Reece]

On 5/10/2009, at 8:07 AM, Martin Dorey wrote:

> I'll try to answer that question myself from the patch...

Please, don't fucking bother.

[Martin Dorey]

> i thought your comment to chris was on
> this thread

That rfc-violatin' Chris, taking it off-thread to say something that he should have rephrased less amusingly and said here.

> and all you had to say

For the others, Chris mentioned the attractiveness of Terminator's "infinite" scrollback to potential new users. I couldn't let that stand. I can't live with all my terminals locking up when we run out of memory and I know I'm not alone. BlueArc's QA love having a lot of scrollback but "unbounded" is too much. I didn't want to encourage the side thread by replying to any point there which belonged on the main thread.

> otherwise i'd have waited longer.

No harm done. It's only with makefile changes and direct reversions of changes that I'm relying on that I'm likely to splutter "don't check that in!".

Patch looks nice. None of the whitespace inconsistency that made it a little distracting to read the original. Thanks for polishing it.

Per discussions at the time of the original submission, I see we've moved from storing indexes into the palette to caching the color from the palette. This means that the color of existing text is no longer changed when the palette is switched. As noted at the time, I don't think that's a problematic change, but one that I might have noted in the check-in comment, in case someone was attached to that feature.

> > What happens with the new Java running against the old terminfo?

> should be fine.

Agreed, at least theoretically and I think I've tested it to some degree too.

If we'd have unconditionally changed the TERM name, then Chris (and many like him) would have had to distribute the new terminfo. Having to do that Right Now or live without a recognized term type, and without eg cursor keys, well, I think that'd clearly suck much worse than monochrome when connected from an old Terminator to a box with new terminfo, which also sounds likely to be less frequent.

We could have a preference for the emulation, which, in this case, would control nothing but TERM. To benefit, we'd leave the default as "terminator" rather than "terminator-256color", but then no-one would use the new code. Perhaps the most compelling reason to do this would be that it'd give us a less painful avenue to make further changes to terminfo, to get us closer to xterm.

If we keep 256 color support in the "terminator" terminfo, then there'd be a small potential benefit to distributing the old terminfo as terminator-8color, but I doubt anyone would ever use it. Monochrome might actually be a plus for situations where I'm ssh()d in from someone else's desk.

I'm just thinking aloud here, so we don't do anything rash if the questions start pouring in on Monday. I don't think I'd suggest changing anything preemptively.

----- Original Message -----
From: terminat...@googlegroups.com <terminat...@googlegroups.com>
To: terminator-users <terminat...@googlegroups.com>

Sent: Sun Oct 04 12:20:14 2009
Subject: [terminator-users] Re: Feature request: additional color support via common SGR escape sequences

On Oct 4, 12:07 pm, "Martin Dorey" <mdo...@bluearc.com> wrote:
> > if no-one sees anything wrong with the attached patch
>
> (14 hours might seem plenty of time to review a patch but I for one spent most of it asleep.)

i thought your comment to chris was on this thread, and all you had to
say. otherwise i'd have waited longer.

> > the terminfo change will make curses applications started in any
> > already-running Terminator go monochrome
>
> What happens with the new Java running against the old terminfo?  I see Chris is already whining, presumably about the pain of distributing new terminfo around various machines which don't have the latest, or perhaps any, Terminator installed.  If that's not actually necessary, except in the case where the new feature is required, then he needn't bother or worry about it.
>

> I'll try to answer that question myself from the patch...

[Elliott Hughes]

On Sun, October 4, 2009 13:22, Martin Dorey wrote:
>> i thought your comment to chris was on this thread
>
> That rfc-violatin' Chris, taking it off-thread to say something that he
> should have rephrased less amusingly and said here.
>
>> and all you had to say
>
> For the others, Chris mentioned the attractiveness of Terminator's
> "infinite" scrollback to potential new users. I couldn't let that stand.
> I can't live with all my terminals locking up when we run out of memory
> and I know I'm not alone. BlueArc's QA love having a lot of scrollback
> but "unbounded" is too much. I didn't want to encourage the side thread
> by replying to any point there which belonged on the main thread.
>
>> otherwise i'd have waited longer.
>
> No harm done. It's only with makefile changes and direct reversions of
> changes that I'm relying on that I'm likely to splutter "don't check that
> in!".
>
> Patch looks nice. None of the whitespace inconsistency that made it a
> little distracting to read the original. Thanks for polishing it.
>
> Per discussions at the time of the original submission, I see we've moved
> from storing indexes into the palette to caching the color from the
> palette. This means that the color of existing text is no longer changed
> when the palette is switched. As noted at the time, I don't think that's
> a problematic change, but one that I might have noted in the check-in
> comment, in case someone was attached to that feature.

i'd completely forgotten that discussion. i've added a code comment.

>>> What happens with the new Java running against the old terminfo?
>
>> should be fine.
>
> Agreed, at least theoretically and I think I've tested it to some degree
> too.
>
> If we'd have unconditionally changed the TERM name, then Chris (and many
> like him) would have had to distribute the new terminfo. Having to do
> that Right Now or live without a recognized term type, and without eg
> cursor keys, well, I think that'd clearly suck much worse than monochrome
> when connected from an old Terminator to a box with new terminfo, which
> also sounds likely to be less frequent.

indeed. i didn't think it was worth the disruption.

> We could have a preference for the emulation, which, in this case, would
> control nothing but TERM. To benefit, we'd leave the default as
> "terminator" rather than "terminator-256color", but then no-one would use
> the new code. Perhaps the most compelling reason to do this would be
> that it'd give us a less painful avenue to make further changes to
> terminfo, to get us closer to xterm.
>
> If we keep 256 color support in the "terminator" terminfo, then there'd
> be a small potential benefit to distributing the old terminfo as
> terminator-8color, but I doubt anyone would ever use it. Monochrome
> might actually be a plus for situations where I'm ssh()d in from someone
> else's desk.

i didn't think of adding a new name for the old sequences, but my concern
with changing the default name (other than that people might have it
hard-coded in various places) was that i didn't want to burn through them
too quickly. if we do change, it should probably be a big change. such as,
for example, getting more in line with xterm. but afaik, there aren't any
sequences that mean different things to xterm and rxvt, so as long as we
don't remove anything, there shouldn't be any need to change the terminfo
(except because we want to, for cleanliness; but that wouldn't mean anyone
should bother "upgrading").

> I'm just thinking aloud here, so we don't do anything rash if the
> questions start pouring in on Monday. I don't think I'd suggest changing
> anything preemptively.

vim users will find vim behaves quite differently when it knows it has
lots of colors available. as well as using more colors, it uses more bold
text. (i saw this when testing the patch, and thought it was a Terminator
bug until i found vim behaves the same with xterm-256color.)

--elliott