all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames
       [not found] <handler.62994.B.168208734930664.ack@debbugs.gnu.org>
@ 2023-04-21 14:34 ` mohkale
  2023-04-21 14:34   ` bug#62994: [PATCH 1/3] Add face definitions for more underline styles mohkale
                     ` (3 more replies)
  2023-04-21 19:24 ` bug#62994: [PATCH v2 0/1] " mohkale
                   ` (3 subsequent siblings)
  4 siblings, 4 replies; 35+ messages in thread
From: mohkale @ 2023-04-21 14:34 UTC (permalink / raw)
  To: 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@kisara.moe>

Modern terminals (such as kitty) support setting the style of an
underline with the escape sequence exposed in the Smulx termcap.
This allows for (among others) wavy underlines on terminals.
These terminals also support setting the color of these underlines
using a separate escape sequence that to the best of my knowledge
is not exposed as a termcap but has been adopted by other terminal
supporting editors like neovim.

Mohsin Kaleem (3):
  Add face definitions for more underline styles
  Add support for styled underlines on tty frames
  Add support for colored underlines on tty frames

 etc/NEWS         | 19 ++++++++++
 lisp/cus-face.el |  5 ++-
 src/dispextern.h | 11 ++++--
 src/term.c       | 46 +++++++++++++++++++++---
 src/termchar.h   |  7 ++++
 src/xfaces.c     | 93 ++++++++++++++++++++++++++++++++++++++++++++----
 6 files changed, 167 insertions(+), 14 deletions(-)

-- 
2.40.0






^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 1/3] Add face definitions for more underline styles
  2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
@ 2023-04-21 14:34   ` mohkale
  2023-04-21 15:58     ` Eli Zaretskii
  2023-04-21 14:34   ` bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames mohkale
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 35+ messages in thread
From: mohkale @ 2023-04-21 14:34 UTC (permalink / raw)
  To: 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@kisara.moe>

* src/dispextern.h (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.
* lisp/cus-face.el (custom-face-attributes): Add entries for Double,
Dotted and Dashed so they can be set through `customize'.
---
 lisp/cus-face.el |  5 ++++-
 src/dispextern.h |  8 ++++++--
 src/xfaces.c     | 21 ++++++++++++++++++++-
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index ec89b4f7ff6..2d6e6c7b73e 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -141,7 +141,10 @@ custom-face-attributes
 		   (const :format "" :value :style)
 		   (choice :tag "Style"
 			   (const :tag "Line" line)
-			   (const :tag "Wave" wave))
+			   (const :tag "Double" double)
+			   (const :tag "Wave" wave)
+			   (const :tag "Dotted" dotted)
+			   (const :tag "Dashed" dashed))
                    (const :format "" :value :position)
                    (choice :tag "Position"
                            (const :tag "At Default Position" nil)
diff --git a/src/dispextern.h b/src/dispextern.h
index 4dcab113ea2..1dc84e32efc 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1653,9 +1653,13 @@ #define FONT_TOO_HIGH(ft)						\
 
 enum face_underline_type
 {
+  // Note: Order matches the order of the Smulx terminfo extension.
   FACE_NO_UNDERLINE = 0,
   FACE_UNDER_LINE,
-  FACE_UNDER_WAVE
+  FACE_DOUBLE_UNDER_LINE,
+  FACE_UNDER_WAVE,
+  FACE_DOTTED_UNDER_LINE,
+  FACE_DASHED_UNDER_LINE,
 };
 
 /* Structure describing a realized face.
@@ -1737,7 +1741,7 @@ #define FONT_TOO_HIGH(ft)						\
   ENUM_BF (face_box_type) box : 2;
 
   /* Style of underlining. */
-  ENUM_BF (face_underline_type) underline : 2;
+  ENUM_BF (face_underline_type) underline : 3;
 
   /* If `box' above specifies a 3D type, true means use box_color for
      drawing shadows.  */
diff --git a/src/xfaces.c b/src/xfaces.c
index 37b703984be..cfbb89d2ae2 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3255,7 +3255,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
                 }
 
               else if (EQ (key, QCstyle)
-                       && !(EQ (val, Qline) || EQ (val, Qwave)))
+                       && !(EQ (val, Qline) ||
+                            EQ (val, Qdouble) ||
+                            EQ (val, Qwave) ||
+                            EQ (val, Qdotted) ||
+                            EQ (val, Qdashed)))
                 {
                   valid_p = false;
                   break;
@@ -5204,6 +5208,7 @@ gui_supports_face_attributes_p (struct frame *f,
                                 Lisp_Object attrs[LFACE_VECTOR_SIZE],
                                 struct face *def_face)
 {
+  Lisp_Object val;
   Lisp_Object *def_attrs = def_face->lface;
   Lisp_Object lattrs[LFACE_VECTOR_SIZE];
 
@@ -5298,6 +5303,17 @@ gui_supports_face_attributes_p (struct frame *f,
       return false;
     }
 
+  /* 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 */
+      }
+    }
+  }
+
   /* Everything checks out, this face is supported.  */
   return true;
 }
@@ -7165,6 +7181,9 @@ syms_of_xfaces (void)
   DEFSYM (QCposition, ":position");
   DEFSYM (Qline, "line");
   DEFSYM (Qwave, "wave");
+  DEFSYM (Qdouble, "double");
+  DEFSYM (Qdotted, "dotted");
+  DEFSYM (Qdashed, "dashed");
   DEFSYM (Qreleased_button, "released-button");
   DEFSYM (Qpressed_button, "pressed-button");
   DEFSYM (Qflat_button, "flat-button");
-- 
2.40.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
  2023-04-21 14:34   ` bug#62994: [PATCH 1/3] Add face definitions for more underline styles mohkale
@ 2023-04-21 14:34   ` mohkale
  2023-04-21 16:07     ` Eli Zaretskii
  2023-04-21 14:34   ` bug#62994: [PATCH 3/3] Add support for colored " mohkale
  2023-04-21 15:52   ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames Eli Zaretskii
  3 siblings, 1 reply; 35+ messages in thread
From: mohkale @ 2023-04-21 14:34 UTC (permalink / raw)
  To: 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@kisara.moe>

* src/dispextern.h (face): Convert tty_underline_p from bool to a
face_underline_type enumeration. Add a flag to check whether styled
underlines are available.
* src/termchar.c (tty_display_info): Add an entry for the escape
sequence to set the underline style 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.
* src/xfaces.c (tty_supports_face_attributes_p, realize_tty_face):
Assert whether styled underlines are supported by the current terminal
on display-supports-face-attributes-p checks. Populate the correct
underline style in the face spec when realizing a face.
---
 etc/NEWS         | 16 +++++++++++++
 src/dispextern.h |  3 ++-
 src/term.c       | 35 ++++++++++++++++++++++++----
 src/termchar.h   |  4 ++++
 src/xfaces.c     | 60 ++++++++++++++++++++++++++++++++++++++++++++----
 5 files changed, 109 insertions(+), 9 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 62d2fdcd3a4..9f34927dfad 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1916,6 +1916,22 @@ This command switches to the "*scratch*" buffer.  If "*scratch*" doesn't
 exist, the command creates it first.  You can use this command if you
 inadvertently delete the "*scratch*" buffer.
 
+---
+** Support for 'styled-underline' face attributes on TTY frames
+If your terminals termcap or terminfo database entry has the 'Su' or
+'Smulx' capability defined, Emacs will now emit the prescribed escape
+sequence necessary to render faces with styled underlines on TTY
+frames.
+
+Styled underlines are any underlines containing a non-default
+underline style.  The available underline styles for TTY frames are
+'double', 'wave', 'dotted', and 'dashed'.
+
+The 'Smulx' capability should define the actual sequence needed to
+render styled underlines. If ommitted, but the 'Su' flag is defined,
+then a default sequence will be used. It's recommended to use 'Smulx'
+instead of 'Su', with priority being given to 'Smulx'.
+
 ** Debugging
 
 ---
diff --git a/src/dispextern.h b/src/dispextern.h
index 1dc84e32efc..6ea6f6170e4 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1773,7 +1773,7 @@ #define FONT_TOO_HIGH(ft)						\
      string meaning the default color of the TTY.  */
   bool_bf tty_bold_p : 1;
   bool_bf tty_italic_p : 1;
-  bool_bf tty_underline_p : 1;
+  ENUM_BF (face_underline_type) tty_underline : 3;
   bool_bf tty_reverse_p : 1;
   bool_bf tty_strike_through_p : 1;
 
@@ -3365,6 +3365,7 @@ #define TTY_CAP_BOLD		0x04
 #define TTY_CAP_DIM		0x08
 #define TTY_CAP_ITALIC  	0x10
 #define TTY_CAP_STRIKE_THROUGH	0x20
+#define TTY_CAP_UNDERLINE_STYLED	0x32 & TTY_CAP_UNDERLINE
 
 \f
 /***********************************************************************
diff --git a/src/term.c b/src/term.c
index 53ba2a231e4..0f0393780eb 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1948,8 +1948,17 @@ 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->tty_underline && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE)) {
+    if (face->tty_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;
+      p = tparam(tty->TF_set_underline_style, NULL, 0, face->tty_underline, 0, 0, 0);
+      OUTPUT (tty, p);
+      xfree (p);
+    }
+  }
 
   if (face->tty_strike_through_p
       && MAY_USE_WITH_COLORS_P (tty, NC_STRIKE_THROUGH))
@@ -1995,7 +2004,7 @@ turn_off_face (struct frame *f, int face_id)
       if (face->tty_bold_p
 	  || face->tty_italic_p
 	  || face->tty_reverse_p
-	  || face->tty_underline_p
+	  || face->tty_underline
 	  || face->tty_strike_through_p)
 	{
 	  OUTPUT1_IF (tty, tty->TS_exit_attribute_mode);
@@ -2007,7 +2016,7 @@ turn_off_face (struct frame *f, int face_id)
     {
       /* If we don't have "me" we can only have those appearances
 	 that have exit sequences defined.  */
-      if (face->tty_underline_p)
+      if (face->tty_underline)
 	OUTPUT_IF (tty, tty->TS_exit_underline_mode);
     }
 
@@ -2036,6 +2045,9 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_UNDERLINE,	  tty->TS_enter_underline_mode,
 		     NC_UNDERLINE);
