Re: [hackers] [PATCH] Restore cursor color customization and defaults

From: Jules Maselbas <jules.maselbas_AT_grenoble-inp.org>
Date: Wed, 18 Jul 2018 02:27:45 +0200 (CEST)

> On Tue, Jul 17, 2018 at 10:58:40PM +0200, Jules Maselbas wrote:
>> defaultcs define the (main) cursor color. defaultcfg define the secondary
>> cursor color, this is the color taken by the text color when using the
>> cursor with a block or snowman shape.(base.mode & ATTR_BOLD_FAINT) == ATTR_BOLD
>>
>> defaultcs and defaultcfg have a variant for reversed cursor (when selected)
>> respectively defaultrcs and defaultrcfg.
>>
>> In the default configuration, the cursor color is black (defaultbg) text
>> over a white (color index 256) block. Once selected the cursor color
>> will be white (defaultfg) on a grey (color index 257) block cursor.
>>
>> The cursor can be configured to follow the underlining text color:
>> Setting defaultcs to g.fg will make the cursor color follow the text fg.
>> Setting defaultcfg to g.bg will make the cursor fg color follow the text bg.
>> This can be independently done for selected cursor by setting defaulrcs to
>> g.bg and defaultrfg to g.fg.
>>
>> --- 8< ---
>>
>> This patch has been made over '5535c1f Fix crash when cursor color is truecolor'
>> and cannot be applied after the revert.
>>
>> Any feedback will be appreciated.
>> Thanks
>>
>> ---
>> config.def.h | 5 +++++
>> x.c | 13 ++++++++-----
>> 2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/config.def.h b/config.def.h
>> index ca6b0db..9f47bca 100644
>> --- a/config.def.h
>> +++ b/config.def.h
>> _AT_@ -118,6 +118,11 @@ static const char *colorname[] = {
>> */
>> unsigned int defaultfg = 7;
>> unsigned int defaultbg = 0;
>> +#define defaultcs 256 /* index or 'g.fg' */
>> +#define defaultrcs 257 /* index or 'g.bg' */
>> +/* Foreground color of cursor's Rune (when shape is Block or Snowman) */
>> +#define defaultcfg defaultbg /* index or 'g.bg' */
>> +#define defaultrcfg defaultfg /* index or 'g.fg' */
>>
>> /*
>> * Default shape of cursor
>> diff --git a/x.c b/x.c
>> index 4155a70..109ff67 100644
>> --- a/x.c
>> +++ b/x.c
>> _AT_@ -1420,13 +1420,16 @@ xdrawcursor(int cx, int cy, Glyph g, int ox, int oy,
>> Glyph og)
>> g.mode &= ATTR_BOLD|ATTR_ITALIC|ATTR_UNDERLINE|ATTR_STRUCK|ATTR_WIDE;
>>
>> if (selected(cx, cy)) {
>> - cc = g.bg;
>> + cc = g.bg = defaultrcs;
>> + g.fg = defaultrcfg;
>> } else {
>> g.mode |= ATTR_REVERSE;
>> - if (g.mode & ATTR_BOLD && BETWEEN(g.fg, 0, 7))
>> - cc = g.fg + 8;
>> - else
>> - cc = g.fg;
>> + if (g.mode & ATTR_BOLD && BETWEEN(g.fg, 0, 7)) {
>> + g.mode |= ATTR_FAINT;
>
> Is the ATTR_FAINT a separate fix? Please split up the patch if so.
The ATTR_FAINT is related to this patch.
Setting ATTR_FAINT will prevent the other block (in xdrawglyphfontspecs)
to brighten the color, guarded by (g.mode & ATTR_BOLD_FAINT) == ATTR_BOLD

No need to do this fancy test here because only few attributes are selected
on the cursor glyph, see 'g.mode &= ..' above.

>> + g.fg += 8;
Highlight is taken care here, note: now g.fg is modified (this wasn't the case before)

>> + }
>> + cc = g.fg = defaultcs;
defaultcs might be red (1) and g.mode might be bold, and we do not want this
color to be changed to bright red (9) by xdrawglyphfontspecs. But we still want
to have a bold glyph, thus ATTR_FAINT.

>> + g.bg = defaultcfg;
>> }
>>
>> if (IS_TRUECOL(cc)) {
>> --
>> 2.18.0
>>
>>
>
> The current cursor behaviour will stay as is.
This patch set default settings that should match the 'original' cursor behavior,
prior to this patch series.
Still I would like someone (else than me) to test and verify that's true.

> Please be careful in not causing regressions.
I will be more cautious from now on.

Best regards
Received on Wed Jul 18 2018 - 02:27:45 CEST