bug#75024: [PATCH] Fix check for underlining capability on ttys

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Gerd Möllmann]

[-- Attachment #1: Type: text/plain, Size: 1973 bytes --]

Tags: patch

With current master

emacs -nw -Q on Terminal.app, $TERM=xterm-256color

1. (display-supports-face-attributes-p '(underline t))
     => nil

2. C-h f context-menu-map RET
   => The separator line in *Help* in underlined, which
   means that term.c thinks that underlines can be used.

display-supports-face-attribute-p uses tty_capable_p in term.c.
This code in tty_capable_p looks wrong:

  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);

It returns false as soon as it finds TS_enter_underline_mode is cannot
be used, and doesn't check TS_set_underline_style. The output code uses
one or the other

  if (face->underline && MAY_USE_WITH_COLORS_P (tty, NC_UNDERLINE))
    {
      if (face->underline == FACE_UNDERLINE_SINGLE
	  || !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 GNU Emacs 31.0.50 (build 6, aarch64-apple-darwin24.2.0) of 2024-12-22
 built on pro2
Repository revision: d481da70010eab163d12f770ed11f8fef171406a
Repository branch: cl-packages
System Description:  macOS 15.2

Configured using:
 'configure --without-ns --cache-file
 /var/folders/1d/k_6t25f94sl83szqbf8gpkrh0000gn/T//config.cache.cl-packages
 --with-native-compilation --with-mps=yes CC=clang
 'CFLAGS=-Wgnu-imaginary-constant -Wunused-result -g
 -fno-omit-frame-pointer -F
 /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/System/Library/Frameworks
 -Wno-ignored-attributes -Wno-flag-enum -Wno-missing-method-return-type
 -Wno-variadic-macros -Wno-strict-prototypes -Wno-availability
 -Wno-nullability-completeness' --prefix=/Users/gerd/.local'


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-check-for-underlining-capability-on-ttys.patch --]
[-- Type: text/patch, Size: 1991 bytes --]

From 419a5fa0fd98f673660b123f5b37c99cd0b8c61b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gerd=20M=C3=B6llmann?= <gerd@gnu.org>
Date: Sun, 22 Dec 2024 14:11:33 +0100
Subject: [PATCH] Fix check for underlining capability on ttys

* src/term.c (tty_capable_p): Check both possible terminal capabilities
for underlining.
---
 src/term.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/src/term.c b/src/term.c
index f2d1846e488..875d4d2deff 100644
--- a/src/term.c
+++ b/src/term.c
@@ -2113,18 +2113,20 @@ turn_off_face (struct frame *f, struct face *face)
 tty_capable_p (struct tty_display_info *tty, unsigned int caps)
 {
 #ifndef HAVE_ANDROID
-#define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
-  if ((caps & (cap)) && (!(TS) || !MAY_USE_WITH_COLORS_P (tty, NC_bit)))	\
-    return 0;
+# define TTY_CAPABLE_P(tty, cap, TS, NC_bit) \
+   ((caps & (cap)) && (TS) && MAY_USE_WITH_COLORS_P (tty, NC_bit))
+# define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit) \
+  if (!TTY_CAPABLE_P (tty, cap, TS, NC_bit)) \
+    return false;
+
+  if (!TTY_CAPABLE_P (tty, TTY_CAP_UNDERLINE, tty->TS_enter_underline_mode,
+		      NC_UNDERLINE)
+      && !TTY_CAPABLE_P (tty, TTY_CAP_UNDERLINE_STYLED,
+			 tty->TF_set_underline_style, NC_UNDERLINE))
+    return false;
 
   TTY_CAPABLE_P_TRY (tty,
 		     TTY_CAP_INVERSE,	  tty->TS_standout_mode, NC_REVERSE);
-  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,
@@ -2135,8 +2137,7 @@ #define TTY_CAPABLE_P_TRY(tty, cap, TS, NC_bit)				\
 		     TTY_CAP_STRIKE_THROUGH, tty->TS_enter_strike_through_mode,
 		     NC_STRIKE_THROUGH);
 
-  /* We can do it!  */
-  return 1;
+  return true;
 #else
   return false;
 #endif
-- 
2.47.1

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Gerd Möllmann]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Tags: patch

Please disregard the patch. I'll send another one later.

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Gerd Möllmann]

Hi Mohsin, friendly ping. Could you find the time to look at this?

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>>
>>> Tags: patch
>>
>> Please disregard the patch. I'll send another one later.
>
> I meanwhile found this, to my great surprise:
>
> #define TTY_CAP_UNDERLINE_STYLED	(0x32 & TTY_CAP_UNDERLINE)
>
> That makes TTY_CAP_UNDERLINE_STYLED == TTY_CAP_UNDERLINE. And this test
> in tty_capable_p
>
>   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,
>
> fails because it tests TTY_CAP_UNDERLINE twice, and requires both
> TS_enter_underline_mode and TF_set_underline_style to be usable for
> underline support. In Terminal.app, only TS_enter_underline_mode is
> available.
>
> Maybe this should have been 
>
> #define TTY_CAP_UNDERLINE_STYLED	0x40
>
> ?
>
> BTW, the 0x32 also also makes no sense to me because of 
>
> #define TTY_CAP_ITALIC  	0x10
> #define TTY_CAP_STRIKE_THROUGH	0x20
>
> CC to the original author to check.

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Mohsin Kaleem]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

