GNU bug report logs - #50179
[PATCH] Add support for "bright" ANSI colors to ansi-color and term-mode

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 50179 in the body.
You can then email your comments to 50179 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Add support for "bright" ANSI colors to ansi-color and
 term-mode
Date: Mon, 23 Aug 2021 21:02:46 -0700

[Message part 1 (text/plain, inline)]

Note: My copyright assignment paperwork is out of date, but I've
already sent a message to assign@ to get it updated. Thus, these
patches won't be able to merge right away. However, I wanted to start
the review process now so that there's plenty of time for
back-and-forth before Emacs 28.1 is released.

With the administrative issues out of the way... these patches provide
support for "bright" ANSI colors (SGR 90-97 and 100-107 for foreground
and background, respectively)[1]. Most of the complexity here is due
to the new defcustoms `*-bold-is-bright'. Enabling this results in
ANSI "bold" text (SGR 1) to be rendered in the bright color palette
(as well as being bold). This is a pretty common option in terminal
emulators; all the ones I looked at[2] support it, and it's often the
default behavior. For me, the main benefit of this option is so I can
easily match the color palettes between Emacs and my terminal
emulator.

I've split this into two patches, one for 'ansi-color' and one for
'term-mode'. Despite the similarity in functionality, the
implementations are pretty different. It might be nice if they could
be unified somehow, but that may be more trouble than it's worth...

- Jim

[1] https://en.wikipedia.org/wiki/ANSI_escape_code#SGR_(Select_Graphic_Rendition)_parameters
[2] gnome-terminal, alacritty, and PuTTY

[0001-Add-support-for-bright-ANSI-colors-in-ansi-color.patch (application/octet-stream, attachment)]

[0002-Add-support-for-bright-ANSI-colors-in-term-mode.patch (application/octet-stream, attachment)]

Message #8 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Tue, 24 Aug 2021 15:07:01 +0300

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Mon, 23 Aug 2021 21:02:46 -0700
> 
> With the administrative issues out of the way... these patches provide
> support for "bright" ANSI colors (SGR 90-97 and 100-107 for foreground
> and background, respectively)[1]. Most of the complexity here is due
> to the new defcustoms `*-bold-is-bright'. Enabling this results in
> ANSI "bold" text (SGR 1) to be rendered in the bright color palette
> (as well as being bold). This is a pretty common option in terminal
> emulators; all the ones I looked at[2] support it, and it's often the
> default behavior. For me, the main benefit of this option is so I can
> easily match the color palettes between Emacs and my terminal
> emulator.
> 
> I've split this into two patches, one for 'ansi-color' and one for
> 'term-mode'. Despite the similarity in functionality, the
> implementations are pretty different. It might be nice if they could
> be unified somehow, but that may be more trouble than it's worth...

Thanks, please see some comments below.