+  TTY_CAPABLE_P_TRY (tty,
+		     TTY_CAP_UNDERLINE_STYLED,	  tty->TF_set_underline_style,
+             NC_UNDERLINE);
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_BOLD,	  tty->TS_enter_bold_mode, NC_BOLD);
   TTY_CAPABLE_P_TRY (tty,
@@ -4250,6 +4262,21 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   tty->TF_underscore = tgetflag ("ul");
   tty->TF_teleray = tgetflag ("xt");
 
+  // Styled underlines.
+  //
+  // Support for this is provided either by the escape sequence in
+  // Smulx or the Su flag. The latter results in a common default
+  // escape sequence and is not recommended.
+#ifdef TERMINFO
+  tty->TF_set_underline_style = tigetstr("Smulx");
+  if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
+    tty->TF_set_underline_style = NULL;
+#else
+  tty->TF_set_underline_style = tgetstr("Smulx", address);
+#endif
+  if (!tty->TF_set_underline_style && tgetflag("Su"))
+    tty->TF_set_underline_style = "\x1b[4:%p1%dm";
+
 #else /* DOS_NT */
 #ifdef WINDOWSNT
   {
diff --git a/src/termchar.h b/src/termchar.h
index 5c47679a994..319c2319fba 100644
--- a/src/termchar.h
+++ b/src/termchar.h
@@ -171,6 +171,10 @@ #define EMACS_TERMCHAR_H
                                    non-blank position.  Must clear before writing _.  */
   int TF_teleray;               /* termcap xt flag: many weird consequences.
                                    For t1061. */
+  const char *TF_set_underline_style;   /* termcap Smulx entry: Switches the underline
+                                           style based on the parameter. Param should
+                                           be one of: 0 (none), 1 (straight), 2 (double),
+                                           3 (wave), 4 (dotted), or 5 (dashed). */
 
   int RPov;                     /* # chars to start a TS_repeat */
 
diff --git a/src/xfaces.c b/src/xfaces.c
index cfbb89d2ae2..2c6c554d01d 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5408,8 +5408,18 @@ tty_supports_face_attributes_p (struct frame *f,
     {
       if (STRINGP (val))
 	return false;		/* ttys can't use colored underlines */
-      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
-	return false;		/* ttys can't use wave underlines */
+      else if (EQ (CAR_SAFE (val), QCstyle))
+    {
+        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; /* Unsupported underline style */
+        }
+
+	    test_caps |= TTY_CAP_UNDERLINE_STYLED;
+    }
       else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
 	return false;		/* same as default */
       else
@@ -6335,6 +6345,8 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
                 face->underline = FACE_UNDER_LINE;
               else if (EQ (value, Qwave))
                 face->underline = FACE_UNDER_WAVE;
+              else
+                face->underline = FACE_UNDER_LINE;
             }
 	  else if (EQ (keyword, QCposition))
 	    {
@@ -6469,6 +6481,7 @@ realize_tty_face (struct face_cache *cache,
 {
   struct face *face;
   int weight, slant;
+  Lisp_Object underline;
   bool face_colors_defaulted = false;
   struct frame *f = cache->f;
 
@@ -6488,13 +6501,52 @@ realize_tty_face (struct face_cache *cache,
     face->tty_bold_p = true;
   if (slant != 100)
     face->tty_italic_p = true;
-  if (!NILP (attrs[LFACE_UNDERLINE_INDEX]))
-    face->tty_underline_p = true;
   if (!NILP (attrs[LFACE_INVERSE_INDEX]))
     face->tty_reverse_p = true;
   if (!NILP (attrs[LFACE_STRIKE_THROUGH_INDEX]))
     face->tty_strike_through_p = true;
 
+  /* Text underline.  */
+  underline = attrs[LFACE_UNDERLINE_INDEX];
+  if (NILP (underline)) {
+    face->tty_underline = FACE_NO_UNDERLINE;
+  } else if (EQ (underline, Qt)) {
+    face->tty_underline = FACE_UNDER_LINE;
+  } else if (STRINGP (underline)) {
+    face->tty_underline = FACE_UNDER_LINE;
+  } else if (CONSP (underline)) {
+    /* `(:style STYLE)'.
+       STYLE being one of `line', `double', `wave', `dotted' or `dashed'. */
+    face->tty_underline = FACE_UNDER_LINE;
+
+    while (CONSP (underline)) {
+      Lisp_Object keyword, value;
+
+      keyword = XCAR (underline);
+      underline = XCDR (underline);
+
+      if (!CONSP (underline))
+        break;
+      value = XCAR (underline);
+      underline = XCDR (underline);
+
+      if (EQ (keyword, QCstyle)) {
+        if (EQ (value, Qline))
+          face->tty_underline = FACE_UNDER_LINE;
+        else if (EQ (value, Qdouble))
+          face->tty_underline = FACE_DOUBLE_UNDER_LINE;
+        else if (EQ (value, Qwave))
+          face->tty_underline = FACE_UNDER_WAVE;
+        else if (EQ (value, Qdotted))
+          face->tty_underline = FACE_DOTTED_UNDER_LINE;
+        else if (EQ (value, Qdashed))
+          face->tty_underline = FACE_DASHED_UNDER_LINE;
+        else
+          face->tty_underline = FACE_UNDER_LINE;
+      }
+    }
+  }
+
   /* Map color names to color indices.  */
   map_tty_color (f, face, LFACE_FOREGROUND_INDEX, &face_colors_defaulted);
   map_tty_color (f, face, LFACE_BACKGROUND_INDEX, &face_colors_defaulted);
-- 
2.40.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 3/3] Add support for colored underlines on tty frames
  2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
  2023-04-21 14:34   ` bug#62994: [PATCH 1/3] Add face definitions for more underline styles mohkale
  2023-04-21 14:34   ` bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames mohkale
@ 2023-04-21 14:34   ` mohkale
  2023-04-21 16:12     ` Eli Zaretskii
  2023-04-21 15:52   ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames Eli Zaretskii
  3 siblings, 1 reply; 35+ messages in thread
From: mohkale @ 2023-04-21 14:34 UTC (permalink / raw)
  To: 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@kisara.moe>

* src/term.c (turn_on_face, init_tty): 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/termchar.c (tty_display_info): Add an entry for the escape
sequence to set the underline color on terminal frames.
* src/xfaces.c (tty_supports_face_attributes_p, realize_tty_face):
Assert whether colored underlines are supported by the current terminal
on display-supports-face-attributes-p checks. Load and save the color
of the underline from the face spec when realizing a face.
---
 etc/NEWS       | 11 +++++++----
 src/term.c     | 11 +++++++++++
 src/termchar.h |  3 +++
 src/xfaces.c   | 16 +++++++++++++---
 4 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 9f34927dfad..46b2b0e25c5 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1917,20 +1917,23 @@ exist, the command creates it first.  You can use this command if you
 inadvertently delete the "*scratch*" buffer.
 
 ---
-** Support for 'styled-underline' face attributes on TTY frames
+** Support for 'styled' and 'colored' underline face attributes on TTY frames
 If your terminals termcap or terminfo database entry has the 'Su' or
 'Smulx' capability defined, Emacs will now emit the prescribed escape
 sequence necessary to render faces with styled underlines on TTY
 frames.
 
 Styled underlines are any underlines containing a non-default
-underline style.  The available underline styles for TTY frames are
-'double', 'wave', 'dotted', and 'dashed'.
+underline style or a color other than the foreground-color.
+The available underline styles for TTY frames are 'double', 'wave',
+'dotted', and 'dashed'.
 
 The 'Smulx' capability should define the actual sequence needed to
 render styled underlines. If ommitted, but the 'Su' flag is defined,
 then a default sequence will be used. It's recommended to use 'Smulx'
-instead of 'Su', with priority being given to 'Smulx'.
+instead of 'Su', with priority being given to 'Smulx'.  Support for
+colored underlines is automatically enabled with a default escape
+sequence when styled underline are supported.
 
 ** Debugging
 
diff --git a/src/term.c b/src/term.c
index 0f0393780eb..a1eb1961fad 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1984,6 +1984,14 @@ turn_on_face (struct frame *f, int face_id)
 	  OUTPUT (tty, p);
 	  xfree (p);
 	}
+
+      ts = tty->TF_set_underline_color;
+      if (ts && face->underline_color)
+	{
+          p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);
+	  OUTPUT (tty, p);
+	  xfree (p);
+	}
     }
 }
 
@@ -4277,6 +4285,9 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   if (!tty->TF_set_underline_style && tgetflag("Su"))
     tty->TF_set_underline_style = "\x1b[4:%p1%dm";
 
+  if (tty->TF_set_underline_style)
+    tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";
+
 #else /* DOS_NT */
 #ifdef WINDOWSNT
   {
diff --git a/src/termchar.h b/src/termchar.h
index 319c2319fba..563cde715ec 100644
--- a/src/termchar.h
+++ b/src/termchar.h
@@ -175,6 +175,9 @@ #define EMACS_TERMCHAR_H
                                            style based on the parameter. Param should
                                            be one of: 0 (none), 1 (straight), 2 (double),
                                            3 (wave), 4 (dotted), or 5 (dashed). */
+  const char *TF_set_underline_color;   /* Enabled when TF_set_underline_style is set:
+                                           Sets the color of the underline. Accepts a
+                                           single parameter, the color index. */
 
   int RPov;                     /* # chars to start a TS_repeat */
 
diff --git a/src/xfaces.c b/src/xfaces.c
index 2c6c554d01d..c547a0b92f3 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -5407,7 +5407,7 @@ tty_supports_face_attributes_p (struct frame *f,
   if (!UNSPECIFIEDP (val))
     {
       if (STRINGP (val))
-	return false;		/* ttys can't use colored underlines */
+	test_caps |= TTY_CAP_UNDERLINE_STYLED;
       else if (EQ (CAR_SAFE (val), QCstyle))
     {
         if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline) ||
@@ -6510,14 +6510,18 @@ realize_tty_face (struct face_cache *cache,
   underline = attrs[LFACE_UNDERLINE_INDEX];
   if (NILP (underline)) {
     face->tty_underline = FACE_NO_UNDERLINE;
+    face->underline_color = 0;
   } else if (EQ (underline, Qt)) {
     face->tty_underline = FACE_UNDER_LINE;
+    face->underline_color = 0;
   } else if (STRINGP (underline)) {
     face->tty_underline = FACE_UNDER_LINE;
+	face->underline_color = load_color (f, face, underline, LFACE_UNDERLINE_INDEX);
   } else if (CONSP (underline)) {
-    /* `(:style STYLE)'.
+    /* `(:color COLOR :style STYLE)'.
        STYLE being one of `line', `double', `wave', `dotted' or `dashed'. */
     face->tty_underline = FACE_UNDER_LINE;
+    face->underline_color = 0;
 
     while (CONSP (underline)) {
       Lisp_Object keyword, value;
@@ -6530,7 +6534,13 @@ realize_tty_face (struct face_cache *cache,
       value = XCAR (underline);
       underline = XCDR (underline);
 
-      if (EQ (keyword, QCstyle)) {
+      if (EQ (keyword, QCcolor)) {
+        if (EQ (value, Qforeground_color))
+          face->underline_color = 0;
+        else if (STRINGP (value))
+          face->underline_color = load_color (f, face, value, LFACE_UNDERLINE_INDEX);
+      }
+      else if (EQ (keyword, QCstyle)) {
         if (EQ (value, Qline))
           face->tty_underline = FACE_UNDER_LINE;
         else if (EQ (value, Qdouble))
-- 
2.40.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames
  2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
                     ` (2 preceding siblings ...)
  2023-04-21 14:34   ` bug#62994: [PATCH 3/3] Add support for colored " mohkale
@ 2023-04-21 15:52   ` Eli Zaretskii
  2023-04-21 16:10     ` Mohsin Kaleem
  3 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-21 15:52 UTC (permalink / raw)
  To: mohkale; +Cc: 62994

> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> From: mohkale@kisara.moe
> Date: Fri, 21 Apr 2023 15:34:45 +0100
> 
> From: Mohsin Kaleem <mohkale@kisara.moe>
> 
> Modern terminals (such as kitty) support setting the style of an
> underline with the escape sequence exposed in the Smulx termcap.
> This allows for (among others) wavy underlines on terminals.
> These terminals also support setting the color of these underlines
> using a separate escape sequence that to the best of my knowledge
> is not exposed as a termcap but has been adopted by other terminal
> supporting editors like neovim.

Thanks.

Please note up front that we must wait for the completion of your
legal paperwork before we can accept significant contributions (such
as this one) from you.

I will send a few comments to each of the patches separately.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 1/3] Add face definitions for more underline styles
  2023-04-21 14:34   ` bug#62994: [PATCH 1/3] Add face definitions for more underline styles mohkale
@ 2023-04-21 15:58     ` Eli Zaretskii
  2023-04-21 16:08       ` Mohsin Kaleem
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-21 15:58 UTC (permalink / raw)
  To: mohkale; +Cc: 62994

> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> From: mohkale@kisara.moe
> Date: Fri, 21 Apr 2023 15:34:46 +0100
> 
> From: Mohsin Kaleem <mohkale@kisara.moe>
> 
> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -1653,9 +1653,13 @@ #define FONT_TOO_HIGH(ft)						\
>  
>  enum face_underline_type
>  {
> +  // Note: Order matches the order of the Smulx terminfo extension.

Please use the C style of comments, /* Like this. */, not the C++
style.

>    FACE_NO_UNDERLINE = 0,
>    FACE_UNDER_LINE,
> -  FACE_UNDER_WAVE
> +  FACE_DOUBLE_UNDER_LINE,
> +  FACE_UNDER_WAVE,
> +  FACE_DOTTED_UNDER_LINE,
> +  FACE_DASHED_UNDER_LINE,

Is it really necessary to change the numerical value of
FACE_UNDER_WAVE?  Can it be left at its original value?

> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -3255,7 +3255,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
>                  }
>  
>                else if (EQ (key, QCstyle)
> -                       && !(EQ (val, Qline) || EQ (val, Qwave)))
> +                       && !(EQ (val, Qline) ||
> +                            EQ (val, Qdouble) ||
> +                            EQ (val, Qwave) ||
> +                            EQ (val, Qdotted) ||
> +                            EQ (val, Qdashed)))

Our style is to put the operators ("||" in this case) at the beginning
of the line, not at its end.

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

Likewise here.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-21 14:34   ` bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames mohkale
@ 2023-04-21 16:07     ` Eli Zaretskii
       [not found]       ` <878rel5fqr.fsf@kisara.moe>
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-21 16:07 UTC (permalink / raw)
  To: mohkale; +Cc: 62994

> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> From: mohkale@kisara.moe
> Date: Fri, 21 Apr 2023 15:34:47 +0100
> 
> diff --git a/etc/NEWS b/etc/NEWS
> index 62d2fdcd3a4..9f34927dfad 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1916,6 +1916,22 @@ This command switches to the "*scratch*" buffer.  If "*scratch*" doesn't
>  exist, the command creates it first.  You can use this command if you
>  inadvertently delete the "*scratch*" buffer.
>  
> +---
> +** Support for 'styled-underline' face attributes on TTY frames
> +If your terminals termcap or terminfo database entry has the 'Su' or
> +'Smulx' capability defined, Emacs will now emit the prescribed escape
> +sequence necessary to render faces with styled underlines on TTY
> +frames.
> +
> +Styled underlines are any underlines containing a non-default
> +underline style.  The available underline styles for TTY frames are
> +'double', 'wave', 'dotted', and 'dashed'.
> +
> +The 'Smulx' capability should define the actual sequence needed to
> +render styled underlines. If ommitted, but the 'Su' flag is defined,
> +then a default sequence will be used. It's recommended to use 'Smulx'
> +instead of 'Su', with priority being given to 'Smulx'.

I think the last paragraph is unnecessary in NEWS, since there's
nothing users can do about the capabilities exposed by their terminfo
database.  In any case, please observe our convention of leaving two
spaces between sentences, not one.

> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -1773,7 +1773,7 @@ #define FONT_TOO_HIGH(ft)						\
>       string meaning the default color of the TTY.  */
>    bool_bf tty_bold_p : 1;
>    bool_bf tty_italic_p : 1;
> -  bool_bf tty_underline_p : 1;
> +  ENUM_BF (face_underline_type) tty_underline : 3;
>    bool_bf tty_reverse_p : 1;
>    bool_bf tty_strike_through_p : 1;

Is there any reason for separate tty_* face attribute bits? why cannot
we use the same bits on both TTY and GUI frames?

> --- a/src/term.c
> +++ b/src/term.c
> @@ -1948,8 +1948,17 @@ 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->tty_underline && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE)) {
> +    if (face->tty_underline == FACE_UNDER_LINE ||
> +        !tty->TF_set_underline_style) {
> +      OUTPUT1_IF (tty, tty->TS_enter_underline_mode);
> +    } else if (tty->TF_set_underline_style) {

Please use our style of braces and placing of operators in continued
lines.  The brace should be on their own lines, alone.

> +  // Styled underlines.
> +  //
> +  // Support for this is provided either by the escape sequence in
> +  // Smulx or the Su flag. The latter results in a common default
> +  // escape sequence and is not recommended.

Comment style again: please use the C style.

> +#ifdef TERMINFO
> +  tty->TF_set_underline_style = tigetstr("Smulx");
> +  if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
> +    tty->TF_set_underline_style = NULL;
> +#else
> +  tty->TF_set_underline_style = tgetstr("Smulx", address);
> +#endif
> +  if (!tty->TF_set_underline_style && tgetflag("Su"))
> +    tty->TF_set_underline_style = "\x1b[4:%p1%dm";

Any pointers to where this standard escape sequence is defined?  I'd
like to have that in a comment to this line.

> +      else if (EQ (CAR_SAFE (val), QCstyle))
> +    {
> +        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))) {

Continuation line style again.

> +            return false; /* Unsupported underline style */

Comments should be complete sentences, and should end in a period and
2 spaces.

> +  /* Text underline.  */
> +  underline = attrs[LFACE_UNDERLINE_INDEX];
> +  if (NILP (underline)) {
> +    face->tty_underline = FACE_NO_UNDERLINE;
> +  } else if (EQ (underline, Qt)) {
> +    face->tty_underline = FACE_UNDER_LINE;
> +  } else if (STRINGP (underline)) {
> +    face->tty_underline = FACE_UNDER_LINE;
> +  } else if (CONSP (underline)) {

Style of braces again.

> +    while (CONSP (underline)) {
> +      Lisp_Object keyword, value;

And here.

> +      if (EQ (keyword, QCstyle)) {

And here.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 1/3] Add face definitions for more underline styles
  2023-04-21 15:58     ` Eli Zaretskii
@ 2023-04-21 16:08       ` Mohsin Kaleem
  0 siblings, 0 replies; 35+ messages in thread
From: Mohsin Kaleem @ 2023-04-21 16:08 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 62994

Eli Zaretskii <eliz@gnu.org> writes:

> Is it really necessary to change the numerical value of
> FACE_UNDER_WAVE?  Can it be left at its original value?

If consistency with Smulx is desirable then I think it is. Otherwise
when we populate the escape sequence for the underline we'll need to map
from the face underline style to the sequence underline style.
Reordering like this makes them match so we don't have to do any extra
processing later, but if preferable I can retain the original order and
add a helper function to map to the sequence value.

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames
  2023-04-21 15:52   ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames Eli Zaretskii
@ 2023-04-21 16:10     ` Mohsin Kaleem
  0 siblings, 0 replies; 35+ messages in thread
From: Mohsin Kaleem @ 2023-04-21 16:10 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 62994

Eli Zaretskii <eliz@gnu.org> writes:

> Please note up front that we must wait for the completion of your
> legal paperwork before we can accept significant contributions (such
> as this one) from you.

Yep, I'm aware. I've already reached out to Craig at the FSF help desk.
I don't think there's anything more needed from me or my employer so I'm
just waiting on a confirmation response. Otherwise I'm OK keeping this
patch dormant until it's ready. I'd like to just get it in a state where
it's good to merge, then when the paperwork is signed we can merge.

> I will send a few comments to each of the patches separately.

Much appreciated, thanks Eli :-).

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 3/3] Add support for colored underlines on tty frames
  2023-04-21 14:34   ` bug#62994: [PATCH 3/3] Add support for colored " mohkale
@ 2023-04-21 16:12     ` Eli Zaretskii
       [not found]       ` <875y9p5fg0.fsf@kisara.moe>
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-21 16:12 UTC (permalink / raw)
  To: mohkale; +Cc: 62994

> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> From: mohkale@kisara.moe
> Date: Fri, 21 Apr 2023 15:34:48 +0100
> 
> diff --git a/etc/NEWS b/etc/NEWS
> index 9f34927dfad..46b2b0e25c5 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -1917,20 +1917,23 @@ exist, the command creates it first.  You can use this command if you
>  inadvertently delete the "*scratch*" buffer.
>  
>  ---
> -** Support for 'styled-underline' face attributes on TTY frames
> +** Support for 'styled' and 'colored' underline face attributes on TTY frames
>  If your terminals termcap or terminfo database entry has the 'Su' or
>  'Smulx' capability defined, Emacs will now emit the prescribed escape
>  sequence necessary to render faces with styled underlines on TTY
>  frames.
>  
>  Styled underlines are any underlines containing a non-default
> -underline style.  The available underline styles for TTY frames are
> -'double', 'wave', 'dotted', and 'dashed'.
> +underline style or a color other than the foreground-color.
> +The available underline styles for TTY frames are 'double', 'wave',
> +'dotted', and 'dashed'.
>  
>  The 'Smulx' capability should define the actual sequence needed to
>  render styled underlines. If ommitted, but the 'Su' flag is defined,
>  then a default sequence will be used. It's recommended to use 'Smulx'
> -instead of 'Su', with priority being given to 'Smulx'.
> +instead of 'Su', with priority being given to 'Smulx'.  Support for
> +colored underlines is automatically enabled with a default escape
> +sequence when styled underline are supported.

Please don't break the changeset into several patches, not in this
case: this changeset introduces a feature that there's no reason to
break into several sub-features, so making a single patch will make it
easier to review the changes.

> +      ts = tty->TF_set_underline_color;
> +      if (ts && face->underline_color)
> +	{
> +          p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);

It looks like your edits use indentation with only spaces.  Our style
in C sources is to use TABs and spaces, so please set up your Emacs to
follow our style (it should happen automatically if you let Emacs read
and follow the .dir-locals.el file in the Emacs repository).

> +  if (tty->TF_set_underline_style)
> +    tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";

What is the source for this escape sequence? can you mention it in a
comment?

Thanks.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
       [not found]       ` <878rel5fqr.fsf@kisara.moe>
@ 2023-04-21 17:49         ` Eli Zaretskii
  2023-04-21 18:04           ` Mohsin Kaleem
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-21 17:49 UTC (permalink / raw)
  To: Mohsin Kaleem; +Cc: 62994

[Please use Reply All to keep the bug address on the CC list.]

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Fri, 21 Apr 2023 18:04:12 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think the last paragraph is unnecessary in NEWS, since there's
> > nothing users can do about the capabilities exposed by their terminfo
> > database.
> 
> I kinda disagree, I think mentioning this can help users debug why
> certain termcaps aren't being processed correctly or to direct requests
> to fix the processing directly to the terminal emulator instead of to
> Emacs first. I'll go ahead and remove it for now.

This is stuff for etc/PROBLEMS, not for NEWS.  And we only add it to
PROBLEMS if someone actually reports the relevant problems.

> > Please use our style of braces and placing of operators in continued
> > lines.  The brace should be on their own lines, alone.
> 
> For future reference is this style documented anywhere?

Yes, in standards.info.  This is the GNU recommended style.

> > Any pointers to where this standard escape sequence is defined?  I'd
> > like to have that in a comment to this line.
> 
> For the underline style I think kitty is a good reference since it was
> the first to support it. That documentation is here [1]. The sequence
> for color is also documented there... but I find the way its described
> at best confusing and at worst unhelpful. I've been using the neovim
> source code [2] as a reference for the implementation and color sequence
> alongside trial and error.
> 
> [1]: https://sw.kovidgoyal.net/kitty/underlines/
> [2]: https://github.com/neovim/neovim/commit/42f492ac99058bd1cd56c3c7871e7e464b2a5e24

Did Kitty invent that?  If not, there must be some reference that is
not the source code of a terminal emulator.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 3/3] Add support for colored underlines on tty frames
       [not found]       ` <875y9p5fg0.fsf@kisara.moe>
@ 2023-04-21 17:56         ` Eli Zaretskii
  0 siblings, 0 replies; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-21 17:56 UTC (permalink / raw)
  To: Mohsin Kaleem; +Cc: 62994

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Fri, 21 Apr 2023 18:10:39 +0100
> 
> > It looks like your edits use indentation with only spaces.  Our style
> > in C sources is to use TABs and spaces, so please set up your Emacs to
> > follow our style (it should happen automatically if you let Emacs read
> > and follow the .dir-locals.el file in the Emacs repository).
> 
> Looks like .dir-locals.el won't work for me since I use c-ts-mode and it
> hooks into c-mode only.

You are right, I've just fixed that.

> >> +  if (tty->TF_set_underline_style)
> >> +    tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";
> >
> > What is the source for this escape sequence? can you mention it in a
> > comment?
> 
> The form here is nonstandard. It's adapted from the same escape sequence
> used by neovim [1]. The difference here is that this form accepts a
> single color parameter whereas that one accepts three different
> parameters for the color range. Since this form is how the existing
> foreground and background sequences are set AND the sequence doesn't
> come from a termcap I figured it was okay to adapt it to a form Emacs
> can already substitute.
> 
> [1]: https://github.com/neovim/neovim/blob/42f492ac99058bd1cd56c3c7871e7e464b2a5e24/src/nvim/tui/tui.c#L1932

This should all be in a comment there, thanks.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-21 17:49         ` Eli Zaretskii
@ 2023-04-21 18:04           ` Mohsin Kaleem
  2023-04-21 18:43             ` Mohsin Kaleem
  2023-04-22  6:47             ` Eli Zaretskii
  0 siblings, 2 replies; 35+ messages in thread
From: Mohsin Kaleem @ 2023-04-21 18:04 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 62994

Eli Zaretskii <eliz@gnu.org> writes:

> [Please use Reply All to keep the bug address on the CC list.]

Thought I did, my bad. Sorry.

> This is stuff for etc/PROBLEMS, not for NEWS.  And we only add it to
> PROBLEMS if someone actually reports the relevant problems.

Good enough :-).

> Yes, in standards.info.  This is the GNU recommended style.

Cool, doesn't seem to be in the Emacs repo but I'm guessing it's this
[1]. I'll try to adapt to it.

[1]: https://www.gnu.org/prep/standards/standards.html#Formatting

> Did Kitty invent that?  If not, there must be some reference that is
> not the source code of a terminal emulator.

I think it did. What I've found online indicates kitty invented it, used
Su for the termcap, this lead to an issue on tmux which suggested
changing it to Smulx [2]. That's why the termcap for it is inconsistent.
I can't find any reference for support before kitty.

[2]: https://github.com/tmux/tmux/issues/1492

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-21 18:04           ` Mohsin Kaleem
@ 2023-04-21 18:43             ` Mohsin Kaleem
  2023-04-22  7:00               ` Eli Zaretskii
  2023-04-22  6:47             ` Eli Zaretskii
  1 sibling, 1 reply; 35+ messages in thread
From: Mohsin Kaleem @ 2023-04-21 18:43 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 62994

Mohsin Kaleem <mohkale@kisara.moe> writes:

> Cool, doesn't seem to be in the Emacs repo but I'm guessing it's this
> [1]. I'll try to adapt to it.

I still don't get how to apply this to this repo Eli. You mentioned using
tabs and spaces but every lines aside from a nested expression starts
with 2 spaces. Some times there's 3 levels deep of expressions all
indented with spaces and then one tab width just wide enough at 8
characters to appear like it's indented with spaces atop the previous
scope.

Can I just do leading indent with tabs matching current indentation
level, curly braces 2 space ahead of the parent indent, and spaces ahead
of tabs to align a parenthesized expression or multi line condition with
the previous line?

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v2 0/1] Support styled underlines on tty Emacs frames
       [not found] <handler.62994.B.168208734930664.ack@debbugs.gnu.org>
  2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
@ 2023-04-21 19:24 ` mohkale
  2023-04-21 19:24   ` bug#62994: [PATCH v2 1/1] Add support for colored and styled underlines on tty frames mohkale
  2023-04-22 10:21 ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames mohkale
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 35+ messages in thread
From: mohkale @ 2023-04-21 19:24 UTC (permalink / raw)
  To: Eli Zaretskii, 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@kisara.moe>

Modern terminals (such as kitty) support setting the style of an
underline with the escape sequence exposed in the Smulx termcap.
This allows for (among others) wavy underlines on terminals.
These terminals also support setting the color of these underlines
using a separate escape sequence that to the best of my knowledge
is not exposed as a termcap but has been adopted by other terminal
supporting editors like neovim.

Version 2: This should've acted on all the comments on the last patch.
The only thing I'm not sure about is the formatting. I've tried my best
to get something consistent with what's already their or the style guide
reference I found online but it's probably broken in a few places.

Mohsin Kaleem (1):
  Add support for colored and styled underlines on tty frames

 etc/NEWS         |  12 +++++
 lisp/cus-face.el |   5 +-
 src/dispextern.h |  10 ++--
 src/term.c       |  54 +++++++++++++++++++--
 src/termchar.h   |   7 +++
 src/xfaces.c     | 121 ++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 189 insertions(+), 20 deletions(-)

-- 
2.40.0






^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v2 1/1] Add support for colored and styled underlines on tty frames
  2023-04-21 19:24 ` bug#62994: [PATCH v2 0/1] " mohkale
@ 2023-04-21 19:24   ` mohkale
  2023-04-24  9:21     ` Robert Pluim
  0 siblings, 1 reply; 35+ messages in thread
From: mohkale @ 2023-04-21 19:24 UTC (permalink / raw)
  To: Eli Zaretskii, 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@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):
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.
---
 etc/NEWS         |  12 +++++
 lisp/cus-face.el |   5 +-
 src/dispextern.h |  10 ++--
 src/term.c       |  54 +++++++++++++++++++--
 src/termchar.h   |   7 +++
 src/xfaces.c     | 121 ++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 189 insertions(+), 20 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 62d2fdcd3a4..e0798687bf4 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1916,6 +1916,18 @@ This command switches to the "*scratch*" buffer.  If "*scratch*" doesn't
 exist, the command creates it first.  You can use this command if you
 inadvertently delete the "*scratch*" buffer.
 
+---
+** Support for 'styled' and 'colored' underline face attributes on TTY frames
+If your terminals termcap or terminfo database entry has the 'Su' or
+'Smulx' capability defined, Emacs will now emit the prescribed escape
+sequence necessary to render faces with styled underlines on TTY
+frames.
+
+Styled underlines are any underlines containing a non-default
+underline style or a color other than the foreground-color.
+The available underline styles for TTY frames are 'double', 'wave',
+'dotted', and 'dashed'.
+
 ** Debugging
 
 ---
diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index ec89b4f7ff6..2d6e6c7b73e 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -141,7 +141,10 @@ custom-face-attributes
 		   (const :format "" :value :style)
 		   (choice :tag "Style"
 			   (const :tag "Line" line)
-			   (const :tag "Wave" wave))
+			   (const :tag "Double" double)
+			   (const :tag "Wave" wave)
+			   (const :tag "Dotted" dotted)
+			   (const :tag "Dashed" dashed))
                    (const :format "" :value :position)
                    (choice :tag "Position"
                            (const :tag "At Default Position" nil)
diff --git a/src/dispextern.h b/src/dispextern.h
index 4dcab113ea2..753bee446f1 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1653,9 +1653,13 @@ #define FONT_TOO_HIGH(ft)						\
 
 enum face_underline_type
 {
+	/* Note: Order matches the order of the Smulx terminfo extension. */
   FACE_NO_UNDERLINE = 0,
   FACE_UNDER_LINE,
-  FACE_UNDER_WAVE
+  FACE_DOUBLE_UNDER_LINE,
+  FACE_UNDER_WAVE,
+  FACE_DOTTED_UNDER_LINE,
+  FACE_DASHED_UNDER_LINE,
 };
 
 /* Structure describing a realized face.
@@ -1737,7 +1741,7 @@ #define FONT_TOO_HIGH(ft)						\
   ENUM_BF (face_box_type) box : 2;
 
   /* Style of underlining. */
-  ENUM_BF (face_underline_type) underline : 2;
+  ENUM_BF (face_underline_type) underline : 3;
 
   /* If `box' above specifies a 3D type, true means use box_color for
      drawing shadows.  */
@@ -1769,7 +1773,6 @@ #define FONT_TOO_HIGH(ft)						\
      string meaning the default color of the TTY.  */
   bool_bf tty_bold_p : 1;
   bool_bf tty_italic_p : 1;
-  bool_bf tty_underline_p : 1;
   bool_bf tty_reverse_p : 1;
   bool_bf tty_strike_through_p : 1;
 
@@ -3361,6 +3364,7 @@ #define TTY_CAP_BOLD		0x04
 #define TTY_CAP_DIM		0x08
 #define TTY_CAP_ITALIC  	0x10
 #define TTY_CAP_STRIKE_THROUGH	0x20
+#define TTY_CAP_UNDERLINE_STYLED	0x32 & TTY_CAP_UNDERLINE
 
 \f
 /***********************************************************************
diff --git a/src/term.c b/src/term.c
index 53ba2a231e4..2c1d44bff7a 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1948,8 +1948,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;
+		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
+		OUTPUT (tty, p);
+		xfree (p);
+	  }
+    }
 
   if (face->tty_strike_through_p
       && MAY_USE_WITH_COLORS_P (tty, NC_STRIKE_THROUGH))
@@ -1975,6 +1986,14 @@ turn_on_face (struct frame *f, int face_id)
 	  OUTPUT (tty, p);
 	  xfree (p);
 	}
+
+      ts = tty->TF_set_underline_color;
+      if (ts && face->underline_color)
+	{
+	  p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);
+	  OUTPUT (tty, p);
+	  xfree (p);
+	}
     }
 }
 
@@ -1995,7 +2014,7 @@ turn_off_face (struct frame *f, int face_id)
       if (face->tty_bold_p
 	  || face->tty_italic_p
 	  || face->tty_reverse_p
-	  || face->tty_underline_p
+	  || face->underline
 	  || face->tty_strike_through_p)
 	{
 	  OUTPUT1_IF (tty, tty->TS_exit_attribute_mode);
@@ -2007,7 +2026,7 @@ turn_off_face (struct frame *f, int face_id)
     {
       /* If we don't have "me" we can only have those appearances
 	 that have exit sequences defined.  */
-      if (face->tty_underline_p)
+      if (face->underline)
 	OUTPUT_IF (tty, tty->TS_exit_underline_mode);
     }
 
@@ -2036,6 +2055,9 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_UNDERLINE,	  tty->TS_enter_underline_mode,
 		     NC_UNDERLINE);
+  TTY_CAPABLE_P_TRY (tty,
+		     TTY_CAP_UNDERLINE_STYLED,	  tty->TF_set_underline_style,
+		     NC_UNDERLINE);
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_BOLD,	  tty->TS_enter_bold_mode, NC_BOLD);
   TTY_CAPABLE_P_TRY (tty,
@@ -4250,6 +4272,30 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   tty->TF_underscore = tgetflag ("ul");
   tty->TF_teleray = tgetflag ("xt");
 
+  /* Styled underlines.  Support for this is provided either by the
+     escape sequence in Smulx or the Su flag.  The latter results in a
+     common default escape sequence and is not recommended.  */
+#ifdef TERMINFO
+	tty->TF_set_underline_style = tigetstr("Smulx");
+	if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
+		tty->TF_set_underline_style = NULL;
+#else
+	tty->TF_set_underline_style = tgetstr("Smulx", address);
+#endif
+	if (!tty->TF_set_underline_style && tgetflag("Su"))
+		/* Default to the kitty escape sequence.  See
+		   https://sw.kovidgoyal.net/kitty/underlines/ */
+		tty->TF_set_underline_style = "\x1b[4:%p1%dm";
+
+	if (tty->TF_set_underline_style)
+		/* This escape sequence for setting the underline color is
+		   consistent with the one described in kitty (see above) and
+		   adapted from the one used by neovim.  This sequence has
+		   been altered from the neovim sequence at
+		   https://github.com/neovim/neovim/blob/42f492ac99058bd1cd56c3c7871e7e464b2a5e24/src/nvim/tui/tui.c
+		   to require only a single parameter, the color index.  */
+		tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";
+
 #else /* DOS_NT */
 #ifdef WINDOWSNT
   {
diff --git a/src/termchar.h b/src/termchar.h
index 5c47679a994..946ffb344d9 100644
--- a/src/termchar.h
+++ b/src/termchar.h
@@ -171,6 +171,13 @@ #define EMACS_TERMCHAR_H
                                    non-blank position.  Must clear before writing _.  */
   int TF_teleray;               /* termcap xt flag: many weird consequences.
                                    For t1061. */
+  const char *TF_set_underline_style; /* termcap Smulx entry: Switches the underline
+                                         style based on the parameter.  Param should
+                                         be one of: 0 (none), 1 (straight), 2 (double),
+                                         3 (wave), 4 (dotted), or 5 (dashed).  */
+  const char *TF_set_underline_color; /* Enabled when TF_set_underline_style is set:
+                                         Sets the color of the underline.  Accepts a
+                                         single parameter, the color index.  */
 
   int RPov;                     /* # chars to start a TS_repeat */
 
diff --git a/src/xfaces.c b/src/xfaces.c
index 37b703984be..65118ff020e 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3255,7 +3255,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
                 }
 
               else if (EQ (key, QCstyle)
-                       && !(EQ (val, Qline) || EQ (val, Qwave)))
+                       && !(EQ (val, Qline)
+                            || EQ (val, Qdouble)
+                            || EQ (val, Qwave)
+                            || EQ (val, Qdotted)
+                            || EQ (val, Qdashed)))
                 {
                   valid_p = false;
                   break;
@@ -5204,6 +5208,7 @@ gui_supports_face_attributes_p (struct frame *f,
                                 Lisp_Object attrs[LFACE_VECTOR_SIZE],
                                 struct face *def_face)
 {
+  Lisp_Object val;
   Lisp_Object *def_attrs = def_face->lface;
   Lisp_Object lattrs[LFACE_VECTOR_SIZE];
 
@@ -5298,6 +5303,20 @@ gui_supports_face_attributes_p (struct frame *f,
       return false;
     }
 
+  /* 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 */
+		  }
+	  }
+    }
+
   /* Everything checks out, this face is supported.  */
   return true;
 }
@@ -5390,15 +5409,26 @@ tty_supports_face_attributes_p (struct frame *f,
   val = attrs[LFACE_UNDERLINE_INDEX];
   if (!UNSPECIFIEDP (val))
     {
-      if (STRINGP (val))
-	return false;		/* ttys can't use colored underlines */
-      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
-	return false;		/* ttys can't use wave underlines */
-      else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
-	return false;		/* same as default */
-      else
-	test_caps |= TTY_CAP_UNDERLINE;
-    }
+	if (STRINGP (val))
+		test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	else if (EQ (CAR_SAFE (val), QCstyle))
+	  {
+	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.  */
+	  }
+
+	test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	  }
+	else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
+		return false;  /* same as default */
+	else
+		test_caps |= TTY_CAP_UNDERLINE;
+  }
 
   /* inverse video */
   val = attrs[LFACE_INVERSE_INDEX];
@@ -6319,6 +6349,8 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
                 face->underline = FACE_UNDER_LINE;
               else if (EQ (value, Qwave))
                 face->underline = FACE_UNDER_WAVE;
+              else
+                face->underline = FACE_UNDER_LINE;
             }
 	  else if (EQ (keyword, QCposition))
 	    {
@@ -6453,6 +6485,7 @@ realize_tty_face (struct face_cache *cache,
 {
   struct face *face;
   int weight, slant;
+  Lisp_Object underline;
   bool face_colors_defaulted = false;
   struct frame *f = cache->f;
 
@@ -6472,13 +6505,74 @@ realize_tty_face (struct face_cache *cache,
     face->tty_bold_p = true;
   if (slant != 100)
     face->tty_italic_p = true;
-  if (!NILP (attrs[LFACE_UNDERLINE_INDEX]))
-    face->tty_underline_p = true;
   if (!NILP (attrs[LFACE_INVERSE_INDEX]))
     face->tty_reverse_p = true;
   if (!NILP (attrs[LFACE_STRIKE_THROUGH_INDEX]))
     face->tty_strike_through_p = true;
 
+  /* Text underline.  */
+  underline = attrs[LFACE_UNDERLINE_INDEX];
+  if (NILP (underline))
+    {
+	face->underline = FACE_NO_UNDERLINE;
+	face->underline_color = 0;
+    }
+  else if (EQ (underline, Qt))
+    {
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+    }
+  else if (STRINGP (underline))
+    {
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = load_color (f, face, underline, LFACE_UNDERLINE_INDEX);
+    }
+  else if (CONSP (underline))
+    {
+	/* `(:color COLOR :style STYLE)'.
+	   STYLE being one of `line', `double', `wave', `dotted' or `dashed'.  */
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+
+	while (CONSP (underline))
+	  {
+		Lisp_Object keyword, value;
+
+		keyword = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (!CONSP (underline))
+			break;
+		value = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (EQ (keyword, QCcolor))
+		  {
+			if (EQ (value, Qforeground_color))
+				face->underline_color = 0;
+			else if (STRINGP (value))
+				face->underline_color
+				    = load_color (f, face, value,
+				                  LFACE_UNDERLINE_INDEX);
+		  }
+		else if (EQ (keyword, QCstyle))
+		  {
+			if (EQ (value, Qline))
+				face->underline = FACE_UNDER_LINE;
+			else if (EQ (value, Qdouble))
+				face->underline = FACE_DOUBLE_UNDER_LINE;
+			else if (EQ (value, Qwave))
+				face->underline = FACE_UNDER_WAVE;
+			else if (EQ (value, Qdotted))
+				face->underline = FACE_DOTTED_UNDER_LINE;
+			else if (EQ (value, Qdashed))
+				face->underline = FACE_DASHED_UNDER_LINE;
+			else
+				face->underline = FACE_UNDER_LINE;
+		  }
+	  }
+    }
+
   /* Map color names to color indices.  */
   map_tty_color (f, face, LFACE_FOREGROUND_INDEX, &face_colors_defaulted);
   map_tty_color (f, face, LFACE_BACKGROUND_INDEX, &face_colors_defaulted);
@@ -7165,6 +7259,9 @@ syms_of_xfaces (void)
   DEFSYM (QCposition, ":position");
   DEFSYM (Qline, "line");
   DEFSYM (Qwave, "wave");
+  DEFSYM (Qdouble, "double");
+  DEFSYM (Qdotted, "dotted");
+  DEFSYM (Qdashed, "dashed");
   DEFSYM (Qreleased_button, "released-button");
   DEFSYM (Qpressed_button, "pressed-button");
   DEFSYM (Qflat_button, "flat-button");
-- 
2.40.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-21 18:04           ` Mohsin Kaleem
  2023-04-21 18:43             ` Mohsin Kaleem
@ 2023-04-22  6:47             ` Eli Zaretskii
  2023-04-22  9:57               ` Mohsin Kaleem
  1 sibling, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-22  6:47 UTC (permalink / raw)
  To: Mohsin Kaleem; +Cc: 62994

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Cc: 62994@debbugs.gnu.org
> Date: Fri, 21 Apr 2023 19:04:13 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, in standards.info.  This is the GNU recommended style.
> 
> Cool, doesn't seem to be in the Emacs repo but I'm guessing it's this
> [1].

Yes.

> I'll try to adapt to it.

It should be easier now, since .dir-locals.el now does that for
c-ts-mode as well, not just for c-mode.

> > Did Kitty invent that?  If not, there must be some reference that is
> > not the source code of a terminal emulator.
> 
> I think it did. What I've found online indicates kitty invented it, used
> Su for the termcap, this lead to an issue on tmux which suggested
> changing it to Smulx [2]. That's why the termcap for it is inconsistent.
> I can't find any reference for support before kitty.

Are there any terminal emulators besides Kitty which support these
attributes?  They should perhaps be mentioned in the NEWS entry.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-21 18:43             ` Mohsin Kaleem
@ 2023-04-22  7:00               ` Eli Zaretskii
  2023-04-22  9:32                 ` Mohsin Kaleem
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2023-04-22  7:00 UTC (permalink / raw)
  To: Mohsin Kaleem; +Cc: 62994

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Cc: 62994@debbugs.gnu.org
> Date: Fri, 21 Apr 2023 19:43:07 +0100
> 
> Mohsin Kaleem <mohkale@kisara.moe> writes:
> 
> > Cool, doesn't seem to be in the Emacs repo but I'm guessing it's this
> > [1]. I'll try to adapt to it.
> 
> I still don't get how to apply this to this repo Eli. You mentioned using
> tabs and spaces but every lines aside from a nested expression starts
> with 2 spaces. Some times there's 3 levels deep of expressions all
> indented with spaces and then one tab width just wide enough at 8
> characters to appear like it's indented with spaces atop the previous
> scope.
> 
> Can I just do leading indent with tabs matching current indentation
> level, curly braces 2 space ahead of the parent indent, and spaces ahead
> of tabs to align a parenthesized expression or multi line condition with
> the previous line?

Why are you doing this manually?  Emacs does that for you, as soon as
you set indent-tabs-mode to a non-nil value.  .dir-locals.el in the
Emacs Git repository should do that automatically; it already does on
the release branch and soon will be doing that on the master branch as
well.  And to reformat existing code, make a region around it and
invoke "M-x tabify".

There should be no need to insert tabs and spaces manually.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-22  7:00               ` Eli Zaretskii
@ 2023-04-22  9:32                 ` Mohsin Kaleem
  0 siblings, 0 replies; 35+ messages in thread
From: Mohsin Kaleem @ 2023-04-22  9:32 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 62994

Eli Zaretskii <eliz@gnu.org> writes:

> Why are you doing this manually?  Emacs does that for you, as soon as
> you set indent-tabs-mode to a non-nil value.  .dir-locals.el in the
> Emacs Git repository should do that automatically; it already does on
> the release branch and soon will be doing that on the master branch as
> well.  And to reformat existing code, make a region around it and
> invoke "M-x tabify".
>
> There should be no need to insert tabs and spaces manually.

Cool. If that works then I'll just reindent any affected hunks using M-x
tabify :-). As for why I didn't do it before, I suppose because I'm used
to Emacs never being able to indent as it should and then re-indenting on
top of my changes with formatters like clang-format. When I don't have
that the only approach I know of is manual.

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames
  2023-04-22  6:47             ` Eli Zaretskii
@ 2023-04-22  9:57               ` Mohsin Kaleem
  0 siblings, 0 replies; 35+ messages in thread
From: Mohsin Kaleem @ 2023-04-22  9:57 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 62994

Eli Zaretskii <eliz@gnu.org> writes:

> Are there any terminal emulators besides Kitty which support these
> attributes?  They should perhaps be mentioned in the NEWS entry.

I've tested the following. I'll add the relevant ones to the NEWS file:

- vte4 (through libvte). Supports both colored and styled underlines,
  but for some reason vte4 uses a TERM of xterm-256color for me so it
  hasn't got the termcaps to expose that it supports it. I guess it
  expects users to override it or is waiting for xterm to support it :/.
- xterm doesn't. I've never used xterm, it might be possible to
  configure it but I won't bother testing too thoroughly.
- St supports styled and colored underlines through the curly underlines
  patch.
- xterm.js apparently supports it [1] but installing hyper to check is
  taking forever so I can't confirm :/. Darn web terminals.

[1]: https://github.com/xtermjs/xterm.js/issues/1145

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames
       [not found] <handler.62994.B.168208734930664.ack@debbugs.gnu.org>
  2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
  2023-04-21 19:24 ` bug#62994: [PATCH v2 0/1] " mohkale
@ 2023-04-22 10:21 ` mohkale
  2023-04-22 10:21   ` bug#62994: [PATCH v3 1/1] Add support for colored and styled underlines on tty frames mohkale
  2024-01-28 11:30   ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames Mohsin Kaleem
  2024-02-11 17:40 ` bug#62994: [PATCH v4] Add support for colored and styled underlines on tty frames mohkale
  2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
  4 siblings, 2 replies; 35+ messages in thread
From: mohkale @ 2023-04-22 10:21 UTC (permalink / raw)
  To: Eli Zaretskii, 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@kisara.moe>

Modern terminals (such as kitty) support setting the style of an
underline with the escape sequence exposed in the Smulx termcap.
This allows for (among others) wavy underlines on terminals.
These terminals also support setting the color of these underlines
using a separate escape sequence that to the best of my knowledge
is not exposed as a termcap but has been adopted by other terminal
supporting editors like neovim.

Version 3: All changed hunks reformatted with M-x tabify.
I think the only comment that hasn't been actioned on is whether to
retain the original order of underline styles in face_underline_type.

Mohsin Kaleem (1):
  Add support for colored and styled underlines on tty frames

 etc/NEWS         |  13 +++++
 lisp/cus-face.el |   5 +-
 src/dispextern.h |  10 ++--
 src/term.c       |  54 +++++++++++++++++++--
 src/termchar.h   |   7 +++
 src/xfaces.c     | 121 ++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 190 insertions(+), 20 deletions(-)

-- 
2.40.0






^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v3 1/1] Add support for colored and styled underlines on tty frames
  2023-04-22 10:21 ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames mohkale
@ 2023-04-22 10:21   ` mohkale
  2024-02-12  1:28     ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-01-28 11:30   ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames Mohsin Kaleem
  1 sibling, 1 reply; 35+ messages in thread
From: mohkale @ 2023-04-22 10:21 UTC (permalink / raw)
  To: Eli Zaretskii, 62994; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@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):
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.
---
 etc/NEWS         |  13 +++++
 lisp/cus-face.el |   5 +-
 src/dispextern.h |  10 ++--
 src/term.c       |  54 +++++++++++++++++++--
 src/termchar.h   |   7 +++
 src/xfaces.c     | 121 ++++++++++++++++++++++++++++++++++++++++++-----
 6 files changed, 190 insertions(+), 20 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 62d2fdcd3a4..b552bb2d75e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1916,6 +1916,19 @@ This command switches to the "*scratch*" buffer.  If "*scratch*" doesn't
 exist, the command creates it first.  You can use this command if you
 inadvertently delete the "*scratch*" buffer.
 
+---
+** Support for 'styled' and 'colored' underline face attributes on TTY frames
+If your terminals termcap or terminfo database entry has the 'Su' or
+'Smulx' capability defined, Emacs will now emit the prescribed escape
+sequence necessary to render faces with styled underlines on TTY
+frames.
+
+Styled underlines are any underlines containing a non-default
+underline style or a color other than the foreground-color.
+The available underline styles for TTY frames are 'double', 'wave',
+'dotted', and 'dashed'.  These are currently supported by Kitty,
+libvte, and st (through the undercurl patch) among other terminals.
+
 ** Debugging
 
 ---
diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index ec89b4f7ff6..2d6e6c7b73e 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -141,7 +141,10 @@ custom-face-attributes
 		   (const :format "" :value :style)
 		   (choice :tag "Style"
 			   (const :tag "Line" line)
-			   (const :tag "Wave" wave))
+			   (const :tag "Double" double)
+			   (const :tag "Wave" wave)
+			   (const :tag "Dotted" dotted)
+			   (const :tag "Dashed" dashed))
                    (const :format "" :value :position)
                    (choice :tag "Position"
                            (const :tag "At Default Position" nil)
diff --git a/src/dispextern.h b/src/dispextern.h
index 4dcab113ea2..753bee446f1 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1653,9 +1653,13 @@ #define FONT_TOO_HIGH(ft)						\
 
 enum face_underline_type
 {
+	/* Note: Order matches the order of the Smulx terminfo extension. */
   FACE_NO_UNDERLINE = 0,
   FACE_UNDER_LINE,
-  FACE_UNDER_WAVE
+  FACE_DOUBLE_UNDER_LINE,
+  FACE_UNDER_WAVE,
+  FACE_DOTTED_UNDER_LINE,
+  FACE_DASHED_UNDER_LINE,
 };
 
 /* Structure describing a realized face.
@@ -1737,7 +1741,7 @@ #define FONT_TOO_HIGH(ft)						\
   ENUM_BF (face_box_type) box : 2;
 
   /* Style of underlining. */
-  ENUM_BF (face_underline_type) underline : 2;
+  ENUM_BF (face_underline_type) underline : 3;
 
   /* If `box' above specifies a 3D type, true means use box_color for
      drawing shadows.  */
@@ -1769,7 +1773,6 @@ #define FONT_TOO_HIGH(ft)						\
      string meaning the default color of the TTY.  */
   bool_bf tty_bold_p : 1;
   bool_bf tty_italic_p : 1;
-  bool_bf tty_underline_p : 1;
   bool_bf tty_reverse_p : 1;
   bool_bf tty_strike_through_p : 1;
 
@@ -3361,6 +3364,7 @@ #define TTY_CAP_BOLD		0x04
 #define TTY_CAP_DIM		0x08
 #define TTY_CAP_ITALIC  	0x10
 #define TTY_CAP_STRIKE_THROUGH	0x20
+#define TTY_CAP_UNDERLINE_STYLED	0x32 & TTY_CAP_UNDERLINE
 
 \f
 /***********************************************************************
diff --git a/src/term.c b/src/term.c
index 53ba2a231e4..b4fb607ee1f 100644
--- a/src/term.c
+++ b/src/term.c
@@ -1948,8 +1948,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;
+		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
+		OUTPUT (tty, p);
+		xfree (p);
+	  }
+	}
 
   if (face->tty_strike_through_p
       && MAY_USE_WITH_COLORS_P (tty, NC_STRIKE_THROUGH))
@@ -1975,6 +1986,14 @@ turn_on_face (struct frame *f, int face_id)
 	  OUTPUT (tty, p);
 	  xfree (p);
 	}
+
+	  ts = tty->TF_set_underline_color;
+	  if (ts && face->underline_color)
+	{
+	  p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);
+	  OUTPUT (tty, p);
+	  xfree (p);
+	}
     }
 }
 
@@ -1995,7 +2014,7 @@ turn_off_face (struct frame *f, int face_id)
       if (face->tty_bold_p
 	  || face->tty_italic_p
 	  || face->tty_reverse_p
-	  || face->tty_underline_p
+	  || face->underline
 	  || face->tty_strike_through_p)
 	{
 	  OUTPUT1_IF (tty, tty->TS_exit_attribute_mode);
@@ -2007,7 +2026,7 @@ turn_off_face (struct frame *f, int face_id)
     {
       /* If we don't have "me" we can only have those appearances
 	 that have exit sequences defined.  */
-      if (face->tty_underline_p)
+      if (face->underline)
 	OUTPUT_IF (tty, tty->TS_exit_underline_mode);
     }
 
@@ -2036,6 +2055,9 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_UNDERLINE,	  tty->TS_enter_underline_mode,
 		     NC_UNDERLINE);
+  TTY_CAPABLE_P_TRY (tty,
+			 TTY_CAP_UNDERLINE_STYLED,	  tty->TF_set_underline_style,
+			 NC_UNDERLINE);
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_BOLD,	  tty->TS_enter_bold_mode, NC_BOLD);
   TTY_CAPABLE_P_TRY (tty,
@@ -4250,6 +4272,30 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   tty->TF_underscore = tgetflag ("ul");
   tty->TF_teleray = tgetflag ("xt");
 
+  /* Styled underlines.	 Support for this is provided either by the
+	 escape sequence in Smulx or the Su flag.  The latter results in a
+	 common default escape sequence and is not recommended.	 */
+#ifdef TERMINFO
+	tty->TF_set_underline_style = tigetstr("Smulx");
+	if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
+		tty->TF_set_underline_style = NULL;
+#else
+	tty->TF_set_underline_style = tgetstr("Smulx", address);
+#endif
+	if (!tty->TF_set_underline_style && tgetflag("Su"))
+		/* Default to the kitty escape sequence.  See
+		   https://sw.kovidgoyal.net/kitty/underlines/ */
+		tty->TF_set_underline_style = "\x1b[4:%p1%dm";
+
+	if (tty->TF_set_underline_style)
+		/* This escape sequence for setting the underline color is
+		   consistent with the one described in kitty (see above) and
+		   adapted from the one used by neovim.	 This sequence has
+		   been altered from the neovim sequence at
+		   https://github.com/neovim/neovim/blob/42f492ac99058bd1cd56c3c7871e7e464b2a5e24/src/nvim/tui/tui.c#L1932
+		   to require only a single parameter, the color index.	 */
+		tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";
+
 #else /* DOS_NT */
 #ifdef WINDOWSNT
   {
diff --git a/src/termchar.h b/src/termchar.h
index 5c47679a994..a9c28fff5cf 100644
--- a/src/termchar.h
+++ b/src/termchar.h
@@ -171,6 +171,13 @@ #define EMACS_TERMCHAR_H
                                    non-blank position.  Must clear before writing _.  */
   int TF_teleray;               /* termcap xt flag: many weird consequences.
                                    For t1061. */
+  const char *TF_set_underline_style; /* termcap Smulx entry: Switches the underline
+										 style based on the parameter.	Param should
+										 be one of: 0 (none), 1 (straight), 2 (double),
+										 3 (wave), 4 (dotted), or 5 (dashed).  */
+  const char *TF_set_underline_color; /* Enabled when TF_set_underline_style is set:
+										 Sets the color of the underline.  Accepts a
+										 single parameter, the color index.	 */
 
   int RPov;                     /* # chars to start a TS_repeat */
 
diff --git a/src/xfaces.c b/src/xfaces.c
index 37b703984be..f07bb6c8eca 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3255,7 +3255,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
                 }
 
               else if (EQ (key, QCstyle)
-                       && !(EQ (val, Qline) || EQ (val, Qwave)))
+					   && !(EQ (val, Qline)
+							|| EQ (val, Qdouble)
+							|| EQ (val, Qwave)
+							|| EQ (val, Qdotted)
+							|| EQ (val, Qdashed)))
                 {
                   valid_p = false;
                   break;
@@ -5204,6 +5208,7 @@ gui_supports_face_attributes_p (struct frame *f,
                                 Lisp_Object attrs[LFACE_VECTOR_SIZE],
                                 struct face *def_face)
 {
+  Lisp_Object val;
   Lisp_Object *def_attrs = def_face->lface;
   Lisp_Object lattrs[LFACE_VECTOR_SIZE];
 
@@ -5298,6 +5303,20 @@ gui_supports_face_attributes_p (struct frame *f,
       return false;
     }
 
+  /* 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 */
+		  }
+	  }
+	}
+
   /* Everything checks out, this face is supported.  */
   return true;
 }
