bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames

[Eli Zaretskii]

> From: mohkale@kisara.moe
> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Sun, 11 Feb 2024 18:05:01 +0000
> 
> From: Mohsin Kaleem <mohkale@kisara.moe>
> 
> * src/dispextern.h (face, face_underline_type, syms_of_xfacse,
> internal-set-lisp-face-attribute, gui_supports_face_attributes_p):
> Add definitions for new underline styles of Double, Dotted and Dashed.
> Delete tty_underline_p from the face struct and use just underline going
> forward.  Add a flag to check whether styled underlines are available.
> * lisp/cus-face.el (custom-face-attributes): Add entries for Double,
> Dotted and Dashed so they can be set through `customize'.
> * src/termchar.c (tty_display_info): Add an entry for the escape
> sequence to set the underline style and color on terminal frames.
> * src/term.c (init_tty, tty_capable_p, turn_on_face): Read and save the
> underline style escape sequence from the Smulx termcap (alternatively if
> the Su flag is set use a default sequence).  Allow checking for support
> of styled underlines in the current terminal frame.  Output the necessary
> escape sequences to activate a styled underline on turn_on_face; this is
> currently only used for the new special underline styles, a default
> straight underline will still use the "us" termcap.  Output escape
> sequence to set underline color when set in the face and supported by
> the tty.  Save a default value for this sequence on init_tty when styled
> underlines are supported.
> * src/xfaces.c (tty_supports_face_attributes_p, realize_tty_face,
> map_tty_color, map_tty_color2): Assert whether styled underlines are
> supported by the current terminal on
> display-supports-face-attributes-p checks.  Populate the correct
> underline style and color in the face spec when realizing a face.
> Allow map_tty_color to map underline colors alongside foreground and
> background.  The interface of map_tty_color was amended to allow
> the caller to supply the underline color instead of accessing it
> through the face attributes.  A new variant map_tty_color2 was added
> for contexts where caller doesn't care about foreground/background
> face defaulting.
> ---
>  etc/NEWS         |  15 +++++
>  lisp/cus-face.el |   5 +-
>  src/dispextern.h |  10 ++-
>  src/term.c       |  54 +++++++++++++--
>  src/termchar.h   |   7 ++
>  src/xfaces.c     | 171 +++++++++++++++++++++++++++++++++++++++--------
>  6 files changed, 227 insertions(+), 35 deletions(-)

Thanks.  I think in addition to NEWS, we'd need to update the ELisp
Reference manual, because the new underline styles are not currently
mentioned there.

> --- a/src/term.c
> +++ b/src/term.c
> @@ -2014,8 +2014,19 @@ turn_on_face (struct frame *f, int face_id)
>       OUTPUT1 (tty, tty->TS_enter_dim_mode);
>      }
>  
> -  if (face->tty_underline_p && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE))
> -    OUTPUT1_IF (tty, tty->TS_enter_underline_mode);
> +  if (face->underline && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE))
> +     {
> +     if (face->underline == FACE_UNDER_LINE
> +             || !tty->TF_set_underline_style)
> +             OUTPUT1_IF (tty, tty->TS_enter_underline_mode);
> +     else if (tty->TF_set_underline_style)
> +       {
> +             char *p;

Here and elsewhere in the patch, you use indentation style slightly
different from ours, so please reindent to follow our style (which
uses TABs and SPACEs, not just TABs).

> +             p = tparam(tty->TF_set_underline_style, NULL, 0, 
> face->underline, 0, 0, 0);
                         ^^
Our style is to leave a single SPACE between the name of a function
and the opening parenthesis.  Several places in the patch don't leave
that SPACE.

> +  /* Styled underlines.       Support for this is provided either by the
                          ^^^^^^^
Please don't use TABs inside comments, except as indentation.

> +     if (!tty->TF_set_underline_style && tgetflag("Su"))
> +             /* Default to the kitty escape sequence.  See
> +                https://sw.kovidgoyal.net/kitty/underlines/ */
                                                             ^^
This should be a period.  Also, our style is to leave two SPACEs after
the final sentence of a comment, before the "*/" comment delimiter.

> +                     return false; /* Unsupported underline style */
                                                                    ^
Period and one more SPACE are missing there.

> +     if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline)
> +               || EQ (CAR_SAFE (CDR_SAFE (val)), Qdouble)
> +               || EQ (CAR_SAFE (CDR_SAFE (val)), Qwave)
> +               || EQ (CAR_SAFE (CDR_SAFE (val)), Qdotted)
> +               || EQ (CAR_SAFE (CDR_SAFE (val)), Qdashed)))
> +       {
> +             return false; /* Face uses an unsupported underline style.      
> */
> +       }

Our style is not to use braces for single-statement blocks.

> +/* Map the specified color COLOR of face FACE on frame F to a tty
> +   color index.  IDX is one of LFACE_FOREGROUND_INDEX,
> +   LFACE_BACKGROUND_INDEX or LFACE_UNDERLINE_INDEX, and specifies
> +   which color to map.  Set *DEFAULTED to true if mapping to the
> +   default foreground/background colors. */
                                          ^^
One more SPACE there.

> -  if (foreground_p)
> -    face->foreground = pixel;
> -  else
> -    face->background = pixel;
> +  switch (idx)
> +     {
> +     case LFACE_FOREGROUND_INDEX:
> +       face->foreground = pixel;
> +       break;
> +     case LFACE_BACKGROUND_INDEX:
> +       face->background = pixel;
> +       break;
> +     case LFACE_UNDERLINE_INDEX:
> +       face->underline_color = pixel;
> +       break;
> +     default:
> +       emacs_abort ();

The original code didn't call emacs_abort, but instead simply used
PIXEL as the background color.  Why would we do something different
now?

> +static void
> +map_tty_color2 (struct frame *f, struct face *face, Lisp_Object color,
> +             enum lface_attribute_index idx)
> +{
> +     bool face_colors_defaulted = false;
> +     map_tty_color (f, face, color, idx, &face_colors_defaulted);
>  }

Is this function really justified? why not call map_tty_color?