GNU bug report logs - #62994
Support styled underlines on TTY frames

Previous Next

Full log

View this message in rfc822 format

From: Eli Zaretskii <eliz <at> gnu.org>
To: mohkale <at> kisara.moe
Cc: mohkale <at> kisara.moe, 62994 <at> debbugs.gnu.org
Subject: bug#62994: [PATCH v6] Add support for colored and styled underlines on tty frames
Date: Sat, 20 Apr 2024 11:16:44 +0300

> Cc: Mohsin Kaleem <mohkale <at> kisara.moe>
> From: mohkale <at> kisara.moe
> Date: Sun, 14 Apr 2024 14:56:32 +0100

The Subject line is longer than 78 characters, so our commit hooks
didn't allow me to apply this.  Please make the Subject line shorter,
possibly by dropping the bug number (mention the bug number somewhere
in the rest of the log message).

> * 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-line, Dots and
> Dashes.  Rename FACE_UNDER_LINE and FACE_UNDER_WAVE to make definitions
> consistent.  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-line,
> Dots and Dashes 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): 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.
> * src/xterm.c (x_draw_glyph_string): Updated to use renamed
> FACE_UNDERLINE_SINGLE and FACE_UNDERLINE_WAVE face_underline_type
> enumerations.

These lines are also too long, and don't follow our conventions.  I
suggest to use change-log-mode to format and fill those correctly.

> +  if (!tty->TF_set_underline_style && tgetflag ("Su"))
> +    /* Default to the kitty escape sequence.  See
> +       https://sw.kovidgoyal.net/kitty/underlines/ */

Period missing at the end of the comment.  Also, please leave two
spaces between the end of the comment and the closing "*/", per our
conventions.

> +  /* 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.  */

Can this be written in a cleaner way, as a single 'if' with all the
conditions combined together, instead of nesting them?

> -	return false;		/* same as default */
> +	return false;  /* same as default */

Why this whitespace change?

Thanks.

Previous Next

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