@@ -5390,15 +5409,26 @@ tty_supports_face_attributes_p (struct frame *f,
   val = attrs[LFACE_UNDERLINE_INDEX];
   if (!UNSPECIFIEDP (val))
     {
-      if (STRINGP (val))
-	return false;		/* ttys can't use colored underlines */
-      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
-	return false;		/* ttys can't use wave underlines */
-      else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
-	return false;		/* same as default */
-      else
-	test_caps |= TTY_CAP_UNDERLINE;
-    }
+	if (STRINGP (val))
+		test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	else if (EQ (CAR_SAFE (val), QCstyle))
+	  {
+	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.	*/
+	  }
+
+	test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	  }
+	else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
+		return false;  /* same as default */
+	else
+		test_caps |= TTY_CAP_UNDERLINE;
+  }
 
   /* inverse video */
   val = attrs[LFACE_INVERSE_INDEX];
@@ -6319,6 +6349,8 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
                 face->underline = FACE_UNDER_LINE;
               else if (EQ (value, Qwave))
                 face->underline = FACE_UNDER_WAVE;
+	      else
+		face->underline = FACE_UNDER_LINE;
             }
 	  else if (EQ (keyword, QCposition))
 	    {
@@ -6453,6 +6485,7 @@ realize_tty_face (struct face_cache *cache,
 {
   struct face *face;
   int weight, slant;
+  Lisp_Object underline;
   bool face_colors_defaulted = false;
   struct frame *f = cache->f;
 
@@ -6472,13 +6505,74 @@ realize_tty_face (struct face_cache *cache,
     face->tty_bold_p = true;
   if (slant != 100)
     face->tty_italic_p = true;
-  if (!NILP (attrs[LFACE_UNDERLINE_INDEX]))
-    face->tty_underline_p = true;
   if (!NILP (attrs[LFACE_INVERSE_INDEX]))
     face->tty_reverse_p = true;
   if (!NILP (attrs[LFACE_STRIKE_THROUGH_INDEX]))
     face->tty_strike_through_p = true;
 
+  /* Text underline.  */
+  underline = attrs[LFACE_UNDERLINE_INDEX];
+  if (NILP (underline))
+    {
+	face->underline = FACE_NO_UNDERLINE;
+	face->underline_color = 0;
+    }
+  else if (EQ (underline, Qt))
+    {
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+    }
+  else if (STRINGP (underline))
+    {
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = load_color (f, face, underline, LFACE_UNDERLINE_INDEX);
+    }
+  else if (CONSP (underline))
+    {
+	/* `(:color COLOR :style STYLE)'.
+	   STYLE being one of `line', `double', `wave', `dotted' or `dashed'.  */
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+
+	while (CONSP (underline))
+	  {
+		Lisp_Object keyword, value;
+
+		keyword = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (!CONSP (underline))
+			break;
+		value = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (EQ (keyword, QCcolor))
+		  {
+			if (EQ (value, Qforeground_color))
+				face->underline_color = 0;
+			else if (STRINGP (value))
+				face->underline_color
+				    = load_color (f, face, value,
+						  LFACE_UNDERLINE_INDEX);
+		  }
+		else if (EQ (keyword, QCstyle))
+		  {
+			if (EQ (value, Qline))
+				face->underline = FACE_UNDER_LINE;
+			else if (EQ (value, Qdouble))
+				face->underline = FACE_DOUBLE_UNDER_LINE;
+			else if (EQ (value, Qwave))
+				face->underline = FACE_UNDER_WAVE;
+			else if (EQ (value, Qdotted))
+				face->underline = FACE_DOTTED_UNDER_LINE;
+			else if (EQ (value, Qdashed))
+				face->underline = FACE_DASHED_UNDER_LINE;
+			else
+				face->underline = FACE_UNDER_LINE;
+		  }
+	  }
+    }
+
   /* Map color names to color indices.  */
   map_tty_color (f, face, LFACE_FOREGROUND_INDEX, &face_colors_defaulted);
   map_tty_color (f, face, LFACE_BACKGROUND_INDEX, &face_colors_defaulted);
@@ -7165,6 +7259,9 @@ syms_of_xfaces (void)
   DEFSYM (QCposition, ":position");
   DEFSYM (Qline, "line");
   DEFSYM (Qwave, "wave");
+  DEFSYM (Qdouble, "double");
+  DEFSYM (Qdotted, "dotted");
+  DEFSYM (Qdashed, "dashed");
   DEFSYM (Qreleased_button, "released-button");
   DEFSYM (Qpressed_button, "pressed-button");
   DEFSYM (Qflat_button, "flat-button");
-- 
2.40.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v2 1/1] Add support for colored and styled underlines on tty frames
  2023-04-21 19:24   ` bug#62994: [PATCH v2 1/1] Add support for colored and styled underlines on tty frames mohkale
@ 2023-04-24  9:21     ` Robert Pluim
  0 siblings, 0 replies; 35+ messages in thread
From: Robert Pluim @ 2023-04-24  9:21 UTC (permalink / raw)
  To: mohkale; +Cc: Eli Zaretskii, 62994

>>>>> On Fri, 21 Apr 2023 20:24:33 +0100, mohkale@kisara.moe said:

    Mohsin> From: Mohsin Kaleem <mohkale@kisara.moe>
    Mohsin> * src/dispextern.h (face, face_underline_type,
    Mohsin> syms_of_xfacse,

This typo tell me youʼre not using 'C-x 4 a' to generate your
ChangeLog entries.
 
    Mohsin> +---
    Mohsin> +** Support for 'styled' and 'colored' underline face
    Mohsin> attributes on TTY frames

Full stop at the end of the sentence please.

    Mohsin> +  if (face->underline && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE))
    Mohsin> +    {
    Mohsin> +	if (face->underline == FACE_UNDER_LINE
    Mohsin> +	    || !tty->TF_set_underline_style)
    Mohsin> +		OUTPUT1_IF (tty, tty->TS_enter_underline_mode);
    Mohsin> +	else if (tty->TF_set_underline_style)
    Mohsin> +	  {
    Mohsin> +		char *p;
    Mohsin> +		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
    Mohsin> +		OUTPUT (tty, p);
    Mohsin> +		xfree (p);
    Mohsin> +	  }
    Mohsin> +    }

space before '(' please.


    Mohsin>                else if (EQ (key, QCstyle)
    Mohsin> -                       && !(EQ (val, Qline) || EQ (val, Qwave)))
    Mohsin> +                       && !(EQ (val, Qline)
    Mohsin> +                            || EQ (val, Qdouble)
    Mohsin> +                            || EQ (val, Qwave)
    Mohsin> +                            || EQ (val, Qdotted)
    Mohsin> +                            || EQ (val, Qdashed)))

If this was in lisp Iʼd expect to see `memq', but we work with what we
have :-)

    Mohsin> +  /* Check supported underline styles. */
    Mohsin> +  val = attrs[LFACE_UNDERLINE_INDEX];
    Mohsin> +  if (!UNSPECIFIEDP (val))
    Mohsin> +    {
    Mohsin> +	if (EQ (CAR_SAFE (val), QCstyle))
    Mohsin> +	  {
    Mohsin> +		if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline)
    Mohsin> +		      || EQ (CAR_SAFE (CDR_SAFE (val)), Qwave)))
    Mohsin> +		  {
    Mohsin> +			return false; /* Unsupported underline style */
    Mohsin> +		  }
    Mohsin> +	  }
    Mohsin> +    }
    Mohsin> +
    Mohsin>    /* Everything checks out, this face is supported.  */
    Mohsin>    return true;
    Mohsin>  }

Your TAB usage here still looks wrong

    Mohsin> @@ -5390,15 +5409,26 @@ tty_supports_face_attributes_p (struct frame *f,
    Mohsin>    val = attrs[LFACE_UNDERLINE_INDEX];
    Mohsin>    if (!UNSPECIFIEDP (val))
    Mohsin>      {
    Mohsin> -      if (STRINGP (val))
    Mohsin> -	return false;		/* ttys can't use colored underlines */
    Mohsin> -      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
    Mohsin> -	return false;		/* ttys can't use wave underlines */
    Mohsin> -      else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
    Mohsin> -	return false;		/* same as default */
    Mohsin> -      else
    Mohsin> -	test_caps |= TTY_CAP_UNDERLINE;
    Mohsin> -    }
    Mohsin> +	if (STRINGP (val))
    Mohsin> +		test_caps |= TTY_CAP_UNDERLINE_STYLED;
    Mohsin> +	else if (EQ (CAR_SAFE (val), QCstyle))
    Mohsin> +	  {
    Mohsin> +	if (!(EQ (CAR_SAFE (CDR_SAFE (val)), Qline)
    Mohsin> +	      || EQ (CAR_SAFE (CDR_SAFE (val)), Qdouble)
    Mohsin> +	      || EQ (CAR_SAFE (CDR_SAFE (val)), Qwave)
    Mohsin> +	      || EQ (CAR_SAFE (CDR_SAFE (val)), Qdotted)
    Mohsin> +	      || EQ (CAR_SAFE (CDR_SAFE (val)), Qdashed)))
    Mohsin> +	  {
    Mohsin> +		return false; /* Face uses an unsupported underline style.  */
    Mohsin> +	  }
    Mohsin> +
    Mohsin> +	test_caps |= TTY_CAP_UNDERLINE_STYLED;
    Mohsin> +	  }
    Mohsin> +	else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
    Mohsin> +		return false;  /* same as default */
    Mohsin> +	else
    Mohsin> +		test_caps |= TTY_CAP_UNDERLINE;
    Mohsin> +  }

And here.

Robert
-- 





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames
  2023-04-22 10:21 ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames mohkale
  2023-04-22 10:21   ` bug#62994: [PATCH v3 1/1] Add support for colored and styled underlines on tty frames mohkale
@ 2024-01-28 11:30   ` Mohsin Kaleem
  2024-01-28 12:05     ` Eli Zaretskii
  1 sibling, 1 reply; 35+ messages in thread
From: Mohsin Kaleem @ 2024-01-28 11:30 UTC (permalink / raw)
  To: Eli Zaretskii, 62994

mohkale@kisara.moe writes:

Hi there,

My copyright situation should be solved now. I was waiting on something
else from my work but would rather not put this off any longer. I've
figured out how to do the correct resolution for color. Only thing
that's confusing me a little is that in src/xfaces:6466 for
map_tty_color it looks like we change the color in the face spec itself
only on MSDOS frames. What are the ramifications of this? Is this logic
we want to keep going forward? For reference I don't this should be done
for the underline spec because unlike foreground and background it can
be a string or a plist containing the color value and we would not want
to update it in place.

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames
  2024-01-28 11:30   ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames Mohsin Kaleem
@ 2024-01-28 12:05     ` Eli Zaretskii
       [not found]       ` <86fryg62kh.fsf@kisara.moe>
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2024-01-28 12:05 UTC (permalink / raw)
  To: Mohsin Kaleem; +Cc: 62994

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Sun, 28 Jan 2024 12:30:25 +0100
> 
> mohkale@kisara.moe writes:
> 
> My copyright situation should be solved now. I was waiting on something
> else from my work but would rather not put this off any longer.

That's good to know, thanks.

> I've
> figured out how to do the correct resolution for color. Only thing
> that's confusing me a little is that in src/xfaces:6466 for
> map_tty_color it looks like we change the color in the face spec itself
> only on MSDOS frames. What are the ramifications of this? Is this logic
> we want to keep going forward?

The MSDOS frames are different from other TTY frames in that we can
know the default colors used by the MSDOS terminals.  That's why MSDOS
needs a special code there.  And yes, we want to keep this logic, for
MSDOS only.

> For reference I don't this should be done for the underline spec
> because unlike foreground and background it can be a string or a
> plist containing the color value and we would not want to update it
> in place.

The MSDOS terminal doesn't support underline, so no worries here.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames
       [not found]       ` <86fryg62kh.fsf@kisara.moe>
@ 2024-01-29 12:37         ` Eli Zaretskii
  0 siblings, 0 replies; 35+ messages in thread
From: Eli Zaretskii @ 2024-01-29 12:37 UTC (permalink / raw)
  To: Mohsin Kaleem; +Cc: 62994

[Please use Reply All to reply, to keep the bug tracker CC'ed.]

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Mon, 29 Jan 2024 08:06:38 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The MSDOS frames are different from other TTY frames in that we can
> > know the default colors used by the MSDOS terminals.  That's why MSDOS
> > needs a special code there.  And yes, we want to keep this logic, for
> > MSDOS only.
> 
> Interesting. Didn't know MSDOS terminals expose that. Neat. Although why
> do we update the face spec (face->lface) itself in this case instead of
> only the foreground/background values which we do later anyways.

AFAIR, it's because those default colors are then used to define the
'default' face and the frame's fore/back-ground pixel values, so we
need the actual colors.

> face->lface seems to be the actual spec set in the theme/customisation
> display (unless I'm misunderstanding). If we change it wouldn't that
> alter what's shown in customize for example.

How can that alter what is shown, when the colors we stick in the face
are the exact colors that the screen uses?  It's like you take a
"blue" color that is specified by the special name "default" and
replace it with an actual "blue" -- how can the result be different.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v4] Add support for colored and styled underlines on tty frames
       [not found] <handler.62994.B.168208734930664.ack@debbugs.gnu.org>
                   ` (2 preceding siblings ...)
  2023-04-22 10:21 ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames mohkale
@ 2024-02-11 17:40 ` mohkale
  2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
  4 siblings, 0 replies; 35+ messages in thread
From: mohkale @ 2024-02-11 17:40 UTC (permalink / raw)
  To: 62994, Eli Zaretskii; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@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):
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.
---
 etc/NEWS         |  13 ++++
 lisp/cus-face.el |   5 +-
 src/dispextern.h |  10 ++-
 src/term.c       |  54 +++++++++++++--
 src/termchar.h   |   7 ++
 src/xfaces.c     | 167 +++++++++++++++++++++++++++++++++++++++++++----
 6 files changed, 235 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 5ee1509859b..e6192e09f81 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -311,6 +311,19 @@ Previously, it was set to t but this broke remote file name detection.
 ** Multi-character key echo now ends with a suggestion to use Help.
 Customize 'echo-keystrokes-help' to nil to prevent that.
 
+---
+** Support for 'styled' and 'colored' underline face attributes on TTY frames
+If your terminals termcap or terminfo database entry has the 'Su' or
+'Smulx' capability defined, Emacs will now emit the prescribed escape
+sequence necessary to render faces with styled underlines on TTY
+frames.
+
+Styled underlines are any underlines containing a non-default
+underline style or a color other than the foreground-color.
+The available underline styles for TTY frames are 'double', 'wave',
+'dotted', and 'dashed'.  These are currently supported by Kitty,
+libvte, and st (through the undercurl patch) among other terminals.
+
 \f
 * Editing Changes in Emacs 30.1
 
diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index 47afa841f5e..12551e37785 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -141,7 +141,10 @@ custom-face-attributes
 		   (const :format "" :value :style)
 		   (choice :tag "Style"
 			   (const :tag "Line" line)
-			   (const :tag "Wave" wave))
+			   (const :tag "Double" double)
+			   (const :tag "Wave" wave)
+			   (const :tag "Dotted" dotted)
+			   (const :tag "Dashed" dashed))
                    (const :format "" :value :position)
                    (choice :tag "Position"
                            (const :tag "At Default Position" nil)
diff --git a/src/dispextern.h b/src/dispextern.h
index 5387cb45603..574798fc547 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1690,9 +1690,13 @@ #define FONT_TOO_HIGH(ft)						\
 
 enum face_underline_type
 {
+	/* Note: Order matches the order of the Smulx terminfo extension. */
   FACE_NO_UNDERLINE = 0,
   FACE_UNDER_LINE,
-  FACE_UNDER_WAVE
+  FACE_DOUBLE_UNDER_LINE,
+  FACE_UNDER_WAVE,
+  FACE_DOTTED_UNDER_LINE,
+  FACE_DASHED_UNDER_LINE,
 };
 
 /* Structure describing a realized face.
@@ -1776,7 +1780,7 @@ #define FONT_TOO_HIGH(ft)						\
   ENUM_BF (face_box_type) box : 2;
 
   /* Style of underlining. */
-  ENUM_BF (face_underline_type) underline : 2;
+  ENUM_BF (face_underline_type) underline : 3;
 
   /* If `box' above specifies a 3D type, true means use box_color for
      drawing shadows.  */
@@ -1808,7 +1812,6 @@ #define FONT_TOO_HIGH(ft)						\
      string meaning the default color of the TTY.  */
   bool_bf tty_bold_p : 1;
   bool_bf tty_italic_p : 1;
-  bool_bf tty_underline_p : 1;
   bool_bf tty_reverse_p : 1;
   bool_bf tty_strike_through_p : 1;
 
@@ -3421,6 +3424,7 @@ #define TTY_CAP_BOLD		0x04
 #define TTY_CAP_DIM		0x08
 #define TTY_CAP_ITALIC  	0x10
 #define TTY_CAP_STRIKE_THROUGH	0x20
+#define TTY_CAP_UNDERLINE_STYLED	0x32 & TTY_CAP_UNDERLINE
 
 \f
 /***********************************************************************
diff --git a/src/term.c b/src/term.c
index 3fa244be824..d5a5442cb19 100644
--- 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;
+		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
+		OUTPUT (tty, p);
+		xfree (p);
+	  }
+	}
 
   if (face->tty_strike_through_p
       && MAY_USE_WITH_COLORS_P (tty, NC_STRIKE_THROUGH))
@@ -2041,6 +2052,14 @@ turn_on_face (struct frame *f, int face_id)
 	  OUTPUT (tty, p);
 	  xfree (p);
 	}
+
+	  ts = tty->TF_set_underline_color;
+	  if (ts && face->underline_color)
+	{
+	  p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);
+	  OUTPUT (tty, p);
+	  xfree (p);
+	}
     }
 }
 
@@ -2061,7 +2080,7 @@ turn_off_face (struct frame *f, int face_id)
       if (face->tty_bold_p
 	  || face->tty_italic_p
 	  || face->tty_reverse_p
-	  || face->tty_underline_p
+	  || face->underline
 	  || face->tty_strike_through_p)
 	{
 	  OUTPUT1_IF (tty, tty->TS_exit_attribute_mode);
@@ -2073,7 +2092,7 @@ turn_off_face (struct frame *f, int face_id)
     {
       /* If we don't have "me" we can only have those appearances
 	 that have exit sequences defined.  */
-      if (face->tty_underline_p)
+      if (face->underline)
 	OUTPUT_IF (tty, tty->TS_exit_underline_mode);
     }
 
@@ -2104,6 +2123,9 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_UNDERLINE,	  tty->TS_enter_underline_mode,
 		     NC_UNDERLINE);