> +(defcustom ansi-bright-color-names-vector
> +  ["gray30" "red2" "green2" "yellow2" "blue1" "magenta2" "cyan2" "white"]
> +  "Colors used for SGR control sequences determining a \"bright\" color.
> +This vector holds the colors used for SGR control sequences parameters
> +90 to 97 (bright foreground colors) and 100 to 107 (brightbackground
> +colors).

I wouldn't offer a customizable list for this: users have no
particular reason to redefine standard colors.

>  (defun ansi-color--find-face (codes)
>    "Return the face corresponding to CODES."
> -  (let (faces)
> +  ;; Sort the codes in ascending order to can guarantee that "bold" comes
                                          ^^^^^^^^^^^^^^^^
Something wrong with the wording here.

> (term-color-bright-*): New faces.

Please name the new faces explicitly.

> +(defcustom term-color-bold-is-bright nil
> +  "If set to non-nil, combining ANSI bold and a color produces the bright
> +version of that color."
> +  :group 'term
> +  :type 'boolean
> +  :version "28.1")

Do we really need 2 separate knobs for these two features?  How
probable is it that the same user will want to have bright colors in
one package, but not in the other?

Message #11 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Tue, 24 Aug 2021 10:38:06 -0700

On Tue, Aug 24, 2021 at 5:07 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > +(defcustom ansi-bright-color-names-vector
> > +  ["gray30" "red2" "green2" "yellow2" "blue1" "magenta2" "cyan2" "white"]
> > +  "Colors used for SGR control sequences determining a \"bright\" color.
> > +This vector holds the colors used for SGR control sequences parameters
> > +90 to 97 (bright foreground colors) and 100 to 107 (brightbackground
> > +colors).
>
> I wouldn't offer a customizable list for this: users have no
> particular reason to redefine standard colors.

I made this a defcustom because `ansi-color-faces-vector' and
`ansi-color-names-vector' are defcustoms too. More practically
speaking, I'd want to customize this new variable to make these colors
match the Emacs theme I use. I chose colors in this patch to
complement `ansi-color-names-vector', but they'd clash with my theme.

> > +(defcustom term-color-bold-is-bright nil
> > +  "If set to non-nil, combining ANSI bold and a color produces the bright
> > +version of that color."
> > +  :group 'term
> > +  :type 'boolean
> > +  :version "28.1")
>
> Do we really need 2 separate knobs for these two features?  How
> probable is it that the same user will want to have bright colors in
> one package, but not in the other?

I doubt anyone would want to control these independently. It'd be nice
to have a single defcustom for this, but I wasn't sure where it should
go in that case. Would it be ok to use `ansi-color-bold-is-bright'
(from patch 1 in `ansi-color.el') in `term.el'? I see that `term.el`
requires `comint.el', which requires `ansi-color.el', so `term.el'
should see it without changing what gets loaded.

(It might even be nice to use the same definitions for the colors in
both `ansi-color.el' and `term.el', but that would take a bit of work
to migrate users'/themes' customizations. Maybe that's something to do
for Emacs 29 so I have time to figure out a good migration path.)

Message #14 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Tue, 24 Aug 2021 20:59:40 +0300

> From: Jim Porter <jporterbugs <at> gmail.com>
> Date: Tue, 24 Aug 2021 10:38:06 -0700
> Cc: 50179 <at> debbugs.gnu.org
> 
> > I wouldn't offer a customizable list for this: users have no
> > particular reason to redefine standard colors.
> 
> I made this a defcustom because `ansi-color-faces-vector' and
> `ansi-color-names-vector' are defcustoms too. More practically
> speaking, I'd want to customize this new variable to make these colors
> match the Emacs theme I use. I chose colors in this patch to
> complement `ansi-color-names-vector', but they'd clash with my theme.

How can named colors change with the theme?  Faces can, but colors are
absolute.  Bright-yellow is the same color whatever the theme.  At
least IMO.  I wonder if others think otherwise.

> > > +(defcustom term-color-bold-is-bright nil
> > > +  "If set to non-nil, combining ANSI bold and a color produces the bright
> > > +version of that color."
> > > +  :group 'term
> > > +  :type 'boolean
> > > +  :version "28.1")
> >
> > Do we really need 2 separate knobs for these two features?  How
> > probable is it that the same user will want to have bright colors in
> > one package, but not in the other?
> 
> I doubt anyone would want to control these independently. It'd be nice
> to have a single defcustom for this, but I wasn't sure where it should
> go in that case.

If we agree to have a single defcustom, then where to put it is
secondary.

> Would it be ok to use `ansi-color-bold-is-bright'
> (from patch 1 in `ansi-color.el') in `term.el'?

Yes, why not?

Thanks.

Message #17 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Tue, 24 Aug 2021 11:59:53 -0700

On Tue, Aug 24, 2021 at 10:59 AM Eli Zaretskii <eliz <at> gnu.org> wrote:
>
> > From: Jim Porter <jporterbugs <at> gmail.com>
> > Date: Tue, 24 Aug 2021 10:38:06 -0700
> > Cc: 50179 <at> debbugs.gnu.org
> >
> > > I wouldn't offer a customizable list for this: users have no
> > > particular reason to redefine standard colors.
> >
> > I made this a defcustom because `ansi-color-faces-vector' and
> > `ansi-color-names-vector' are defcustoms too. More practically
> > speaking, I'd want to customize this new variable to make these colors
> > match the Emacs theme I use. I chose colors in this patch to
> > complement `ansi-color-names-vector', but they'd clash with my theme.
>
> How can named colors change with the theme?  Faces can, but colors are
> absolute.  Bright-yellow is the same color whatever the theme.  At
> least IMO.  I wonder if others think otherwise.

The colors in `ansi-bright-color-names-vector' are just the color
values that get used to set face attributes on bits of text. They (and
the values in the other defcustoms mentioned above) get compiled into
`ansi-color-map', which contains entries like `(foreground-color .
"yellow2")'. Those then get applied as face attributes onto the
relevant text. While you'd always want bright-yellow to be some kind
of bright-looking yellow, the exact RGB value for that is just a
matter of preference. In particular, a theme might need to adjust
these color values so that there's appropriate contrast between
ANSI-colorized text and the default foreground/background color
specified by the theme.

> > Would it be ok to use `ansi-color-bold-is-bright'
> > (from patch 1 in `ansi-color.el') in `term.el'?
>
> Yes, why not?

Mostly just my relative unfamiliarity with the preferred way to do
things here. `ansi-color.el' and `term.el' seemed very independent of
each other, and I wasn't sure if there was a particular reason for
that. I'll just use `ansi-color-bold-is-bright' for both patches then.

Message #20 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Tue, 24 Aug 2021 15:53:52 -0700

[Message part 1 (text/plain, inline)]

Ok, here are updated patches addressing your comments. I fixed the
grammatical error in the comment, improved the commit message for the
second patch, and switched to using `ansi-color-bold-is-bright' in
both files. I left `ansi-bright-color-names-vector' as a defcustom
though, since I think that's the right thing to do there (see my
previous message), but I can change that later if we agree on a
different/better route there.

[0001-Add-support-for-bright-ANSI-colors-in-ansi-color.patch (application/octet-stream, attachment)]

[0002-Add-support-for-bright-ANSI-colors-in-term-mode.patch (application/octet-stream, attachment)]

Message #23 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 25 Aug 2021 09:06:35 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> > I wouldn't offer a customizable list for this: users have no
>> > particular reason to redefine standard colors.
>> 
>> I made this a defcustom because `ansi-color-faces-vector' and
>> `ansi-color-names-vector' are defcustoms too. More practically
>> speaking, I'd want to customize this new variable to make these colors
>> match the Emacs theme I use. I chose colors in this patch to
>> complement `ansi-color-names-vector', but they'd clash with my theme.
>
> How can named colors change with the theme?  Faces can, but colors are
> absolute.  Bright-yellow is the same color whatever the theme.  At
> least IMO.  I wonder if others think otherwise.

FWIW, a couple of Emacs's built-in themes set these ansi-color options.
See e.g. the Modus themes, which are designed to meet WCAG's highest
standards for colour contrast.

Maybe there's an analogy to make with terminal emulators?  Most of those
(e.g. Konsole, Terminator, Xfce's) allow the user to customize the
16-color palette.

Digging into the history of this 4-bit palette[1], it almost seems like
no two consoles ever used the exact same colors, so these color names
might not be as absolute as say, HTML color names[2].


[1] https://en.wikipedia.org/wiki/ANSI_escape_code#Colors
[2] https://en.wikipedia.org/wiki/Web_colors#HTML_color_names

Message #26 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
Cc: jporterbugs <at> gmail.com, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 25 Aug 2021 14:57:30 +0300

> From: Kévin Le Gouguec <kevin.legouguec <at> gmail.com>
> Cc: Jim Porter <jporterbugs <at> gmail.com>,  50179 <at> debbugs.gnu.org
> Date: Wed, 25 Aug 2021 09:06:35 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> I made this a defcustom because `ansi-color-faces-vector' and
> >> `ansi-color-names-vector' are defcustoms too. More practically
> >> speaking, I'd want to customize this new variable to make these colors
> >> match the Emacs theme I use. I chose colors in this patch to
> >> complement `ansi-color-names-vector', but they'd clash with my theme.
> >
> > How can named colors change with the theme?  Faces can, but colors are
> > absolute.  Bright-yellow is the same color whatever the theme.  At
> > least IMO.  I wonder if others think otherwise.
> 
> FWIW, a couple of Emacs's built-in themes set these ansi-color options.
> See e.g. the Modus themes, which are designed to meet WCAG's highest
> standards for colour contrast.

It's OK to set a variable.  I was asking why would we want to offer
users the opportunity to customize these translations.

> Maybe there's an analogy to make with terminal emulators?  Most of those
> (e.g. Konsole, Terminator, Xfce's) allow the user to customize the
> 16-color palette.

Once again, if someone _really_ wants that, they can set a variable
allright.  I'm asking why would _we_ want that.

> Digging into the history of this 4-bit palette[1], it almost seems like
> no two consoles ever used the exact same colors, so these color names
> might not be as absolute as say, HTML color names[2].

I don't see how this justifies a desfcustom, sorry.  If there's no
110% consensus, we can choose whatever similar color we want.

Message #29 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 25 Aug 2021 14:04:00 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> Ok, here are updated patches addressing your comments. I fixed the
> grammatical error in the comment, improved the commit message for the
> second patch, and switched to using `ansi-color-bold-is-bright' in
> both files. I left `ansi-bright-color-names-vector' as a defcustom
> though, since I think that's the right thing to do there (see my
> previous message), but I can change that later if we agree on a
> different/better route there.

Thanks; applied to Emacs 28.  But unfortunately, I didn't notice the
test failure until after pushing.  :-/

1 unexpected results:
   FAILED  term-colors-bold-is-bright

And the faces are:

(font-lock-face ((:foreground "yellow3" :background "unspecified-bg"
 :inverse-video nil) :inherit term-bold))
(font-lock-face ((:foreground "yellow2" :background "unspecified-bg"
 :inverse-video nil) :inherit term-bold))


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #32 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 25 Aug 2021 09:41:52 -0700

[Message part 1 (text/plain, inline)]

Oops, I ran the tests locally, but I think I forgot to rebuild
term.elc, so naturally the tests passed without needing any changes.
Here's a patch.

However, I just want to be sure it's ok for these patches to merge. As
mentioned in the original message, my copyright assignment is
currently out of date. I contacted assign@ to get it updated, and I
don't foresee any problems, but you never know with companies. If the
maintainers are ok with the patch being in-tree despite this, then I'm
ok with it too; I just wanted to be sure everyone was aware of the
situation.

On Wed, Aug 25, 2021 at 5:04 AM Lars Ingebrigtsen <larsi <at> gnus.org> wrote:
>
> Jim Porter <jporterbugs <at> gmail.com> writes:
>
> > Ok, here are updated patches addressing your comments. I fixed the
> > grammatical error in the comment, improved the commit message for the
> > second patch, and switched to using `ansi-color-bold-is-bright' in
> > both files. I left `ansi-bright-color-names-vector' as a defcustom
> > though, since I think that's the right thing to do there (see my
> > previous message), but I can change that later if we agree on a
> > different/better route there.
>
> Thanks; applied to Emacs 28.  But unfortunately, I didn't notice the
> test failure until after pushing.  :-/
>
> 1 unexpected results:
>    FAILED  term-colors-bold-is-bright
>
> And the faces are:
>
> (font-lock-face ((:foreground "yellow3" :background "unspecified-bg"
>  :inverse-video nil) :inherit term-bold))
> (font-lock-face ((:foreground "yellow2" :background "unspecified-bg"
>  :inverse-video nil) :inherit term-bold))
>
>
> --
> (domestic pets only, the antidote for overdose, milk.)
>    bloggy blog: http://lars.ingebrigtsen.no

[0001-Update-a-test-that-got-missed-for-bug-50179.patch (application/octet-stream, attachment)]

Message #35 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 25 Aug 2021 18:46:51 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> Oops, I ran the tests locally, but I think I forgot to rebuild
> term.elc, so naturally the tests passed without needing any changes.
> Here's a patch.

Thanks.

> However, I just want to be sure it's ok for these patches to merge. As
> mentioned in the original message, my copyright assignment is
> currently out of date.

Sorry; I did read that yesterday, but today I'd completely forgotten it
again -- I did mean to wait until the paperwork was sorted.

> I contacted assign@ to get it updated, and I don't foresee any
> problems, but you never know with companies. If the maintainers are ok
> with the patch being in-tree despite this, then I'm ok with it too; I
> just wanted to be sure everyone was aware of the situation.

Hm...  perhaps the best thing to do would be to revert the two patches
while waiting for the paperwork to be finalised.  On the other hand,
that's more churn in the VC.  Uhm...  I'm not sure whether we have a
policy here.  Eli?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #38 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: jporterbugs <at> gmail.com, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 25 Aug 2021 19:54:08 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: Eli Zaretskii <eliz <at> gnu.org>,  50179 <at> debbugs.gnu.org
> Date: Wed, 25 Aug 2021 18:46:51 +0200
> 
> > I contacted assign@ to get it updated, and I don't foresee any
> > problems, but you never know with companies. If the maintainers are ok
> > with the patch being in-tree despite this, then I'm ok with it too; I
> > just wanted to be sure everyone was aware of the situation.
> 
> Hm...  perhaps the best thing to do would be to revert the two patches
> while waiting for the paperwork to be finalised.  On the other hand,
> that's more churn in the VC.  Uhm...  I'm not sure whether we have a
> policy here.  Eli?

I think we have to revert and wait for the paperwork to run to
completion.

Message #41 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: jporterbugs <at> gmail.com, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Thu, 26 Aug 2021 15:23:03 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

> I think we have to revert and wait for the paperwork to run to
> completion.

OK; now done.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #44 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>, Eli Zaretskii <eliz <at> gnu.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [UPDATED PATCH] Add support for "bright" ANSI colors
 to ansi-color and term-mode
Date: Sat, 18 Sep 2021 11:58:12 -0700

[Message part 1 (text/plain, inline)]

On 8/26/2021 6:23 AM, Lars Ingebrigtsen wrote:
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
>> I think we have to revert and wait for the paperwork to run to
>> completion.
> 
> OK; now done.

Ok, my paperwork is complete. I've attached updated patches that should 
apply cleanly again (I had to fix the merge to account for changes to 
NEWS). I also rolled my fix to the term.el tests and Eli's fix to 
include version info in the new term faces into the second patch.

There's one further enhancement that might make sense here: currently, 
color values are set via a vector of strings for `ansi-color', whereas 
they're set via faces for `term-mode'. Would it be better to use faces 
for `ansi-color' too? Perhaps they should even be the *same* faces; is 
there any reason an Emacs user (or package) would want ANSI colors to be 
different between `ansi-color' and `term-mode'? If so, maybe each 
`term-color-FOO' face should inherit from the (hypothetical) 
corresponding `ansi-color-FOO' face?

However, maybe there's a particular reason why `ansi-color' works this 
way that I'm not aware of. If the current way is best, that's fine by me 
too. Otherwise, just let me know and I can update the patches to include 
`ansi-color-FOO' faces.

[0001-Add-support-for-bright-ANSI-colors-in-ansi-color.patch (text/plain, attachment)]

[0002-Add-support-for-bright-ANSI-colors-in-term-mode.patch (text/plain, attachment)]

Message #47 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [UPDATED PATCH] Add support for "bright" ANSI colors
 to ansi-color and term-mode
Date: Sun, 19 Sep 2021 16:45:48 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> There's one further enhancement that might make sense here: currently,
> color values are set via a vector of strings for `ansi-color', whereas
> they're set via faces for `term-mode'. Would it be better to use faces
> for `ansi-color' too? Perhaps they should even be the *same* faces; is
> there any reason an Emacs user (or package) would want ANSI colors to
> be different between `ansi-color' and `term-mode'? If so, maybe each
> `term-color-FOO' face should inherit from the (hypothetical)
> corresponding `ansi-color-FOO' face?

It seems like term.el was reworked in

commit ae4969c2d69a74c896eb49c9a34aeb645ffed082
Author:     Julien Danjou <julien <at> danjou.info>
AuthorDate: Thu Jun 28 12:40:24 2012 +0200

to use faces instead of colour names, but ansi-color wasn't.  Looking at
the code in both ansi-color and term, I think it would indeed make sense
to rework ansi-color to use faces, too, and inheriting like you suggest
makes sense to me.  (We generally prefer using faces instead of colour
names these days.)  But:

> However, maybe there's a particular reason why `ansi-color' works this
> way that I'm not aware of. If the current way is best, that's fine by
> me too. Otherwise, just let me know and I can update the patches to
> include `ansi-color-FOO' faces.

I'm not really very familiar with ansi-color.el.  Does anybody else have
an opinion here?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #52 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [WIP PATCH v3] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 22 Sep 2021 12:39:55 -0700

[Message part 1 (text/plain, inline)]

On 9/19/2021 7:45 AM, Lars Ingebrigtsen wrote:
> It seems like term.el was reworked in
> 
> commit ae4969c2d69a74c896eb49c9a34aeb645ffed082
> Author:     Julien Danjou <julien <at> danjou.info>
> AuthorDate: Thu Jun 28 12:40:24 2012 +0200
> 
> to use faces instead of colour names, but ansi-color wasn't.  Looking at
> the code in both ansi-color and term, I think it would indeed make sense
> to rework ansi-color to use faces, too, and inheriting like you suggest
> makes sense to me.  (We generally prefer using faces instead of colour
> names these days.)

Since no one chimed in opposing this change, here's a WIP patch that 
adds faces to ansi-color. This removes the use for `ansi-color-map', 
since `ansi-color-get-face-1' does all the work now. I benchmarked this 
a bit and the performance seems on par with the old implementation, 
except when we fontify *more* stuff thanks to the support for additional 
ANSI escapes; in that case, it takes longer, scaling linearly with the 
amount that gets fontified.

I also took the opportunity to redefine a couple of the faces. For 
example, ANSI SGR 7 (negative image) was defined to use the `error' 
face. Given that `:inverse-video' exists, that seems pretty suboptimal.

I don't quite know what to do about man.el though. It has overrides for 
a few of the faces used by ansi-color. I could maintain that behavior 
fairly easily, but maybe it makes sense to have it use the defaults from 
ansi-color. More generally, I wonder what we should do to accommodate 
users of ansi-color who want to apply non-default faces for a specific 
case. I guess they could redefine `ansi-color-basic-faces-vector' and 
friends? That's basically how man.el used to work, and it wouldn't be 
hard to continue supporting that if we wanted.

[0001-Add-support-for-bright-ANSI-colors-in-ansi-color.patch (text/plain, attachment)]

[0002-Add-support-for-bright-ANSI-colors-in-term-mode.patch (text/plain, attachment)]

Message #55 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [WIP PATCH v3] Add support for "bright" ANSI colors
 to ansi-color and term-mode
Date: Wed, 22 Sep 2021 21:49:23 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> I don't quite know what to do about man.el though. It has overrides
> for a few of the faces used by ansi-color. I could maintain that
> behavior fairly easily, but maybe it makes sense to have it use the
> defaults from ansi-color.

Hm...  I wonder why man.el is overriding the colours?

Skimming the patch, it looks good to me, but some trivial comments:

> -(defcustom ansi-color-faces-vector
> -  [default bold default italic underline success warning error]

Instead of removing the variable, mark it as obsolete instead.  (Users
may have code that references it in their .emacs files, and that
shouldn't bug out.)

> +(defconst ansi-color-basic-faces-vector
> +  [nil

[...]

> -  :type '(vector face face face face face face face face)
> -  :set 'ansi-color-map-update
> -  :initialize 'custom-initialize-default
> -  :group 'ansi-colors)

Needs a :version.

> -(defcustom ansi-color-names-vector
> -  ["black" "red3" "green3" "yellow3" "blue2" "magenta3" "cyan3" "gray90"]

Same thing with obsoletion here.

> -(defvar Man-ansi-color-map (let ((ansi-color-faces-vector
> -				  [ default Man-overstrike default Man-underline
> -				    Man-underline default default Man-reverse ]))
> -			     (ansi-color-make-color-map))
> -  "The value used here for `ansi-color-map'.")

And same obsoletion thing here.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #58 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH v4] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Wed, 22 Sep 2021 18:47:25 -0700

[Message part 1 (text/plain, inline)]

On 9/22/2021 12:49 PM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> I don't quite know what to do about man.el though. It has overrides
>> for a few of the faces used by ansi-color. I could maintain that
>> behavior fairly easily, but maybe it makes sense to have it use the
>> defaults from ansi-color.
> 
> Hm...  I wonder why man.el is overriding the colours?

Since I'm not totally sure, I maintained that behavior. It's not really 
any extra effort, so someone can make that decision later.

> Skimming the patch, it looks good to me, but some trivial comments:
> 
>> -(defcustom ansi-color-faces-vector
>> -  [default bold default italic underline success warning error]
> 
> Instead of removing the variable, mark it as obsolete instead.  (Users
> may have code that references it in their .emacs files, and that
> shouldn't bug out.)

Ok, I've done this here, and all the other places as well. I also 
obsoleted the old `ansi-color-map' and friends instead of deleting them 
for the same reason. Hopefully all that's right; I tried to make sure 
that they warn the user if they use them, but that warnings don't show 
up when building Emacs.

[0001-Add-support-for-bright-ANSI-colors-in-ansi-color.patch (text/plain, attachment)]

[0002-Add-support-for-bright-ANSI-colors-in-term-mode.patch (text/plain, attachment)]

Message #61 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH v4] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Thu, 23 Sep 2021 22:58:34 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> Ok, I've done this here, and all the other places as well. I also
> obsoleted the old `ansi-color-map' and friends instead of deleting
> them for the same reason. Hopefully all that's right; I tried to make
> sure that they warn the user if they use them, but that warnings don't
> show up when building Emacs.

Thanks; looks good to me.  There was a typo in the man.el part of the
patch, but I think I fixed it -- can you have a look and see whether I
did the correct thing?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Message #66 received at 50179 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 50179 <at> debbugs.gnu.org
Subject: Re: bug#50179: [PATCH v4] Add support for "bright" ANSI colors to
 ansi-color and term-mode
Date: Thu, 23 Sep 2021 14:21:23 -0700

On 9/23/2021 1:58 PM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs <at> gmail.com> writes:
> 
>> Ok, I've done this here, and all the other places as well. I also
>> obsoleted the old `ansi-color-map' and friends instead of deleting
>> them for the same reason. Hopefully all that's right; I tried to make
>> sure that they warn the user if they use them, but that warnings don't
>> show up when building Emacs.
> 
> Thanks; looks good to me.  There was a typo in the man.el part of the
> patch, but I think I fixed it -- can you have a look and see whether I
> did the correct thing?

That looks correct to me. Thanks for the fix! (I switched back and forth 
between implementations there a few times, and I guess when I made the 
patch I was midway between changing it back.)

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.