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

[Eli Zaretskii <eliz@gnu.org>]

> 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?