+  TTY_CAPABLE_P_TRY (tty,
+			 TTY_CAP_UNDERLINE_STYLED,	  tty->TF_set_underline_style,
+			 NC_UNDERLINE);
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_BOLD,	  tty->TS_enter_bold_mode, NC_BOLD);
   TTY_CAPABLE_P_TRY (tty,
@@ -4360,6 +4382,30 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   tty->TF_underscore = tgetflag ("ul");
   tty->TF_teleray = tgetflag ("xt");
 
+  /* Styled underlines.	 Support for this is provided either by the
+	 escape sequence in Smulx or the Su flag.  The latter results in a
+	 common default escape sequence and is not recommended.	 */
+#ifdef TERMINFO
+	tty->TF_set_underline_style = tigetstr("Smulx");
+	if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
+		tty->TF_set_underline_style = NULL;
+#else
+	tty->TF_set_underline_style = tgetstr("Smulx", address);
+#endif
+	if (!tty->TF_set_underline_style && tgetflag("Su"))
+		/* Default to the kitty escape sequence.  See
+		   https://sw.kovidgoyal.net/kitty/underlines/ */
+		tty->TF_set_underline_style = "\x1b[4:%p1%dm";
+
+	if (tty->TF_set_underline_style)
+		/* This escape sequence for setting the underline color is
+		   consistent with the one described in kitty (see above) and
+		   adapted from the one used by neovim.	 This sequence has
+		   been altered from the neovim sequence at
+		   https://github.com/neovim/neovim/blob/42f492ac99058bd1cd56c3c7871e7e464b2a5e24/src/nvim/tui/tui.c#L1932
+		   to require only a single parameter, the color index.	 */
+		tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";
+
 #else /* DOS_NT */
 #ifdef WINDOWSNT
   {
diff --git a/src/termchar.h b/src/termchar.h
index 2d845107e11..de9009d32f1 100644
--- a/src/termchar.h
+++ b/src/termchar.h
@@ -171,6 +171,13 @@ #define EMACS_TERMCHAR_H
                                    non-blank position.  Must clear before writing _.  */
   int TF_teleray;               /* termcap xt flag: many weird consequences.
                                    For t1061. */
+  const char *TF_set_underline_style; /* termcap Smulx entry: Switches the underline
+										 style based on the parameter.	Param should
+										 be one of: 0 (none), 1 (straight), 2 (double),
+										 3 (wave), 4 (dotted), or 5 (dashed).  */
+  const char *TF_set_underline_color; /* Enabled when TF_set_underline_style is set:
+										 Sets the color of the underline.  Accepts a
+										 single parameter, the color index.	 */
 
   int RPov;                     /* # chars to start a TS_repeat */
 
diff --git a/src/xfaces.c b/src/xfaces.c
index a558e7328c0..75fae11dca3 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3311,7 +3311,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
                 }
 
               else if (EQ (key, QCstyle)
-                       && !(EQ (val, Qline) || EQ (val, Qwave)))
+					   && !(EQ (val, Qline)
+							|| EQ (val, Qdouble)
+							|| EQ (val, Qwave)
+							|| EQ (val, Qdotted)
+							|| EQ (val, Qdashed)))
                 {
                   valid_p = false;
                   break;
@@ -5266,6 +5270,7 @@ gui_supports_face_attributes_p (struct frame *f,
                                 Lisp_Object attrs[LFACE_VECTOR_SIZE],
                                 struct face *def_face)
 {
+  Lisp_Object val;
   Lisp_Object *def_attrs = def_face->lface;
   Lisp_Object lattrs[LFACE_VECTOR_SIZE];
 
@@ -5360,6 +5365,20 @@ gui_supports_face_attributes_p (struct frame *f,
       return false;
     }
 
+  /* 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 */
+		  }
+	  }
+	}
+
   /* Everything checks out, this face is supported.  */
   return true;
 }
