search

neomutt / neomutt

✉️ Teaching an Old Dog New Tricks -- IRC: #neomutt on irc.libera.chat
https://neomutt.org/
GNU General Public License v2.0
3.2k stars 307 forks source link

'bright', 'alert', 'light' are only allowed for named colors: #3855

Closed misaflo closed 1 year ago

misaflo commented 1 year ago

Expected Behaviour

Colors like brightcolor223 sould works.

Actual Behaviour

I have this error at startup:

[...]/colors-gruvbox.muttrc, ligne 57 : 'bright', 'alert', 'light' are only allowed for named colors: color223
[...]/colors-gruvbox.muttrc, ligne 58 : 'bright', 'alert', 'light' are only allowed for named colors: color142
[...]/colors-gruvbox.muttrc, ligne 59 : 'bright', 'alert', 'light' are only allowed for named colors: color214
[...]/colors-gruvbox.muttrc, ligne 60 : 'bright', 'alert', 'light' are only allowed for named colors: color142

Steps to Reproduce

Add this to your color colorscheme:

color index         brightcolor223 color235 '~N'
color index_flags   brightcolor142 color235 '~N'
color index_author  brightcolor214 color235 '~N'
color index_subject brightcolor142 color235 '~N'

Or this colorscheme.

How often does this happen?

When did it start to happen?

NeoMutt Version

NeoMutt 20230512
Copyright (C) 1996-2022 Michael R. Elkins and others.
NeoMutt comes with ABSOLUTELY NO WARRANTY; for details type 'neomutt -vv'.
NeoMutt is free software, and you are welcome to redistribute it
under certain conditions; type 'neomutt -vv' for details.

System: Linux 6.3.1-arch2-1 (x86_64)
ncurses: ncurses 6.4.20221231 (compiled with 6.4.20221231)
libidn2: 2.3.4 (compiled with 2.3.4)
GPGME: 1.20.0
GnuTLS: 3.8.0
libnotmuch: 5.6.0
storage: kyotocabinet, gdbm, lmdb
compression: lz4, zlib, zstd

Configure options: --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib --gpgme --sqlite --autocrypt --lua --notmuch --gss --gnutls --sasl --with-ui=ncurses --with-idn2=/usr --disable-idn --idn2 --lmdb --kyotocabinet --gdbm --ubsan --lz4 --zlib --zstd

Compilation CFLAGS: -march=x86-64 -mtune=generic -O2 -pipe -fno-plt -fexceptions         -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security         -fstack-clash-protection -fcf-protection -g -ffile-prefix-map=/build/neomutt/src=/usr/src/debug/neomutt -flto=auto -fno-delete-null-pointer-checks -D_ALL_SOURCE=1 -D_GNU_SOURCE=1 -D__EXTENSIONS__ -D_XOPEN_SOURCE_EXTENDED -I/usr/include/lua5.3 -I/usr/include -I/usr/include -I/usr/include -DNCURSES_WIDECHAR -I/usr/include/p11-kit-1 -I/usr/include -I/usr/include -fsanitize=undefined -O0 -fno-omit-frame-pointer -fno-common

Options par défaut :
  +attach_headers_color +compose_to_sender +compress +cond_date +debug
  +encrypt_to_self +forgotten_attachments +forwref +ifdef +imap +index_color
  +initials +limit_current_thread +multiple_fcc +nested_if +new_mail +nntp +pop
  +progress +quasi_delete +regcomp +reply_with_xorig +sensible_browser +sidebar
  +skip_quoted +smtp +status_color +timeout +tls_sni +trash

Options de compilation :
  +autocrypt +fcntl -flock -fmemopen +futimens +getaddrinfo +gnutls +gpgme
  -gsasl +gss +hcache -homespool +idn +inotify -locales_hack +lua -mixmaster
  +nls +notmuch -openssl +pgp +regex +sasl +smime +sqlite +sun_attachment

Devel options:
  ubsan

MAILPATH="/var/mail"
PKGDATADIR="/usr/share/neomutt"
SENDMAIL="/usr/sbin/sendmail"
SYSCONFDIR="/etc"

Extra Info

flatcap commented 1 year ago

Hmm... there's a feature I'm not sure I was aware of.

