Package: emacs;
Reported by: Mohsin Kaleem <mohkale <at> kisara.moe>
Date: Fri, 21 Apr 2023 14:30:02 UTC
Severity: wishlist
Message #121 received at 62994 <at> debbugs.gnu.org (full text, mbox):
From: Eli Zaretskii <eliz <at> gnu.org> To: mohkale <at> kisara.moe Cc: 62994 <at> debbugs.gnu.org Subject: Re: [PATCH v5] Add support for colored and styled underlines on tty frames Date: Mon, 12 Feb 2024 14:48:49 +0200
> From: mohkale <at> kisara.moe
> Cc: Mohsin Kaleem <mohkale <at> kisara.moe>
> Date: Sun, 11 Feb 2024 18:05:01 +0000
>
> From: Mohsin Kaleem <mohkale <at> 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?
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.