@@ -5452,15 +5471,26 @@ tty_supports_face_attributes_p (struct frame *f,
   val = attrs[LFACE_UNDERLINE_INDEX];
   if (!UNSPECIFIEDP (val))
     {
-      if (STRINGP (val))
-	return false;		/* ttys can't use colored underlines */
-      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
-	return false;		/* ttys can't use wave underlines */
-      else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
-	return false;		/* same as default */
-      else
-	test_caps |= TTY_CAP_UNDERLINE;
-    }
+	if (STRINGP (val))
+		test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	else if (EQ (CAR_SAFE (val), QCstyle))
+	  {
+	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.	*/
+	  }
+
+	test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	  }
+	else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
+		return false;  /* same as default */
+	else
+		test_caps |= TTY_CAP_UNDERLINE;
+  }
 
   /* inverse video */
   val = attrs[LFACE_INVERSE_INDEX];
@@ -6381,6 +6411,8 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
                 face->underline = FACE_UNDER_LINE;
               else if (EQ (value, Qwave))
                 face->underline = FACE_UNDER_WAVE;
+	      else
+		face->underline = FACE_UNDER_LINE;
             }
 	  else if (EQ (keyword, QCposition))
 	    {
@@ -6434,7 +6466,53 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
 /* Map a specified color of face FACE on frame F to a tty color index.
    IDX is either LFACE_FOREGROUND_INDEX or LFACE_BACKGROUND_INDEX, and
    specifies which color to map.  Set *DEFAULTED to true if mapping to the
-   default foreground/background colors.  */
+   default foreground/background colors. TODO(me): Update docstring. */
+
+static void
+my_map_tty_color (struct frame *f, struct face *face, Lisp_Object color,
+		  enum lface_attribute_index idx)
+{
+	Lisp_Object frame, def;
+	unsigned long default_pixel = FACE_TTY_DEFAULT_FG_COLOR;
+	unsigned long pixel = default_pixel;
+
+	eassert (idx == LFACE_FOREGROUND_INDEX
+		 || idx == LFACE_BACKGROUND_INDEX
+		 || idx == LFACE_UNDERLINE_INDEX);
+
+	XSETFRAME (frame, f);
+
+	if (STRINGP (color)
+	    && SCHARS (color)
+	    && CONSP (Vtty_defined_color_alist)
+	    && (def = assoc_no_quit (color, call1 (Qtty_color_alist, frame)),
+		CONSP (def)))
+	{
+		/* Associations in tty-defined-color-alist are of the form
+		   (NAME INDEX R G B).  We need the INDEX part.  */
+		pixel = XFIXNUM (XCAR (XCDR (def)));
+	}
+
+	if (pixel == default_pixel && STRINGP (color))
+	{
+		pixel = load_color (f, face, color, idx);
+	}
+
+	switch (idx)
+	{
+		case LFACE_UNDERLINE_INDEX:
+			face->underline_color = pixel;
+			break;
+		case LFACE_FOREGROUND_INDEX:
+			face->foreground = pixel;
+			break;
+		case LFACE_BACKGROUND_INDEX:
+			face->background = pixel;
+			break;
+		default:
+			emacs_abort ();
+	}
+}
 
 static void
 map_tty_color (struct frame *f, struct face *face,
@@ -6515,6 +6593,7 @@ realize_tty_face (struct face_cache *cache,
 {
   struct face *face;
   int weight, slant;
+  Lisp_Object underline;
   bool face_colors_defaulted = false;
   struct frame *f = cache->f;
 
@@ -6534,13 +6613,72 @@ realize_tty_face (struct face_cache *cache,
     face->tty_bold_p = true;
   if (slant != 100)
     face->tty_italic_p = true;
-  if (!NILP (attrs[LFACE_UNDERLINE_INDEX]))
-    face->tty_underline_p = true;
   if (!NILP (attrs[LFACE_INVERSE_INDEX]))
     face->tty_reverse_p = true;
   if (!NILP (attrs[LFACE_STRIKE_THROUGH_INDEX]))
     face->tty_strike_through_p = true;
 
+  /* Text underline.  */
+  underline = attrs[LFACE_UNDERLINE_INDEX];
+  if (NILP (underline))
+    {
+	face->underline = FACE_NO_UNDERLINE;
+	face->underline_color = 0;
+    }
+  else if (EQ (underline, Qt))
+    {
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+    }
+  else if (STRINGP (underline))
+    {
+	face->underline = FACE_UNDER_LINE;
+	my_map_tty_color (f, face, underline, LFACE_UNDERLINE_INDEX);
+    }
+  else if (CONSP (underline))
+    {
+	/* `(:color COLOR :style STYLE)'.
+	   STYLE being one of `line', `double', `wave', `dotted' or `dashed'.  */
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+
+	while (CONSP (underline))
+	  {
+		Lisp_Object keyword, value;
+
+		keyword = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (!CONSP (underline))
+			break;
+		value = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (EQ (keyword, QCcolor))
+		  {
+			if (EQ (value, Qforeground_color))
+				face->underline_color = 0;
+			else if (STRINGP (value))
+				my_map_tty_color (f, face, value, LFACE_UNDERLINE_INDEX);
+		  }
+		else if (EQ (keyword, QCstyle))
+		  {
+			if (EQ (value, Qline))
+				face->underline = FACE_UNDER_LINE;
+			else if (EQ (value, Qdouble))
+				face->underline = FACE_DOUBLE_UNDER_LINE;
+			else if (EQ (value, Qwave))
+				face->underline = FACE_UNDER_WAVE;
+			else if (EQ (value, Qdotted))
+				face->underline = FACE_DOTTED_UNDER_LINE;
+			else if (EQ (value, Qdashed))
+				face->underline = FACE_DASHED_UNDER_LINE;
+			else
+				face->underline = FACE_UNDER_LINE;
+		  }
+	  }
+    }
+
   /* Map color names to color indices.  */
   map_tty_color (f, face, LFACE_FOREGROUND_INDEX, &face_colors_defaulted);
   map_tty_color (f, face, LFACE_BACKGROUND_INDEX, &face_colors_defaulted);
@@ -7229,6 +7367,9 @@ syms_of_xfaces (void)
   DEFSYM (QCposition, ":position");
   DEFSYM (Qline, "line");
   DEFSYM (Qwave, "wave");
+  DEFSYM (Qdouble, "double");
+  DEFSYM (Qdotted, "dotted");
+  DEFSYM (Qdashed, "dashed");
   DEFSYM (Qreleased_button, "released-button");
   DEFSYM (Qpressed_button, "pressed-button");
   DEFSYM (Qflat_button, "flat-button");
-- 
2.43.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
       [not found] <handler.62994.B.168208734930664.ack@debbugs.gnu.org>
                   ` (3 preceding siblings ...)
  2024-02-11 17:40 ` bug#62994: [PATCH v4] Add support for colored and styled underlines on tty frames mohkale
@ 2024-02-11 18:05 ` mohkale
  2024-02-11 18:07   ` Mohsin Kaleem
                     ` (3 more replies)
  4 siblings, 4 replies; 35+ messages in thread
From: mohkale @ 2024-02-11 18:05 UTC (permalink / raw)
  To: 62994, Eli Zaretskii; +Cc: Mohsin Kaleem

From: Mohsin Kaleem <mohkale@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(-)

diff --git a/etc/NEWS b/etc/NEWS
index 5ee1509859b..d8cf8671d9d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -311,6 +311,21 @@ Previously, it was set to t but this broke remote file name detection.
 ** Multi-character key echo now ends with a suggestion to use Help.
 Customize 'echo-keystrokes-help' to nil to prevent that.
 
+** Terminal Emacs
+
++++
+*** Support for 'styled' and 'colored' underline face attributes on TTY frames
+If your terminals termcap or terminfo database entry has the 'Su' or
+'Smulx' capability defined, Emacs will now emit the prescribed escape
+sequence necessary to render faces with styled underlines on TTY
+frames.
+
+Styled underlines are any underlines containing a non-default
+underline style or a color other than the foreground-color.
+The available underline styles for TTY frames are 'double', 'wave',
+'dotted', and 'dashed'.  These are currently supported by Kitty,
+libvte, and st (through the undercurl patch) among other terminals.
+
 \f
 * Editing Changes in Emacs 30.1
 
diff --git a/lisp/cus-face.el b/lisp/cus-face.el
index 47afa841f5e..12551e37785 100644
--- a/lisp/cus-face.el
+++ b/lisp/cus-face.el
@@ -141,7 +141,10 @@ custom-face-attributes
 		   (const :format "" :value :style)
 		   (choice :tag "Style"
 			   (const :tag "Line" line)
-			   (const :tag "Wave" wave))
+			   (const :tag "Double" double)
+			   (const :tag "Wave" wave)
+			   (const :tag "Dotted" dotted)
+			   (const :tag "Dashed" dashed))
                    (const :format "" :value :position)
                    (choice :tag "Position"
                            (const :tag "At Default Position" nil)
diff --git a/src/dispextern.h b/src/dispextern.h
index 5387cb45603..574798fc547 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -1690,9 +1690,13 @@ #define FONT_TOO_HIGH(ft)						\
 
 enum face_underline_type
 {
+	/* Note: Order matches the order of the Smulx terminfo extension. */
   FACE_NO_UNDERLINE = 0,
   FACE_UNDER_LINE,
-  FACE_UNDER_WAVE
+  FACE_DOUBLE_UNDER_LINE,
+  FACE_UNDER_WAVE,
+  FACE_DOTTED_UNDER_LINE,
+  FACE_DASHED_UNDER_LINE,
 };
 
 /* Structure describing a realized face.
@@ -1776,7 +1780,7 @@ #define FONT_TOO_HIGH(ft)						\
   ENUM_BF (face_box_type) box : 2;
 
   /* Style of underlining. */
-  ENUM_BF (face_underline_type) underline : 2;
+  ENUM_BF (face_underline_type) underline : 3;
 
   /* If `box' above specifies a 3D type, true means use box_color for
      drawing shadows.  */
@@ -1808,7 +1812,6 @@ #define FONT_TOO_HIGH(ft)						\
      string meaning the default color of the TTY.  */
   bool_bf tty_bold_p : 1;
   bool_bf tty_italic_p : 1;
-  bool_bf tty_underline_p : 1;
   bool_bf tty_reverse_p : 1;
   bool_bf tty_strike_through_p : 1;
 
@@ -3421,6 +3424,7 @@ #define TTY_CAP_BOLD		0x04
 #define TTY_CAP_DIM		0x08
 #define TTY_CAP_ITALIC  	0x10
 #define TTY_CAP_STRIKE_THROUGH	0x20
+#define TTY_CAP_UNDERLINE_STYLED	0x32 & TTY_CAP_UNDERLINE
 
 \f
 /***********************************************************************
diff --git a/src/term.c b/src/term.c
index 3fa244be824..d5a5442cb19 100644
--- 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;
+		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
+		OUTPUT (tty, p);
+		xfree (p);
+	  }
+	}
 
   if (face->tty_strike_through_p
       && MAY_USE_WITH_COLORS_P (tty, NC_STRIKE_THROUGH))
@@ -2041,6 +2052,14 @@ turn_on_face (struct frame *f, int face_id)
 	  OUTPUT (tty, p);
 	  xfree (p);
 	}
+
+	  ts = tty->TF_set_underline_color;
+	  if (ts && face->underline_color)
+	{
+	  p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);
+	  OUTPUT (tty, p);
+	  xfree (p);
+	}
     }
 }
 
@@ -2061,7 +2080,7 @@ turn_off_face (struct frame *f, int face_id)
       if (face->tty_bold_p
 	  || face->tty_italic_p
 	  || face->tty_reverse_p
-	  || face->tty_underline_p
+	  || face->underline
 	  || face->tty_strike_through_p)
 	{
 	  OUTPUT1_IF (tty, tty->TS_exit_attribute_mode);
@@ -2073,7 +2092,7 @@ turn_off_face (struct frame *f, int face_id)
     {
       /* If we don't have "me" we can only have those appearances
 	 that have exit sequences defined.  */
-      if (face->tty_underline_p)
+      if (face->underline)
 	OUTPUT_IF (tty, tty->TS_exit_underline_mode);
     }
 
@@ -2104,6 +2123,9 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_UNDERLINE,	  tty->TS_enter_underline_mode,
 		     NC_UNDERLINE);
+  TTY_CAPABLE_P_TRY (tty,
+			 TTY_CAP_UNDERLINE_STYLED,	  tty->TF_set_underline_style,
+			 NC_UNDERLINE);
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_BOLD,	  tty->TS_enter_bold_mode, NC_BOLD);
   TTY_CAPABLE_P_TRY (tty,
@@ -4360,6 +4382,30 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   tty->TF_underscore = tgetflag ("ul");
   tty->TF_teleray = tgetflag ("xt");
 
+  /* Styled underlines.	 Support for this is provided either by the
+	 escape sequence in Smulx or the Su flag.  The latter results in a
+	 common default escape sequence and is not recommended.	 */
+#ifdef TERMINFO
+	tty->TF_set_underline_style = tigetstr("Smulx");
+	if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
+		tty->TF_set_underline_style = NULL;
+#else
+	tty->TF_set_underline_style = tgetstr("Smulx", address);
+#endif
+	if (!tty->TF_set_underline_style && tgetflag("Su"))
+		/* Default to the kitty escape sequence.  See
+		   https://sw.kovidgoyal.net/kitty/underlines/ */
+		tty->TF_set_underline_style = "\x1b[4:%p1%dm";
+
+	if (tty->TF_set_underline_style)
+		/* This escape sequence for setting the underline color is
+		   consistent with the one described in kitty (see above) and
+		   adapted from the one used by neovim.	 This sequence has
+		   been altered from the neovim sequence at
+		   https://github.com/neovim/neovim/blob/42f492ac99058bd1cd56c3c7871e7e464b2a5e24/src/nvim/tui/tui.c#L1932
+		   to require only a single parameter, the color index.	 */
+		tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";
+
 #else /* DOS_NT */
 #ifdef WINDOWSNT
   {
diff --git a/src/termchar.h b/src/termchar.h
index 2d845107e11..de9009d32f1 100644
--- a/src/termchar.h
+++ b/src/termchar.h
@@ -171,6 +171,13 @@ #define EMACS_TERMCHAR_H
                                    non-blank position.  Must clear before writing _.  */
   int TF_teleray;               /* termcap xt flag: many weird consequences.
                                    For t1061. */
+  const char *TF_set_underline_style; /* termcap Smulx entry: Switches the underline
+										 style based on the parameter.	Param should
+										 be one of: 0 (none), 1 (straight), 2 (double),
+										 3 (wave), 4 (dotted), or 5 (dashed).  */
+  const char *TF_set_underline_color; /* Enabled when TF_set_underline_style is set:
+										 Sets the color of the underline.  Accepts a
+										 single parameter, the color index.	 */
 
   int RPov;                     /* # chars to start a TS_repeat */
 
diff --git a/src/xfaces.c b/src/xfaces.c
index a558e7328c0..a39e2bb6781 100644
--- a/src/xfaces.c
+++ b/src/xfaces.c
@@ -3311,7 +3311,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
                 }
 
               else if (EQ (key, QCstyle)
-                       && !(EQ (val, Qline) || EQ (val, Qwave)))
+					   && !(EQ (val, Qline)
+							|| EQ (val, Qdouble)
+							|| EQ (val, Qwave)
+							|| EQ (val, Qdotted)
+							|| EQ (val, Qdashed)))
                 {
                   valid_p = false;
                   break;
@@ -5266,6 +5270,7 @@ gui_supports_face_attributes_p (struct frame *f,
                                 Lisp_Object attrs[LFACE_VECTOR_SIZE],
                                 struct face *def_face)
 {
+  Lisp_Object val;
   Lisp_Object *def_attrs = def_face->lface;
   Lisp_Object lattrs[LFACE_VECTOR_SIZE];
 
@@ -5360,6 +5365,20 @@ gui_supports_face_attributes_p (struct frame *f,
       return false;
     }
 
+  /* 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 */
+		  }
+	  }
+	}
+
   /* Everything checks out, this face is supported.  */
   return true;
 }
