[PATCH 1/4] ply-text-display: support bright colors

[Jan Engelhardt]

On Monday 2012-07-30 21:59, Ray Strode wrote:
>
>I think rather than trying to keep this square peg, we could just drop
>PLY_TERMINAL_COLOR_DEFAULT, and introduce new apis:
>
>ply_text_display_reset_background_color (display);
>ply_text_display_reset_foreground_color (display);
>
>What do you think?

I am completely impartial to that :)

>minor review nits below:
>
>> +  if (color == PLY_TERMINAL_COLOR_DEFAULT)
>> +        color = 9;
>
>We unfortunately use GNU coding style

Somehow I must have missed that; plymouth looks a lot cleaner than glibc.

>> +  if (color >= PLY_TERMINAL_COLOR_DARKGRAY &&
>> +      color <= PLY_TERMINAL_COLOR_WHITE) {
>> +        color -= PLY_TERMINAL_COLOR_DARKGRAY;
>> +        hi = true;
>> +  } else if (color == PLY_TERMINAL_COLOR_DEFAULT) {
>> +        color = 9;
>> +  }
>>    ply_terminal_write (display->terminal,
>> -                      COLOR_SEQUENCE_FORMAT,
>> -                      FOREGROUND_COLOR_BASE + color);
>> +                      "\x1b\x5b""%d;%u;%um", hi,
>> +                      FOREGROUND_COLOR_BASE + color,
>> +                      BACKGROUND_COLOR_BASE + display->background_color);
>Should update COLOR_SEQUENCE_FORMAT to use the new format rather than
>stop using it, or remove the COLOR_SEQUENCE_FORMAT definition (though
>I'd prefer if we kept it).

It should then be removed, because you need different sequences
almost everytime. Format strings should preferably also be at the
site of where they are used, rather than stashed away into a lonely
#define somewhere at the top.

>Out of curiosity, why are you setting the background color in the
>set_foreground_color function?

Because the "0" in \e[0;...m, which is emitted when going from bold to
non-bold, resets everything, including bgcolor?