From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Mohsin Kaleem Newsgroups: gmane.emacs.bugs Subject: bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames Date: Sun, 10 Mar 2024 18:08:19 +0000 Message-ID: <87o7bmez8s.fsf@kisara.moe> References: <20240211180501.695192-1-mohkale@kisara.moe> <86a5o5sv9q.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="7641"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 62994@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Mar 10 19:08:47 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 1rjNbL-0001p6-63 for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 10 Mar 2024 19:08:47 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rjNb4-0000GI-5y; Sun, 10 Mar 2024 14:08:30 -0400 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 1rjNb3-0000GA-3D for bug-gnu-emacs@gnu.org; Sun, 10 Mar 2024 14:08:29 -0400 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 1rjNb2-0007pe-Pg for bug-gnu-emacs@gnu.org; Sun, 10 Mar 2024 14:08:28 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rjNbZ-0000e1-OE for bug-gnu-emacs@gnu.org; Sun, 10 Mar 2024 14:09:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Mohsin Kaleem Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 10 Mar 2024 18:09: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.17100941402470 (code B ref 62994); Sun, 10 Mar 2024 18:09:01 +0000 Original-Received: (at 62994) by debbugs.gnu.org; 10 Mar 2024 18:09:00 +0000 Original-Received: from localhost ([127.0.0.1]:37858 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rjNbU-0000dj-VK for submit@debbugs.gnu.org; Sun, 10 Mar 2024 14:09:00 -0400 Original-Received: from 119.ip-51-38-65.eu ([51.38.65.119]:56272 helo=mail.kisara.moe) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rjNbS-0000dZ-Ts for 62994@debbugs.gnu.org; Sun, 10 Mar 2024 14:08:55 -0400 Original-Received: from mk-desktop (05408574.skybroadband.com [5.64.133.116]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by mail.kisara.moe (Postfix) with ESMTPSA id 5627DA2796; Sun, 10 Mar 2024 19:08:20 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kisara.moe; s=default; t=1710094100; bh=H5gky1lhdYPp3GGbkxvrIoYf5ufQHSJs2WfsZRAMZt8=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=0IOkkiZCNHIqFNFru2LO/JZxewWC2nGGp/kHxA8+t0LWl9KyJZEwsRrxgDWn8xJz/ eDfoI/7HaIhpnY+NFKPaXaG4n3JP7UQtYyamRddnu6omRCl4oAXwwwpCKk6qac42Bf eqJkxT6s2DIrgRNAmh9wsjCic6nwNauoKeW6KHNHh5X+vK90tMt2P3pj8ewyuGd4XS zwot6UxtdVTx6ttGfep/HNpsPS4oxLQlkTX3ZLa/LhOqk1HVs8y3m/P8LQud8tLLEf beO2Rno3CzxRLeyhUsC68FMayRzKIsI+QJQJWDTat1sC7rsQ3CJUDOq8uGDXPkpySl Qwu5sKL8KTXiA== In-Reply-To: <86a5o5sv9q.fsf@gnu.org> 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:281424 Archived-At: Eli Zaretskii writes: > 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. Added. Should I mention display support between GUI and TTY frames or is it sufficient just to mention these are the options? > 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). I did just run M-x tabify over all the lines I changed but looks like that was a mistake. I've tried just manually re-adjusting all the changesets in the next patchset but still not fully clear if it adheres to the conventions wanted here =F0=9F=98=85. >> + p =3D 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. Did a quick grep for `[^ ](` and fixed all offending lines. > >> + /* Styled underlines. Support for this is provided either by the > ^^^^^^^ > Please don't use TABs inside comments, except as indentation. Seems to have been a byproduct of `M-x tabify`. Apologies. >> + return false; /* Unsupported underline style */ > ^ > Period and one more SPACE are missing there. Added. > >> + 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. Removed. > >> +/* 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. Added. > >> - if (foreground_p) >> - face->foreground =3D pixel; >> - else >> - face->background =3D pixel; >> + switch (idx) >> + { >> + case LFACE_FOREGROUND_INDEX: >> + face->foreground =3D pixel; >> + break; >> + case LFACE_BACKGROUND_INDEX: >> + face->background =3D pixel; >> + break; >> + case LFACE_UNDERLINE_INDEX: >> + face->underline_color =3D 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? Earlier in the function we call eassert on the index parameter. I might've misunderstood but I thought that check would terminate the function at that point so this line where we handle a index value outside the supported range should never be called. If preferred I can fallthrough into the background statement to stay consistent? > >> +static void >> +map_tty_color2 (struct frame *f, struct face *face, Lisp_Object color, >> + enum lface_attribute_index idx) >> +{ >> + bool face_colors_defaulted =3D false; >> + map_tty_color (f, face, color, idx, &face_colors_defaulted); >> } > > Is this function really justified? why not call map_tty_color? The extra parameter face_colors_defaulted doesn't really make sense for anything but foreground/background color calls. Using the existing one would make the callsite for the underline color set more noisy with an extra output param that we then just ignore. I thought this was a slightly nicer solution from the caller side. If preferred I can remove and go the alternate route? --=20 Mohsin Kaleem