@@ -5452,15 +5471,26 @@ tty_supports_face_attributes_p (struct frame *f,
   val = attrs[LFACE_UNDERLINE_INDEX];
   if (!UNSPECIFIEDP (val))
     {
-      if (STRINGP (val))
-	return false;		/* ttys can't use colored underlines */
-      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
-	return false;		/* ttys can't use wave underlines */
-      else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
-	return false;		/* same as default */
-      else
-	test_caps |= TTY_CAP_UNDERLINE;
-    }
+	if (STRINGP (val))
+		test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	else if (EQ (CAR_SAFE (val), QCstyle))
+	  {
+	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.	*/
+	  }
+
+	test_caps |= TTY_CAP_UNDERLINE_STYLED;
+	  }
+	else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
+		return false;  /* same as default */
+	else
+		test_caps |= TTY_CAP_UNDERLINE;
+  }
 
   /* inverse video */
   val = attrs[LFACE_INVERSE_INDEX];
@@ -6381,6 +6411,8 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
                 face->underline = FACE_UNDER_LINE;
               else if (EQ (value, Qwave))
                 face->underline = FACE_UNDER_WAVE;
+	      else
+		face->underline = FACE_UNDER_LINE;
             }
 	  else if (EQ (keyword, QCposition))
 	    {
@@ -6431,17 +6463,18 @@ realize_gui_face (struct face_cache *cache, Lisp_Object attrs[LFACE_VECTOR_SIZE]
 }
 
 
-/* Map a specified color of face FACE on frame F to a tty color index.
-   IDX is either LFACE_FOREGROUND_INDEX or LFACE_BACKGROUND_INDEX, and
-   specifies which color to map.  Set *DEFAULTED to true if mapping to the
-   default foreground/background colors.  */
+/* 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. */
 
 static void
-map_tty_color (struct frame *f, struct face *face,
+map_tty_color (struct frame *f, struct face *face, Lisp_Object color,
 	       enum lface_attribute_index idx, bool *defaulted)
 {
-  Lisp_Object frame, color, def;
-  bool foreground_p = idx == LFACE_FOREGROUND_INDEX;
+  Lisp_Object frame, def;
+  bool foreground_p = idx != LFACE_BACKGROUND_INDEX;
   unsigned long default_pixel =
     foreground_p ? FACE_TTY_DEFAULT_FG_COLOR : FACE_TTY_DEFAULT_BG_COLOR;
   unsigned long pixel = default_pixel;
@@ -6450,10 +6483,11 @@ map_tty_color (struct frame *f, struct face *face,
     foreground_p ? FACE_TTY_DEFAULT_BG_COLOR : FACE_TTY_DEFAULT_FG_COLOR;
 #endif
 
-  eassert (idx == LFACE_FOREGROUND_INDEX || idx == LFACE_BACKGROUND_INDEX);
+  eassert (idx == LFACE_FOREGROUND_INDEX
+	   || idx == LFACE_BACKGROUND_INDEX
+	   || idx == LFACE_UNDERLINE_INDEX);
 
   XSETFRAME (frame, f);
-  color = face->lface[idx];
 
   if (STRINGP (color)
       && SCHARS (color)
@@ -6498,10 +6532,28 @@ map_tty_color (struct frame *f, struct face *face,
 #endif /* MSDOS */
     }
 
-  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 ();
+	}
+}
+
+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);
 }
 
 
@@ -6515,6 +6567,7 @@ realize_tty_face (struct face_cache *cache,
 {
   struct face *face;
   int weight, slant;
+  Lisp_Object underline;
   bool face_colors_defaulted = false;
   struct frame *f = cache->f;
 
@@ -6534,16 +6587,77 @@ realize_tty_face (struct face_cache *cache,
     face->tty_bold_p = true;
   if (slant != 100)
     face->tty_italic_p = true;
-  if (!NILP (attrs[LFACE_UNDERLINE_INDEX]))
-    face->tty_underline_p = true;
   if (!NILP (attrs[LFACE_INVERSE_INDEX]))
     face->tty_reverse_p = true;
   if (!NILP (attrs[LFACE_STRIKE_THROUGH_INDEX]))
     face->tty_strike_through_p = true;
 
+  /* Text underline.  */
+  underline = attrs[LFACE_UNDERLINE_INDEX];
+  if (NILP (underline))
+    {
+	face->underline = FACE_NO_UNDERLINE;
+	face->underline_color = 0;
+    }
+  else if (EQ (underline, Qt))
+    {
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+    }
+  else if (STRINGP (underline))
+    {
+	face->underline = FACE_UNDER_LINE;
+	map_tty_color2 (f, face, underline, LFACE_UNDERLINE_INDEX);
+    }
+  else if (CONSP (underline))
+    {
+	/* `(:color COLOR :style STYLE)'.
+	   STYLE being one of `line', `double', `wave', `dotted' or `dashed'.  */
+	face->underline = FACE_UNDER_LINE;
+	face->underline_color = 0;
+
+	while (CONSP (underline))
+	  {
+		Lisp_Object keyword, value;
+
+		keyword = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (!CONSP (underline))
+			break;
+		value = XCAR (underline);
+		underline = XCDR (underline);
+
+		if (EQ (keyword, QCcolor))
+		  {
+			if (EQ (value, Qforeground_color))
+				face->underline_color = 0;
+			else if (STRINGP (value))
+				map_tty_color2 (f, face, value, LFACE_UNDERLINE_INDEX);
+		  }
+		else if (EQ (keyword, QCstyle))
+		  {
+			if (EQ (value, Qline))
+				face->underline = FACE_UNDER_LINE;
+			else if (EQ (value, Qdouble))
+				face->underline = FACE_DOUBLE_UNDER_LINE;
+			else if (EQ (value, Qwave))
+				face->underline = FACE_UNDER_WAVE;
+			else if (EQ (value, Qdotted))
+				face->underline = FACE_DOTTED_UNDER_LINE;
+			else if (EQ (value, Qdashed))
+				face->underline = FACE_DASHED_UNDER_LINE;
+			else
+				face->underline = FACE_UNDER_LINE;
+		  }
+	  }
+    }
+
   /* Map color names to color indices.  */
-  map_tty_color (f, face, LFACE_FOREGROUND_INDEX, &face_colors_defaulted);
-  map_tty_color (f, face, LFACE_BACKGROUND_INDEX, &face_colors_defaulted);
+  map_tty_color (f, face, face->lface[LFACE_FOREGROUND_INDEX],
+		 LFACE_FOREGROUND_INDEX, &face_colors_defaulted);
+  map_tty_color (f, face, face->lface[LFACE_BACKGROUND_INDEX],
+		 LFACE_BACKGROUND_INDEX, &face_colors_defaulted);
 
   /* Swap colors if face is inverse-video.  If the colors are taken
      from the frame colors, they are already inverted, since the
@@ -7229,6 +7343,9 @@ syms_of_xfaces (void)
   DEFSYM (QCposition, ":position");
   DEFSYM (Qline, "line");
   DEFSYM (Qwave, "wave");
+  DEFSYM (Qdouble, "double");
+  DEFSYM (Qdotted, "dotted");
+  DEFSYM (Qdashed, "dashed");
   DEFSYM (Qreleased_button, "released-button");
   DEFSYM (Qpressed_button, "pressed-button");
   DEFSYM (Qflat_button, "flat-button");
-- 
2.43.0






^ permalink raw reply related	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
  2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
@ 2024-02-11 18:07   ` Mohsin Kaleem
  2024-02-12  1:43   ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 35+ messages in thread
From: Mohsin Kaleem @ 2024-02-11 18:07 UTC (permalink / raw)
  To: 62994, Eli Zaretskii

mohkale@kisara.moe writes:

Finally got there XD. This is the correct commit. Please disregard V4.

-- 
Mohsin Kaleem





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v3 1/1] Add support for colored and styled underlines on tty frames
  2023-04-22 10:21   ` bug#62994: [PATCH v3 1/1] Add support for colored and styled underlines on tty frames mohkale
@ 2024-02-12  1:28     ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 35+ messages in thread
From: Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-12  1:28 UTC (permalink / raw)
  To: mohkale; +Cc: Eli Zaretskii, 62994

mohkale@kisara.moe writes:

> From: Mohsin Kaleem <mohkale@kisara.moe>
>
> * src/dispextern.h (face, face_underline_type, syms_of_xfacse,
> internal-set-lisp-face-attribute, gui_supports_face_attributes_p):

Type `C-c C-w' within a *vc-log* buffer to generate commit messages, and
fill them with `M-q'.  Otherwise, you will wind up with typos and errors
such as this entry, where a newline is used to separate entries within a
single pair of parentheses.

Thanks.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
  2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
  2024-02-11 18:07   ` Mohsin Kaleem
@ 2024-02-12  1:43   ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
  2024-02-12 13:50     ` Eli Zaretskii
  2024-02-12 12:48   ` Eli Zaretskii
  2024-02-14 19:40   ` Jim Porter
  3 siblings, 1 reply; 35+ messages in thread
From: Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-12  1:43 UTC (permalink / raw)
  To: mohkale; +Cc: Eli Zaretskii, 62994

mohkale@kisara.moe writes:

> ++++

Where's the documentation for this change?

> +*** Support for 'styled' and 'colored' underline face attributes on TTY frames

We prefer to punctuate sentences within NEWS headings.

> +If your terminals termcap or terminfo database entry has the 'Su' or
> +'Smulx' capability defined, Emacs will now emit the prescribed escape
> +sequence necessary to render faces with styled underlines on TTY
> +frames.
> +
> +Styled underlines are any underlines containing a non-default
> +underline style or a color other than the foreground-color.
> +The available underline styles for TTY frames are 'double', 'wave',
> +'dotted', and 'dashed'.  These are currently supported by Kitty,
> +libvte, and st (through the undercurl patch) among other terminals.

What about GUI frames?  I don't want to see a display feature installed
before it is also implemented for the likes of X.

>  \f
>  * Editing Changes in Emacs 30.1
>  
> diff --git a/lisp/cus-face.el b/lisp/cus-face.el
> index 47afa841f5e..12551e37785 100644
> --- a/lisp/cus-face.el
> +++ b/lisp/cus-face.el
> @@ -141,7 +141,10 @@ custom-face-attributes
>  		   (const :format "" :value :style)
>  		   (choice :tag "Style"
>  			   (const :tag "Line" line)
> -			   (const :tag "Wave" wave))
> +			   (const :tag "Double" double)
> +			   (const :tag "Wave" wave)
> +			   (const :tag "Dotted" dotted)
> +			   (const :tag "Dashed" dashed))
>                     (const :format "" :value :position)
>                     (choice :tag "Position"
>                             (const :tag "At Default Position" nil)
> diff --git a/src/dispextern.h b/src/dispextern.h
> index 5387cb45603..574798fc547 100644
> --- a/src/dispextern.h
> +++ b/src/dispextern.h
> @@ -1690,9 +1690,13 @@ #define FONT_TOO_HIGH(ft)						\
>  
>  enum face_underline_type
>  {
> +	/* Note: Order matches the order of the Smulx terminfo extension. */
>    FACE_NO_UNDERLINE = 0,
>    FACE_UNDER_LINE,
> -  FACE_UNDER_WAVE
> +  FACE_DOUBLE_UNDER_LINE,
> +  FACE_UNDER_WAVE,
> +  FACE_DOTTED_UNDER_LINE,
> +  FACE_DASHED_UNDER_LINE,
>  };
>  
>  /* Structure describing a realized face.
> @@ -1776,7 +1780,7 @@ #define FONT_TOO_HIGH(ft)						\
>    ENUM_BF (face_box_type) box : 2;
>  
>    /* Style of underlining. */
> -  ENUM_BF (face_underline_type) underline : 2;
> +  ENUM_BF (face_underline_type) underline : 3;
>  
>    /* If `box' above specifies a 3D type, true means use box_color for
>       drawing shadows.  */
> @@ -1808,7 +1812,6 @@ #define FONT_TOO_HIGH(ft)						\
>       string meaning the default color of the TTY.  */
>    bool_bf tty_bold_p : 1;
>    bool_bf tty_italic_p : 1;
> -  bool_bf tty_underline_p : 1;
>    bool_bf tty_reverse_p : 1;
>    bool_bf tty_strike_through_p : 1;
>  
> @@ -3421,6 +3424,7 @@ #define TTY_CAP_BOLD		0x04
>  #define TTY_CAP_DIM		0x08
>  #define TTY_CAP_ITALIC  	0x10
>  #define TTY_CAP_STRIKE_THROUGH	0x20
> +#define TTY_CAP_UNDERLINE_STYLED	0x32 & TTY_CAP_UNDERLINE

#define TTY_CAP_UNDERLINE_STYLED (0x32 & TTY_CAP_UNDERLINE)

> +  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;
> +		p = tparam(tty->TF_set_underline_style, NULL, 0, face->underline, 0, 0, 0);
> +		OUTPUT (tty, p);
> +		xfree (p);
> +	  }
> +	}

In Emacs, we format code with a mixture of tabs and spaces, using tabs
to indent by whole tab stops and spaces to indent to the desired column,
which is a multiple of 2 columns for most statements or the column after
the opening paren where applicable.  Please also insert a space between
function identifiers and argument lists, and confine all text to 80
columns.

>    if (face->tty_strike_through_p
>        && MAY_USE_WITH_COLORS_P (tty, NC_STRIKE_THROUGH))
> @@ -2041,6 +2052,14 @@ turn_on_face (struct frame *f, int face_id)
>  	  OUTPUT (tty, p);
>  	  xfree (p);
>  	}
> +
> +	  ts = tty->TF_set_underline_color;
> +	  if (ts && face->underline_color)
> +	{
> +	  p = tparam (ts, NULL, 0, face->underline_color, 0, 0, 0);
> +	  OUTPUT (tty, p);
> +	  xfree (p);
> +	}
>      }
>  }

