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: Thu, 14 Mar 2024 12:20:17 +0200 Message-ID: <86v85p9kta.fsf@gnu.org> References: <20240211180501.695192-1-mohkale@kisara.moe> <86a5o5sv9q.fsf@gnu.org> <87o7bmez8s.fsf@kisara.moe> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="40258"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 62994@debbugs.gnu.org To: Mohsin Kaleem Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Mar 14 11:21:57 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 1rkiDk-000AEk-H4 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 14 Mar 2024 11:21:56 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rkiDI-0001LH-JI; Thu, 14 Mar 2024 06:21:28 -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 1rkiDG-0001L1-Ha for bug-gnu-emacs@gnu.org; Thu, 14 Mar 2024 06:21:26 -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 1rkiDG-0001Kp-AG for bug-gnu-emacs@gnu.org; Thu, 14 Mar 2024 06:21:26 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rkiDp-0003Bn-Sy for bug-gnu-emacs@gnu.org; Thu, 14 Mar 2024 06:22:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 14 Mar 2024 10:22: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.171041169512211 (code B ref 62994); Thu, 14 Mar 2024 10:22:01 +0000 Original-Received: (at 62994) by debbugs.gnu.org; 14 Mar 2024 10:21:35 +0000 Original-Received: from localhost ([127.0.0.1]:48396 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rkiDP-0003As-Bs for submit@debbugs.gnu.org; Thu, 14 Mar 2024 06:21:35 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:59778) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rkiDN-0003Ad-JV for 62994@debbugs.gnu.org; Thu, 14 Mar 2024 06:21:34 -0400 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 1rkiCh-0001EB-4E; Thu, 14 Mar 2024 06:20:52 -0400 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=NTiHfY9bLx2SjjSQppN3zHi0LTzb0RBNC0r45oYC4SM=; b=jm6lz/GWhn5M GbOirIugZIrwHM9xKhUglDdzZ0cdJ9b531QXhM4AA3SQsLt5mX7t1gQxnzbRoWZXizF2AgUpuIqYq f3sRU/7Jx/v0A6Z6llMFNoZG7FxQhxeCuPZh2AcOmowjqoTfvDrvSK1Xc0KAUd1pS6eBxIPOhe+ye pdYpVJRu74p83xzaLEflSCteLj1fAQwW9/uQMcemLYfrTG3sHprM+qZENij+zQ1GL78xLzVtBNyXL rkF0aWbZRUzJIzB8CmljlPKFwXWVWWQtOHqDl8TLdzBrRq7cahZ2kGv570/6vQ8Kj1t79z1CO0JHj rqdNDI20MixXXpjGvPpvWQ==; In-Reply-To: <87o7bmez8s.fsf@kisara.moe> (message from Mohsin Kaleem on Sun, 10 Mar 2024 18:08:19 +0000) 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:281605 Archived-At: > From: Mohsin Kaleem > Cc: 62994@debbugs.gnu.org > Date: Sun, 10 Mar 2024 18:08:19 +0000 > > 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? The documentation should say which displays support this. > >> - 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? > > 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. eassert are only active in debug builds with --enable-checking; they compile to nothing in production builds. > If preferred I can > fallthrough into the background statement to stay consistent? Yes, please. > >> +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? > > 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? How many callers does this function have? If just one or two, I don't think a separate function is worth our while. Thanks.