The quick workaround is to use bold. bright on foreground colours is turned into a bold font. On background colours it has no effect.

From your example, these two are equivalent:

color index        brightcolor223 color235 '~N'
color index bold         color223 color235 '~N'

@rayfordshire Please could you take a look at this.

misaflo commented 1 year ago

It works, thanks!

MarijnS95 commented 1 year ago

This error also appears to trigger on builtin color schemes like neonwolf-256.

MarijnS95 commented 1 year ago

@flatcap thanks, the error appears to be gone now in 20230517 but nothing bright from the default neonwolf color profile is being made bold anymore: is that intended?

flatcap commented 1 year ago

oh damn :-) No that isn't intended.

rayfordshire commented 1 year ago

Before we dive into what NeoMutt did and does (its weird), we should reflect on what bright, light, alert should mean.

Here is my take on that.

IMHO "alert" should be an attribute and not a colour prefix. It is, in fact, even implemented as such, namely as "A_BLINK+A_BOLD". The latter (bold) is already an attribute, the former (blink) is not.

"light" makes sense for the first 8 standard terminal colours. The 16 standard colours are two sets of 8 colours, the second being usually a "lighter version" of the first set (the user can configure these 16 colours to their likening, so "lightblue" does not necessarily mean a lighter version of "blue". The solarized theme (ab)uses this fact [0,1,2]). Applying light to a colorNNN (NNN > 16) or #RRGGBB should be forbidden unless we design and define an algorithm which converts a colour into a lighter one. I don't intend to do that.

"bright" is a mix of "bold" and "light". I guess back in the day, some terminals only supported 8 colours, so bold was used to "light" them up. Like "light" it should only be applied to the first 8 colours. Newer NeoMutt configs should IMHO use "light" modifier or the "bold" attribute instead of "bright".

If I'm reading the code correctly, NeoMutt does the following:

light:
   color <= 7
      --> color += 8;  no bold
   color > 7
      --> nothing

bright:
   fg
      --> bold
   bg
      color <= 7
         --> color +=8
      color > 7
         --> do nothing

alert:
  --> bold+blink attribute

Which is somewhat inconsistent.

What could we do?

a) We can keep this the way it is. b) We can try to make it more consistent, e.g.

c) Add blink attribute d) We can additionally print a warning/info that instead of alert/bright the attributes bold/blink should be used.

To clarify: named colours are black, blue, ..., yellow and not colorNNN, i.e. color002 is treated slightly differently than green. (This is already the case as the former is transformed via color_xterm256_to_24bit() into an RGB value whereas the latter is the CURSES constant COLOR_BLACK. We could change that but IMOH if you say color002 you want the colour defined by the xterm palette and not the colour the user configured to be colour 2, use "green" for that)

[0] https://ethanschoonover.com/solarized/ [1] https://github.com/altercation/solarized/blob/master/xresources/solarized#L55 [2] https://github.com/altercation/mutt-colors-solarized/blob/master/mutt-colors-solarized-dark-16.muttrc

Remark for #3864: This did not fix the problem because in the colorNNN case, we use return to exit the function before we apply the is_bright/is_light/is_alert handling. Moving the parsing of the prefix string to the front did change anything.

flatcap commented 1 year ago

TL;DR a) backwards compatibility please

what NeoMutt did and does (it's weird)

hehe, yeah that's twenty years to hacking of both Mutt and ncurses.

IMHO "alert" should be an attribute and not a colour prefix

I agree

a) We can keep this the way it is.

I'd like to maintain compatibility -- as much as possible -- with existing themes. There are a lot of Mutt users out there that we'd like to convert into NeoMutt users. The fewer steps that takes, the better.

b) We can try to make it more consistent, e.g.

  • light only for named colours 0-7 (shift by 8)
  • make bright a synonym for bold (any colour) and bold+light for named colours 0-7

It's a nice thought, but I'd rather not subtly change the behaviour (even for the better).

c) Add blink attribute

urgh! No thanks :-)

d) We can additionally print a warning/info that instead of alert/bright the attributes bold/blink should be used.

We can fix our themes (and tidy them up). Perhaps we can create PRs to help the theme maintainers tidy theirs too.

After that, perhaps we can start warning.

Longer term, I'm happy to discuss throwing out all the colour code and starting again.