Likewise.

> @@ -2061,7 +2080,7 @@ turn_off_face (struct frame *f, int face_id)
>        if (face->tty_bold_p
>  	  || face->tty_italic_p
>  	  || face->tty_reverse_p
> -	  || face->tty_underline_p
> +	  || face->underline
>  	  || face->tty_strike_through_p)
>  	{
>  	  OUTPUT1_IF (tty, tty->TS_exit_attribute_mode);
> @@ -2073,7 +2092,7 @@ turn_off_face (struct frame *f, int face_id)
>      {
>        /* If we don't have "me" we can only have those appearances
>  	 that have exit sequences defined.  */
> -      if (face->tty_underline_p)
> +      if (face->underline)
>  	OUTPUT_IF (tty, tty->TS_exit_underline_mode);
>      }
>  
> @@ -2104,6 +2123,9 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
>    TTY_CAPABLE_P_TRY (tty,
>  		     TTY_CAP_UNDERLINE,	  tty->TS_enter_underline_mode,
>  		     NC_UNDERLINE);
> +  TTY_CAPABLE_P_TRY (tty,
> +			 TTY_CAP_UNDERLINE_STYLED,	  tty->TF_set_underline_style,
> +			 NC_UNDERLINE);
>    TTY_CAPABLE_P_TRY (tty,
>  		     TTY_CAP_BOLD,	  tty->TS_enter_bold_mode, NC_BOLD);
>    TTY_CAPABLE_P_TRY (tty,
> @@ -4360,6 +4382,30 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
>    tty->TF_underscore = tgetflag ("ul");
>    tty->TF_teleray = tgetflag ("xt");
>  
> +  /* Styled underlines.	 Support for this is provided either by the
> +	 escape sequence in Smulx or the Su flag.  The latter results in a
> +	 common default escape sequence and is not recommended.	 */

Here you have evidently typed `M-x tabify' with a comment selected.  Do
not insert tabs by whatever means within the body of a comment, although
it is best to indent the body itself with them, whenever possible.

> +#ifdef TERMINFO
> +	tty->TF_set_underline_style = tigetstr("Smulx");
> +	if (tty->TF_set_underline_style == (char *) (intptr_t) -1)
> +		tty->TF_set_underline_style = NULL;
> +#else
> +	tty->TF_set_underline_style = tgetstr("Smulx", address);
> +#endif
> +	if (!tty->TF_set_underline_style && tgetflag("Su"))
> +		/* Default to the kitty escape sequence.  See
> +		   https://sw.kovidgoyal.net/kitty/underlines/ */
> +		tty->TF_set_underline_style = "\x1b[4:%p1%dm";
> +
> +	if (tty->TF_set_underline_style)
> +		/* This escape sequence for setting the underline color is
> +		   consistent with the one described in kitty (see above) and
> +		   adapted from the one used by neovim.	 This sequence has
> +		   been altered from the neovim sequence at
> +		   https://github.com/neovim/neovim/blob/42f492ac99058bd1cd56c3c7871e7e464b2a5e24/src/nvim/tui/tui.c#L1932
> +		   to require only a single parameter, the color index.	 */
> +		tty->TF_set_underline_color = "\x1b[58:2::%p1%{65536}%/%d:%p1%{256}%/%{255}%&%d:%p1%{255}%&%dm";

More indentation problems.  Please don't link to Github, and explain
instead the reasoning behind the sequence itself.

>  #else /* DOS_NT */
>  #ifdef WINDOWSNT
>    {
> diff --git a/src/termchar.h b/src/termchar.h
> index 2d845107e11..de9009d32f1 100644
> --- a/src/termchar.h
> +++ b/src/termchar.h
> @@ -171,6 +171,13 @@ #define EMACS_TERMCHAR_H
>                                     non-blank position.  Must clear before writing _.  */
>    int TF_teleray;               /* termcap xt flag: many weird consequences.
>                                     For t1061. */
> +  const char *TF_set_underline_style; /* termcap Smulx entry: Switches the underline
> +										 style based on the parameter.	Param should
> +										 be one of: 0 (none), 1 (straight), 2 (double),
> +										 3 (wave), 4 (dotted), or 5 (dashed).  */
> +  const char *TF_set_underline_color; /* Enabled when TF_set_underline_style is set:
> +										 Sets the color of the underline.  Accepts a
> +										 single parameter, the color index.	 */

Egregious indentation error.

>    int RPov;                     /* # chars to start a TS_repeat */
>  
> diff --git a/src/xfaces.c b/src/xfaces.c
> index a558e7328c0..a39e2bb6781 100644
> --- a/src/xfaces.c
> +++ b/src/xfaces.c
> @@ -3311,7 +3311,11 @@ DEFUN ("internal-set-lisp-face-attribute", Finternal_set_lisp_face_attribute,
>                  }
>  
>                else if (EQ (key, QCstyle)
> -                       && !(EQ (val, Qline) || EQ (val, Qwave)))
> +					   && !(EQ (val, Qline)
> +							|| EQ (val, Qdouble)
> +							|| EQ (val, Qwave)
> +							|| EQ (val, Qdotted)
> +							|| EQ (val, Qdashed)))
>                  {
>                    valid_p = false;
>                    break;
> @@ -5266,6 +5270,7 @@ gui_supports_face_attributes_p (struct frame *f,
>                                  Lisp_Object attrs[LFACE_VECTOR_SIZE],
>                                  struct face *def_face)
>  {
> +  Lisp_Object val;
>    Lisp_Object *def_attrs = def_face->lface;
>    Lisp_Object lattrs[LFACE_VECTOR_SIZE];
>  
> @@ -5360,6 +5365,20 @@ gui_supports_face_attributes_p (struct frame *f,
>        return false;
>      }
>  
> +  /* 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 */
> +		  }
> +	  }
> +	}
> +
>    /* Everything checks out, this face is supported.  */
>    return true;
>  }
> @@ -5452,15 +5471,26 @@ tty_supports_face_attributes_p (struct frame *f,
>    val = attrs[LFACE_UNDERLINE_INDEX];
>    if (!UNSPECIFIEDP (val))
>      {
> -      if (STRINGP (val))
> -	return false;		/* ttys can't use colored underlines */
> -      else if (EQ (CAR_SAFE (val), QCstyle) && EQ (CAR_SAFE (CDR_SAFE (val)), Qwave))
> -	return false;		/* ttys can't use wave underlines */
> -      else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
> -	return false;		/* same as default */
> -      else
> -	test_caps |= TTY_CAP_UNDERLINE;
> -    }
> +	if (STRINGP (val))
> +		test_caps |= TTY_CAP_UNDERLINE_STYLED;
> +	else if (EQ (CAR_SAFE (val), QCstyle))
> +	  {
> +	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.	*/
> +	  }
> +
> +	test_caps |= TTY_CAP_UNDERLINE_STYLED;
> +	  }
> +	else if (face_attr_equal_p (val, def_attrs[LFACE_UNDERLINE_INDEX]))
> +		return false;  /* same as default */
> +	else
> +		test_caps |= TTY_CAP_UNDERLINE;
> +  }

Ditto.  Please also avoid inserting redundant braces in if statements.

>  static void
> -map_tty_color (struct frame *f, struct face *face,
> +map_tty_color (struct frame *f, struct face *face, Lisp_Object color,
>  	       enum lface_attribute_index idx, bool *defaulted)
>  {
> -  Lisp_Object frame, color, def;
> -  bool foreground_p = idx == LFACE_FOREGROUND_INDEX;
> +  Lisp_Object frame, def;
> +  bool foreground_p = idx != LFACE_BACKGROUND_INDEX;
>    unsigned long default_pixel =
>      foreground_p ? FACE_TTY_DEFAULT_FG_COLOR : FACE_TTY_DEFAULT_BG_COLOR;
>    unsigned long pixel = default_pixel;
> @@ -6450,10 +6483,11 @@ map_tty_color (struct frame *f, struct face *face,
>      foreground_p ? FACE_TTY_DEFAULT_BG_COLOR : FACE_TTY_DEFAULT_FG_COLOR;
>  #endif
>  
> -  eassert (idx == LFACE_FOREGROUND_INDEX || idx == LFACE_BACKGROUND_INDEX);
> +  eassert (idx == LFACE_FOREGROUND_INDEX
> +	   || idx == LFACE_BACKGROUND_INDEX
> +	   || idx == LFACE_UNDERLINE_INDEX);
>  
>    XSETFRAME (frame, f);
> -  color = face->lface[idx];
>  
>    if (STRINGP (color)
>        && SCHARS (color)
> @@ -6498,10 +6532,28 @@ map_tty_color (struct frame *f, struct face *face,
>  #endif /* MSDOS */
>      }
>  
> -  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 ();
> +	}
> +}
> +
> +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);
>  }

Ditto.

> @@ -6515,6 +6567,7 @@ realize_tty_face (struct face_cache *cache,
>  {
>    struct face *face;
>    int weight, slant;
> +  Lisp_Object underline;
>    bool face_colors_defaulted = false;
>    struct frame *f = cache->f;
>  
> @@ -6534,16 +6587,77 @@ realize_tty_face (struct face_cache *cache,
>      face->tty_bold_p = true;
>    if (slant != 100)
>      face->tty_italic_p = true;
> -  if (!NILP (attrs[LFACE_UNDERLINE_INDEX]))
> -    face->tty_underline_p = true;
>    if (!NILP (attrs[LFACE_INVERSE_INDEX]))
>      face->tty_reverse_p = true;
>    if (!NILP (attrs[LFACE_STRIKE_THROUGH_INDEX]))
>      face->tty_strike_through_p = true;
>  
> +  /* Text underline.  */
> +  underline = attrs[LFACE_UNDERLINE_INDEX];
> +  if (NILP (underline))
> +    {
> +	face->underline = FACE_NO_UNDERLINE;
> +	face->underline_color = 0;
> +    }
> +  else if (EQ (underline, Qt))
> +    {
> +	face->underline = FACE_UNDER_LINE;
> +	face->underline_color = 0;
> +    }
> +  else if (STRINGP (underline))
> +    {
> +	face->underline = FACE_UNDER_LINE;
> +	map_tty_color2 (f, face, underline, LFACE_UNDERLINE_INDEX);
> +    }
> +  else if (CONSP (underline))
> +    {
> +	/* `(:color COLOR :style STYLE)'.
> +	   STYLE being one of `line', `double', `wave', `dotted' or `dashed'.  */
> +	face->underline = FACE_UNDER_LINE;
> +	face->underline_color = 0;
> +
> +	while (CONSP (underline))
> +	  {
> +		Lisp_Object keyword, value;
> +
> +		keyword = XCAR (underline);
> +		underline = XCDR (underline);
> +
> +		if (!CONSP (underline))
> +			break;
> +		value = XCAR (underline);
> +		underline = XCDR (underline);
> +
> +		if (EQ (keyword, QCcolor))
> +		  {
> +			if (EQ (value, Qforeground_color))
> +				face->underline_color = 0;
> +			else if (STRINGP (value))
> +				map_tty_color2 (f, face, value, LFACE_UNDERLINE_INDEX);
> +		  }
> +		else if (EQ (keyword, QCstyle))
> +		  {
> +			if (EQ (value, Qline))
> +				face->underline = FACE_UNDER_LINE;
> +			else if (EQ (value, Qdouble))
> +				face->underline = FACE_DOUBLE_UNDER_LINE;
> +			else if (EQ (value, Qwave))
> +				face->underline = FACE_UNDER_WAVE;
> +			else if (EQ (value, Qdotted))
> +				face->underline = FACE_DOTTED_UNDER_LINE;
> +			else if (EQ (value, Qdashed))
> +				face->underline = FACE_DASHED_UNDER_LINE;
> +			else
> +				face->underline = FACE_UNDER_LINE;
> +		  }
> +	  }
> +    }

And ditto.  Thanks.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
  2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
  2024-02-11 18:07   ` Mohsin Kaleem
  2024-02-12  1:43   ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-02-12 12:48   ` Eli Zaretskii
  2024-02-14 19:40   ` Jim Porter
  3 siblings, 0 replies; 35+ messages in thread
From: Eli Zaretskii @ 2024-02-12 12:48 UTC (permalink / raw)
  To: mohkale; +Cc: 62994

> From: mohkale@kisara.moe
> Cc: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Sun, 11 Feb 2024 18:05:01 +0000
> 
> From: Mohsin Kaleem <mohkale@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?





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
  2024-02-12  1:43   ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
@ 2024-02-12 13:50     ` Eli Zaretskii
  2024-02-12 14:49       ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 1 reply; 35+ messages in thread
From: Eli Zaretskii @ 2024-02-12 13:50 UTC (permalink / raw)
  To: Po Lu; +Cc: mohkale, 62994

> From: Po Lu <luangruo@yahoo.com>
> Cc: 62994@debbugs.gnu.org,  Eli Zaretskii <eliz@gnu.org>
> Date: Mon, 12 Feb 2024 09:43:11 +0800
> 
> > +If your terminals termcap or terminfo database entry has the 'Su' or
> > +'Smulx' capability defined, Emacs will now emit the prescribed escape
> > +sequence necessary to render faces with styled underlines on TTY
> > +frames.
> > +
> > +Styled underlines are any underlines containing a non-default
> > +underline style or a color other than the foreground-color.
> > +The available underline styles for TTY frames are 'double', 'wave',
> > +'dotted', and 'dashed'.  These are currently supported by Kitty,
> > +libvte, and st (through the undercurl patch) among other terminals.
> 
> What about GUI frames?  I don't want to see a display feature installed
> before it is also implemented for the likes of X.

Unless you are saying that implementing this on X would be impossible
or very hard, I don't think we need to delay installing this feature
until it is also available on X.  Of course, if Mohsin can also
provide an X-based implementation, that would be super.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
  2024-02-12 13:50     ` Eli Zaretskii
@ 2024-02-12 14:49       ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
  0 siblings, 0 replies; 35+ messages in thread
From: Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors @ 2024-02-12 14:49 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: mohkale, 62994

Eli Zaretskii <eliz@gnu.org> writes:

> Of course, if Mohsin can also provide an X-based implementation, that
> would be super.

This was my question, yes.  Thanks.





^ permalink raw reply	[flat|nested] 35+ messages in thread

* bug#62994: [PATCH v5] Add support for colored and styled underlines on tty frames
  2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
                     ` (2 preceding siblings ...)
  2024-02-12 12:48   ` Eli Zaretskii
@ 2024-02-14 19:40   ` Jim Porter
  3 siblings, 0 replies; 35+ messages in thread
From: Jim Porter @ 2024-02-14 19:40 UTC (permalink / raw)
  To: mohkale, 62994, Eli Zaretskii

On 2/11/2024 10:05 AM, mohkale@kisara.moe wrote:
> @@ -7229,6 +7343,9 @@ syms_of_xfaces (void)
>     DEFSYM (QCposition, ":position");
>     DEFSYM (Qline, "line");
>     DEFSYM (Qwave, "wave");
> +  DEFSYM (Qdouble, "double");
> +  DEFSYM (Qdotted, "dotted");
> +  DEFSYM (Qdashed, "dashed");
>     DEFSYM (Qreleased_button, "released-button");
>     DEFSYM (Qpressed_button, "pressed-button");
>     DEFSYM (Qflat_button, "flat-button");

This is very minor, but for consistency with the existing styles ("line" 
and "wave"), maybe we should use a noun form for the new styles. For 
example "dots", "dashes", and maybe "double-line"? That would hopefully 
be easier for programmers to remember, and leaves the door open for some 
other kind of doubled underline in the future too (e.g. a double wave).





^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2024-02-14 19:40 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <handler.62994.B.168208734930664.ack@debbugs.gnu.org>
2023-04-21 14:34 ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames mohkale
2023-04-21 14:34   ` bug#62994: [PATCH 1/3] Add face definitions for more underline styles mohkale
2023-04-21 15:58     ` Eli Zaretskii
2023-04-21 16:08       ` Mohsin Kaleem
2023-04-21 14:34   ` bug#62994: [PATCH 2/3] Add support for styled underlines on tty frames mohkale
2023-04-21 16:07     ` Eli Zaretskii
     [not found]       ` <878rel5fqr.fsf@kisara.moe>
2023-04-21 17:49         ` Eli Zaretskii
2023-04-21 18:04           ` Mohsin Kaleem
2023-04-21 18:43             ` Mohsin Kaleem
2023-04-22  7:00               ` Eli Zaretskii
2023-04-22  9:32                 ` Mohsin Kaleem
2023-04-22  6:47             ` Eli Zaretskii
2023-04-22  9:57               ` Mohsin Kaleem
2023-04-21 14:34   ` bug#62994: [PATCH 3/3] Add support for colored " mohkale
2023-04-21 16:12     ` Eli Zaretskii
     [not found]       ` <875y9p5fg0.fsf@kisara.moe>
2023-04-21 17:56         ` Eli Zaretskii
2023-04-21 15:52   ` bug#62994: [PATCH 0/3] Support styled underlines on tty Emacs frames Eli Zaretskii
2023-04-21 16:10     ` Mohsin Kaleem
2023-04-21 19:24 ` bug#62994: [PATCH v2 0/1] " mohkale
2023-04-21 19:24   ` bug#62994: [PATCH v2 1/1] Add support for colored and styled underlines on tty frames mohkale
2023-04-24  9:21     ` Robert Pluim
2023-04-22 10:21 ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames mohkale
2023-04-22 10:21   ` bug#62994: [PATCH v3 1/1] Add support for colored and styled underlines on tty frames mohkale
2024-02-12  1:28     ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-01-28 11:30   ` bug#62994: [PATCH v3 0/1] Support styled underlines on tty Emacs frames Mohsin Kaleem
2024-01-28 12:05     ` Eli Zaretskii
     [not found]       ` <86fryg62kh.fsf@kisara.moe>
2024-01-29 12:37         ` Eli Zaretskii
2024-02-11 17:40 ` bug#62994: [PATCH v4] Add support for colored and styled underlines on tty frames mohkale
2024-02-11 18:05 ` bug#62994: [PATCH v5] " mohkale
2024-02-11 18:07   ` Mohsin Kaleem
2024-02-12  1:43   ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-12 13:50     ` Eli Zaretskii
2024-02-12 14:49       ` Po Lu via Bug reports for GNU Emacs, the Swiss army knife of text editors
2024-02-12 12:48   ` Eli Zaretskii
2024-02-14 19:40   ` Jim Porter

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.