bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames

[Eli Zaretskii]

> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> From: mohkale@kisara.moe
> Date: Fri, 21 Apr 2023 15:34:47 +0100
> 
> diff --git a/etc/NEWS b/etc/NEWS
> index 62d2fdcd3a4..9f34927dfad 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1916,6 +1916,22 @@ This command switches to the "*scratch*" buffer.  If 
> "*scratch*" doesn't
>  exist, the command creates it first.  You can use this command if you
>  inadvertently delete the "*scratch*" buffer.
>  
> +---
> +** Support for 'styled-underline' face attributes on TTY frames
> +If your terminals termcap or terminfo database entry has the 'Su' or
> +'Smulx' capability defined, Emacs will now emit the prescribed escape
> +sequence necessary to render faces with styled underlines on TTY
> +frames.
> +
> +Styled underlines are any underlines containing a non-default
> +underline style.  The available underline styles for TTY frames are
> +'double', 'wave', 'dotted', and 'dashed'.
> +
> +The 'Smulx' capability should define the actual sequence needed to
> +render styled underlines. If ommitted, but the 'Su' flag is defined,
> +then a default sequence will be used. It's recommended to use 'Smulx'
> +instead of 'Su', with priority being given to 'Smulx'.

I think the last paragraph is unnecessary in NEWS, since there's
nothing users can do about the capabilities exposed by their terminfo
database.  In any case, please observe our convention of leaving two
spaces between sentences, not one.

> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -1773,7 +1773,7 @@ #define FONT_TOO_HIGH(ft)                               
>                 \
>       string meaning the default color of the TTY.  */
>    bool_bf tty_bold_p : 1;
>    bool_bf tty_italic_p : 1;
> -  bool_bf tty_underline_p : 1;
> +  ENUM_BF (face_underline_type) tty_underline : 3;
>    bool_bf tty_reverse_p : 1;
>    bool_bf tty_strike_through_p : 1;

Is there any reason for separate tty_* face attribute bits? why cannot
we use the same bits on both TTY and GUI frames?

> --- a/src/term.c
> +++ b/src/term.c
> @@ -1948,8 +1948,17 @@ 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->tty_underline && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE)) {
> +    if (face->tty_underline == FACE_UNDER_LINE ||
> +        !tty->TF_set_underline_style) {
> +      OUTPUT1_IF (tty, tty->TS_enter_underline_mode);
> +    } else if (tty->TF_set_underline_style) {

Please use our style of braces and placing of operators in continued
lines.  The brace should be on their own lines, alone.

> +  // Styled underlines.
> +  //
> +  // Support for this is provided either by the escape sequence in
> +  // Smulx or the Su flag. The latter results in a common default
> +  // escape sequence and is not recommended.

Comment style again: please use the C style.

> +#ifdef TERMINFO
> +  tty->TF_set_underline_style = tigetstr("Smulx");
> +  if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
> +    tty->TF_set_underline_style = NULL;
> +#else
> +  tty->TF_set_underline_style = tgetstr("Smulx", address);
> +#endif
> +  if (!tty->TF_set_underline_style && tgetflag("Su"))
> +    tty->TF_set_underline_style = "\x1b[4:%p1%dm";

Any pointers to where this standard escape sequence is defined?  I'd
like to have that in a comment to this line.

> +      else if (EQ (CAR_SAFE (val), QCstyle))
> +    {
> +        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))) {

Continuation line style again.

> +            return false; /* Unsupported underline style */

Comments should be complete sentences, and should end in a period and
2 spaces.

> +  /* Text underline.  */
> +  underline = attrs[LFACE_UNDERLINE_INDEX];
> +  if (NILP (underline)) {
> +    face->tty_underline = FACE_NO_UNDERLINE;
> +  } else if (EQ (underline, Qt)) {
> +    face->tty_underline = FACE_UNDER_LINE;
> +  } else if (STRINGP (underline)) {
> +    face->tty_underline = FACE_UNDER_LINE;
> +  } else if (CONSP (underline)) {

Style of braces again.

> +    while (CONSP (underline)) {
> +      Lisp_Object keyword, value;

And here.

> +      if (EQ (keyword, QCstyle)) {

And here.