From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames Date: Mon, 12 Feb 2024 14:48:49 +0200 Message-ID: <86a5o5sv9q.fsf@gnu.org> References: <20240211180501.695192-1-mohkale@kisara.moe> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="24157"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 62994@debbugs.gnu.org To: mohkale@kisara.moe Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Feb 12 13:50:09 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rZVlA-00060U-Uy for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 12 Feb 2024 13:50:09 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rZVkt-0004eT-U8; Mon, 12 Feb 2024 07:49:51 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rZVkn-0004ck-Qi for bug-gnu-emacs@gnu.org; Mon, 12 Feb 2024 07:49:45 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rZVkn-0000V4-FD for bug-gnu-emacs@gnu.org; Mon, 12 Feb 2024 07:49:45 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rZVl3-0002S9-RF for bug-gnu-emacs@gnu.org; Mon, 12 Feb 2024 07:50:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 12 Feb 2024 12:50:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 62994 X-GNU-PR-Package: emacs Original-Received: via spool by 62994-submit@debbugs.gnu.org id=B62994.17077421599225 (code B ref 62994); Mon, 12 Feb 2024 12:50:01 +0000 Original-Received: (at 62994) by debbugs.gnu.org; 12 Feb 2024 12:49:19 +0000 Original-Received: from localhost ([127.0.0.1]:54726 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rZVkM-0002Oa-55 for submit@debbugs.gnu.org; Mon, 12 Feb 2024 07:49:19 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:33872) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rZVkJ-0002NR-0Z for 62994@debbugs.gnu.org; Mon, 12 Feb 2024 07:49:16 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rZVjw-0000NR-ID; Mon, 12 Feb 2024 07:48:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=YlrGzxzIq8mYnnXChmXSFaXD4KUU9SVAIiONugrJKks=; b=ixex278BcEzc i8s19p5m1pe2NVqllYULzusPikDPq0TWX8q6agPgru+Yr8b6YykmMLbIrjsojOGTJrT6u62PqTJqP ihANTYpk2FM7A7f+yKZSo1FcEXBu8WnvhqO09uLSPngX5gOrzKj/ffwvNrN7yKOk6xPCTdFEMHi4y ZtkvrUvszZKHW1wcs+SES5alqQJyq0fs2RdAsZQPEU9G2FPtdyAqWmkP+i+JetCloHCOR5cUfCDM0 9hm9x8l1wy4vx9vTQJflS4dUSoMYobPc6o87L84C0XwnkBen5CdekxTVWG5iPOvcKGdhuifeLavuw rUzSlY6fryVhzX7cb1WwsQ==; In-Reply-To: <20240211180501.695192-1-mohkale@kisara.moe> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:279890 Archived-At: > From: mohkale@kisara.moe > Cc: Mohsin Kaleem > Date: Sun, 11 Feb 2024 18:05:01 +0000 > > From: Mohsin Kaleem > > * 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?