Subject: Re: bug#62994: [PATCH 1/3] Add face definitions for more underline styles

From: Eli Zaretskii <[email-masked]>
Date: Fri, 21 Apr 2023 18:58:39 +0300

> Cc: Mohsin Kaleem <[email-masked]>
> From: [email-masked]
> Date: Fri, 21 Apr 2023 15:34:46 +0100
> 
> From: Mohsin Kaleem <[email-masked]>
> 
> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -1653,9 +1653,13 @@ #define FONT_TOO_HIGH(ft)						\
>  
>  enum face_underline_type
>  {
> +  // Note: Order matches the order of the Smulx terminfo extension.

Please use the C style of comments, /* Like this. */, not the C++
style.

>    FACE_NO_UNDERLINE = 0,
>    FACE_UNDER_LINE,
> -  FACE_UNDER_WAVE
> +  FACE_DOUBLE_UNDER_LINE,
> +  FACE_UNDER_WAVE,
> +  FACE_DOTTED_UNDER_LINE,
> +  FACE_DASHED_UNDER_LINE,

Is it really necessary to change the numerical value of
FACE_UNDER_WAVE?  Can it be left at its original value?

> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -3255,7 +3255,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
>                  }
>  
>                else if (EQ (key, QCstyle)
> -                       && !(EQ (val, Qline) || EQ (val, Qwave)))
> +                       && !(EQ (val, Qline) ||
> +                            EQ (val, Qdouble) ||
> +                            EQ (val, Qwave) ||
> +                            EQ (val, Qdotted) ||
> +                            EQ (val, Qdashed)))

Our style is to put the operators ("||" in this case) at the beginning
of the line, not at its end.

> +  /* Check supported underline styles. */
> +  val = attrs[LFACE_UNDERLINE_INDEX];
> +  if (!UNSPECIFIEDP (val)) {
> +    if (EQ (CAR_SAFE (val), QCstyle)) {
> +      if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline) ||
> +            EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))) {
> +        return false; /* Unsupported underline style */

Likewise here.