bug#62265: Underline does not work in Terminal Emacs

bug#62265: Underline does not work in Terminal Emacs

[Mohsin Kaleem]

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


Hi,

Underline support for tty frames was added to Emacs back in the Emacs
28.0.90 release. It hasn't worked for me since. Today I tried
investigating why. I tracked it down ncurses and discovered the tgetstr
function to retrieve the escape sequence for underline support doesn't
fetch the value configured in the terminfo database for my Terminal
(st). I can reproduce this function failing to fetch the entry in a
minimal sample program (code attached).


[-- Attachment #2: Sample program that fails to get smxx --]
[-- Type: text/plain, Size: 390 bytes --]

#include <stdio.h>
#include <curses.h>
#include <term.h>

int main() {
    curses_trace(TRACE_MAXIMUM);

    const char* term = "tmux-24bit";
    char buffer[4096];
    if (tgetent(buffer, term) <= 0) {
        printf("Failed");
        return 1;
    }

    char *area = NULL;
    char **address = &area;
    char * it = tgetstr("smxx", address);
    printf("p: %p\n", it);
    return 0;
}

[-- Attachment #3: Type: text/plain, Size: 734 bytes --]


The cause for this seems to be in ncurses directly. In the definition of
tgetstr in lib_termcap.c there's a check for ValidExt for any
non-standard terminfo entries. This macro fails when fetching an entry
that is longer than 2 characters, meaning tgetstr for "smxx" fails and I
get no underlines. I've managed to fix this by using tigetstr in-place
of tgetstr (this variant is also used for querying the setf24 and setb24
termcaps already in "term.c" so I suspect this is a known issue). I'm
not sure what the termcap library does but I'm guessing it doesn't have
this restriction.

Please fix this by switching to tigetstr instead of tgetstr for this
record when building with terminfo. I've got a sample patch for this
attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: fix-underline.patch --]
[-- Type: text/x-patch, Size: 623 bytes --]

diff --git a/src/term.c b/src/term.c
index d881dee39fe..a2d1743bdad 100644
--- a/src/term.c
+++ b/src/term.c
@@ -4163,7 +4163,11 @@ use the Bourne shell command 'TERM=...; export TERM' (C-shell:\n\
   tty->TS_enter_alt_charset_mode = tgetstr ("as", address);
   tty->TS_exit_alt_charset_mode = tgetstr ("ae", address);
   tty->TS_exit_attribute_mode = tgetstr ("me", address);
+#ifdef TERMINFO
+  tty->TS_enter_strike_through_mode = tigetstr ("smxx");
+#else
   tty->TS_enter_strike_through_mode = tgetstr ("smxx", address);
+#end
 
   MultiUp (tty) = tgetstr ("UP", address);
   MultiDown (tty) = tgetstr ("DO", address);

[-- Attachment #5: Type: text/plain, Size: 19 bytes --]


-- 
Mohsin Kaleem

bug#62265: Underline does not work in Terminal Emacs

[Mohsin Kaleem]

S.N. Sorry, smxx is the code for strike-through. The error description
should be strikethrough doesn't work in terminal emacs. Not underline.

-- 
Mohsin Kaleem

bug#62265: Underline does not work in Terminal Emacs

[Eli Zaretskii]

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Sat, 18 Mar 2023 17:40:51 +0000
> 
> Underline support for tty frames was added to Emacs back in the Emacs
> 28.0.90 release. It hasn't worked for me since. Today I tried
> investigating why. I tracked it down ncurses and discovered the tgetstr
> function to retrieve the escape sequence for underline support doesn't
> fetch the value configured in the terminfo database for my Terminal
> (st). I can reproduce this function failing to fetch the entry in a
> minimal sample program (code attached).
> 
> The cause for this seems to be in ncurses directly. In the definition of
> tgetstr in lib_termcap.c there's a check for ValidExt for any
> non-standard terminfo entries. This macro fails when fetching an entry
> that is longer than 2 characters, meaning tgetstr for "smxx" fails and I
> get no underlines. I've managed to fix this by using tigetstr in-place
> of tgetstr (this variant is also used for querying the setf24 and setb24
> termcaps already in "term.c" so I suspect this is a known issue). I'm
> not sure what the termcap library does but I'm guessing it doesn't have
> this restriction.
> 
> Please fix this by switching to tigetstr instead of tgetstr for this
> record when building with terminfo. I've got a sample patch for this
> attached.

Thanks.  Can someone with access to such a terminal please verify the
issue with ncurses, and also verify the proposed change?

bug#62265: Underline does not work in Terminal Emacs

[Eli Zaretskii]

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Date: Sat, 18 Mar 2023 17:40:51 +0000
> 
> Underline support for tty frames was added to Emacs back in the Emacs
> 28.0.90 release. It hasn't worked for me since. Today I tried
> investigating why. I tracked it down ncurses and discovered the tgetstr
> function to retrieve the escape sequence for underline support doesn't
> fetch the value configured in the terminfo database for my Terminal
> (st). I can reproduce this function failing to fetch the entry in a
> minimal sample program (code attached).
> [...]
> --- a/src/term.c
> +++ b/src/term.c
> @@ -4163,7 +4163,11 @@ use the Bourne shell command 'TERM=...; export TERM' (C-shell:\n\
>    tty->TS_enter_alt_charset_mode = tgetstr ("as", address);
>    tty->TS_exit_alt_charset_mode = tgetstr ("ae", address);
>    tty->TS_exit_attribute_mode = tgetstr ("me", address);
> +#ifdef TERMINFO
> +  tty->TS_enter_strike_through_mode = tigetstr ("smxx");
> +#else
>    tty->TS_enter_strike_through_mode = tgetstr ("smxx", address);
> +#end

There's something I don't understand here: you are talking about
underline support, but the proposed patch affects the support for
strike-through, not underline.  What am I missing here?

bug#62265: Underline does not work in Terminal Emacs

[Mohsin Kaleem]

Eli Zaretskii <eliz@gnu.org> writes:

> There's something I don't understand here: you are talking about
> underline support, but the proposed patch affects the support for
> strike-through, not underline.  What am I missing here?

Hi, yes, I sent a follow up message clarifying my mistake. Just
re-sharing it again:

S.N. Sorry, smxx is the code for strike-through. The error description
should be strikethrough doesn't work in terminal emacs. Not underline.

-- 
Mohsin Kaleem

bug#62265: Underline does not work in Terminal Emacs

[Jim Porter]

On 3/18/2023 10:55 AM, Eli Zaretskii wrote:
> Thanks.  Can someone with access to such a terminal please verify the
> issue with ncurses, and also verify the proposed change?

I'm not sure about the issue with ncurses or this specific terminal, but 
the patch does seem to make strike-through work on the Gnome Terminal. 
Previously, I wasn't able to get strike-through to work, but with this 
patch, it works correctly in Gnome Terminal.

(One easy way to see what happens is to use Org mode and type "+text+" 
into the buffer. That's Org markup for strike-through, and you should 
get a line through "text".)

bug#62265: Underline does not work in Terminal Emacs

[Mohsin Kaleem]

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

Jim Porter <jporterbugs@gmail.com> writes:

Hi, so turns out terminfo returns a max-ptr for tigetstr when the
terminfo definition is missing. There's checks for this for setb24 and
setf24 but not in the patch I supplied. Updated to check this and added
a macro definition to avoid repeating the same condition logic 3 times.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 01-fix-terminfo-strikethrough.patch --]
[-- Type: text/x-patch, Size: 1531 bytes --]

diff --git a/src/term.c b/src/term.c
index d881dee39fe..c47cdc8bb8a 100644
--- a/src/term.c
+++ b/src/term.c
@@ -95,6 +95,8 @@ #define OUTPUT_IF(tty, a)                                               \
 
 #define OUTPUT1_IF(tty, a) do { if (a) emacs_tputs ((tty), a, 1, cmputc); } while (0)
 
+#define TERMINFO_NON_STRING_CAP_P(cap) ((cap) == (char *) (intptr_t) -1)
+
 /* Display space properties.  */
 
 /* Chain of all tty device parameters.  */
@@ -4163,7 +4165,14 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
   tty->TS_enter_alt_charset_mode = tgetstr ("as", address);
   tty->TS_exit_alt_charset_mode = tgetstr ("ae", address);
   tty->TS_exit_attribute_mode = tgetstr ("me", address);
+#ifdef TERMINFO
+  tty->TS_enter_strike_through_mode = tigetstr ("smxx");
+  if (TERMINFO_NON_STRING_CAP_P (tty->TS_enter_strike_through_mode)) {
+    tty->TS_enter_strike_through_mode = NULL;
+  }
+#else
   tty->TS_enter_strike_through_mode = tgetstr ("smxx", address);
+#endif
 
   MultiUp (tty) = tgetstr ("UP", address);
   MultiDown (tty) = tgetstr ("DO", address);
@@ -4193,8 +4202,8 @@ init_tty (const char *name, const char *terminal_type, bool must_succeed)
 	const char *bg = tigetstr ("setb24");
 	/* Non-standard support for 24-bit colors. */
 	if (fg && bg
-	    && fg != (char *) (intptr_t) -1
-	    && bg != (char *) (intptr_t) -1)
+	    && !TERMINFO_NON_STRING_CAP_P (fg)
+	    && !TERMINFO_NON_STRING_CAP_P (bg))
 	  {
 	    tty->TS_set_foreground = fg;
 	    tty->TS_set_background = bg;

[-- Attachment #3: Type: text/plain, Size: 19 bytes --]


-- 
Mohsin Kaleem

bug#62265: Underline does not work in Terminal Emacs

[Eli Zaretskii]

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Cc: 62265@debbugs.gnu.org
> Date: Sun, 19 Mar 2023 10:07:14 +0000
> 
> Hi, so turns out terminfo returns a max-ptr for tigetstr when the
> terminfo definition is missing. There's checks for this for setb24 and
> setf24 but not in the patch I supplied. Updated to check this and added
> a macro definition to avoid repeating the same condition logic 3 times.

Thanks.

However, what about the non-TERMINFO branch?  Do termcap databases
support this capability and tigetstr?  I wonder whether we should do
one of the following:

  . support "smxx" only when TERMINFO is defined
  . support "smxx" regardless of whether TERMINFO is defined

With your patch, it's neither here nor there.  tgetstr is documented
to pay attention only to the first 2 characters of the capability's
name, at least in the ncurses documentation.  If this is generally so
in other curses libraries, then leaving the tgetstr call intact in the
non-TERMINFO case makes no sense.

bug#62265: Underline does not work in Terminal Emacs

[Mohsin Kaleem]

Eli Zaretskii <eliz@gnu.org> writes:

> However, what about the non-TERMINFO branch?  Do termcap databases
> support this capability and tigetstr?  I wonder whether we should do
> one of the following:
>
>   . support "smxx" only when TERMINFO is defined
>   . support "smxx" regardless of whether TERMINFO is defined

The latter wouldn't be possible for those using terminfo because of the
issue I described before. I'm okay with the former approach but I
imagine the author of the original TTY strikethrough patch was building
Emacs without terminfo and they described the patch as working for them
so I'd have to conclude termcap does support this (in a non-compliant
ncurses way). Switching to the former approach might break their
workflow since if they build without terminfo they'd lose strikethrough
altogether. I'm happy to test whether this would be the case but not
sure how to. The difference between termcap and terminfo seem kinda
arbitrary to me and I can't find any documentation describing the exact
difference (except this sort of 2 letter restriction in termcap
extensions).

-- 
Mohsin Kaleem

bug#62265: Underline does not work in Terminal Emacs

[Eli Zaretskii]

> From: Mohsin Kaleem <mohkale@kisara.moe>
> Cc: jporterbugs@gmail.com, 62265@debbugs.gnu.org
> Date: Sun, 19 Mar 2023 11:51:45 +0000
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > However, what about the non-TERMINFO branch?  Do termcap databases
> > support this capability and tigetstr?  I wonder whether we should do
> > one of the following:
> >
> >   . support "smxx" only when TERMINFO is defined
> >   . support "smxx" regardless of whether TERMINFO is defined
> 
> The latter wouldn't be possible for those using terminfo because of the
> issue I described before. I'm okay with the former approach but I
> imagine the author of the original TTY strikethrough patch was building
> Emacs without terminfo and they described the patch as working for them
> so I'd have to conclude termcap does support this (in a non-compliant
> ncurses way). Switching to the former approach might break their
> workflow since if they build without terminfo they'd lose strikethrough
> altogether. I'm happy to test whether this would be the case but not
> sure how to. The difference between termcap and terminfo seem kinda
> arbitrary to me and I can't find any documentation describing the exact
> difference (except this sort of 2 letter restriction in termcap
> extensions).

If your hypothesis is correct, I'm fine with leaving the non-TERMINFO
branch using tgetstr.  But is it indeed correct?

Let's ask the author of that strikethrough patch.  Mike, can you tell
whether you tested the patch on a system with or without terminfo?
And what, if anything, can you tell about using tgetstr for
capabilities whose names are more than 2 characters -- is that
supported with the curses library you were using at the time?

Thanks.

bug#62265: Underline does not work in Terminal Emacs

[Eli Zaretskii]

> Cc: jporterbugs@gmail.com, 62265@debbugs.gnu.org
> Date: Sun, 19 Mar 2023 14:24:19 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: Mohsin Kaleem <mohkale@kisara.moe>
> > Cc: jporterbugs@gmail.com, 62265@debbugs.gnu.org
> > Date: Sun, 19 Mar 2023 11:51:45 +0000
> > 
> > Eli Zaretskii <eliz@gnu.org> writes:
> > 
> > > However, what about the non-TERMINFO branch?  Do termcap databases
> > > support this capability and tigetstr?  I wonder whether we should do
> > > one of the following:
> > >
> > >   . support "smxx" only when TERMINFO is defined
> > >   . support "smxx" regardless of whether TERMINFO is defined
> > 
> > The latter wouldn't be possible for those using terminfo because of the
> > issue I described before. I'm okay with the former approach but I
> > imagine the author of the original TTY strikethrough patch was building
> > Emacs without terminfo and they described the patch as working for them
> > so I'd have to conclude termcap does support this (in a non-compliant
> > ncurses way). Switching to the former approach might break their
> > workflow since if they build without terminfo they'd lose strikethrough
> > altogether. I'm happy to test whether this would be the case but not
> > sure how to. The difference between termcap and terminfo seem kinda
> > arbitrary to me and I can't find any documentation describing the exact
> > difference (except this sort of 2 letter restriction in termcap
> > extensions).
> 
> If your hypothesis is correct, I'm fine with leaving the non-TERMINFO
> branch using tgetstr.  But is it indeed correct?
> 
> Let's ask the author of that strikethrough patch.  Mike, can you tell
> whether you tested the patch on a system with or without terminfo?
> And what, if anything, can you tell about using tgetstr for
> capabilities whose names are more than 2 characters -- is that
> supported with the curses library you were using at the time?

No further comments, so I've installed a fix for this along the lines
we discussed, and I'm closing this bug.