Hi there,

Sorry for the late response.

>> I meanwhile found this, to my great surprise:
>>
>> #define TTY_CAP_UNDERLINE_STYLED	(0x32 & TTY_CAP_UNDERLINE)
>>
>> That makes TTY_CAP_UNDERLINE_STYLED == TTY_CAP_UNDERLINE. And this test
>> in tty_capable_p
>>
>>   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,
>>
>> fails because it tests TTY_CAP_UNDERLINE twice, and requires both
>> TS_enter_underline_mode and TF_set_underline_style to be usable for
>> underline support. In Terminal.app, only TS_enter_underline_mode is
>> available.
>>
>> Maybe this should have been 
>>
>> #define TTY_CAP_UNDERLINE_STYLED	0x40
>>
>> ?

Ah, yep. The original intention was or a new bit flag with the existing
underline bit flag so that styled underlines were only available in
environments with at least regular underlines. In retrospect that was
probably excessive and simply checking for styled underline support by
itself (with a value of 0x40) is sufficient. I'd find it strange to have
a terminal that supported styled underlines but not regular ones but
there's no need to enforce this on the Emacs side.

>>
>> BTW, the 0x32 also also makes no sense to me because of 
>>
>> #define TTY_CAP_ITALIC  	0x10
>> #define TTY_CAP_STRIKE_THROUGH	0x20
>>
>> CC to the original author to check.

Correct here as well, I should've confirmed the binary representation
:-(.

0b00000000000000000000000000010000 0o00000000020 0d0000000016 0x00000010
0b00000000000000000000000000100000 0o00000000040 0d0000000032 0x00000020
0b00000000000000000000000000110010 0o00000000062 0d0000000050 0x00000032
0b00000000000000000000000001000000 0o00000000100 0d0000000064 0x00000040

0x40 is what the next entry in the flag should have been.

-- 
Mohsin Kaleem

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Gerd Möllmann]

Mohsin Kaleem <mohkale@kisara.moe> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
> Hi there,
>
> Sorry for the late response.
>
>>> I meanwhile found this, to my great surprise:
>>>
>>> #define TTY_CAP_UNDERLINE_STYLED	(0x32 & TTY_CAP_UNDERLINE)
>>>
>>> That makes TTY_CAP_UNDERLINE_STYLED == TTY_CAP_UNDERLINE. And this test
>>> in tty_capable_p
>>>
>>>   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,
>>>
>>> fails because it tests TTY_CAP_UNDERLINE twice, and requires both
>>> TS_enter_underline_mode and TF_set_underline_style to be usable for
>>> underline support. In Terminal.app, only TS_enter_underline_mode is
>>> available.
>>>
>>> Maybe this should have been 
>>>
>>> #define TTY_CAP_UNDERLINE_STYLED	0x40
>>>
>>> ?
>
> Ah, yep. The original intention was or a new bit flag with the existing
> underline bit flag so that styled underlines were only available in
> environments with at least regular underlines. In retrospect that was
> probably excessive and simply checking for styled underline support by
> itself (with a value of 0x40) is sufficient. I'd find it strange to have
> a terminal that supported styled underlines but not regular ones but
> there's no need to enforce this on the Emacs side.
>
>>>
>>> BTW, the 0x32 also also makes no sense to me because of 
>>>
>>> #define TTY_CAP_ITALIC  	0x10
>>> #define TTY_CAP_STRIKE_THROUGH	0x20
>>>
>>> CC to the original author to check.
>
> Correct here as well, I should've confirmed the binary representation
> :-(.
>
> 0b00000000000000000000000000010000 0o00000000020 0d0000000016 0x00000010
> 0b00000000000000000000000000100000 0o00000000040 0d0000000032 0x00000020
> 0b00000000000000000000000000110010 0o00000000062 0d0000000050 0x00000032
> 0b00000000000000000000000001000000 0o00000000100 0d0000000064 0x00000040
>
> 0x40 is what the next entry in the flag should have been.

Thanks for checking, Mohsin!

Would you perhaps have the time to prepare a patch that fixes this?

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Mohsin Kaleem]

Gerd Möllmann <gerd.moellmann@gmail.com> writes:

> Would you perhaps have the time to prepare a patch that fixes this?

I'm a bit swamped atm but could try prepping something in the next few
weeks. Hopefully shouldn't be too big of a change :-).

-- 
Mohsin Kaleem

bug#75024: [PATCH] Fix check for underlining capability on ttys

[Gerd Möllmann]

Mohsin Kaleem <mohkale@kisara.moe> writes:

> Gerd Möllmann <gerd.moellmann@gmail.com> writes:
>
>> Would you perhaps have the time to prepare a patch that fixes this?
>
> I'm a bit swamped atm but could try prepping something in the next few
> weeks. Hopefully shouldn't be too big of a change :-).

Thanks!