Skip to content

Ticket #4096: use set_escdelay interface instead of ESCDELAY var #4798

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Ticket #4096: use set_escdelay interface instead of ESCDELAY var #4798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions lib/tty/tty-ncurses.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,17 +230,17 @@ tty_init (gboolean mouse_enable, gboolean is_xterm)

initscr ();

#ifdef HAVE_ESCDELAY
#ifdef HAVE_SET_ESCDELAY
/*
* If ncurses exports the ESCDELAY variable, it should be set to
* a low value, or you'll experience a delay in processing escape
* sequences that are recognized by mc (e.g. Esc-Esc). On the other
* hand, making ESCDELAY too small can result in some sequences
* (e.g. cursor arrows) being reported as separate keys under heavy
* processor load, and this can be a problem if mc hasn't learned
* them in the "Learn Keys" dialog. The value is in milliseconds.
* ESCDELAY variable should be set to a low value, or you'll experience a
Copy link
Contributor

@ossilator ossilator Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the ..."

but the whole comment is mildly confusing, because the actual ESCDELAY is an env variable, while we are now setting an internal variable, which would conventionally be lowercase. doesn't really matter, but maybe you have a great idea how to word it better.

* delay in processing escape sequences recognized by mc (e.g. Esc-Esc).
* Making ESCDELAY too small can result in some sequences (like cursor
* arrows) being reported as separate keys under heavy processor load,
* and this can be a problem if mc hasn't learned them in the "Learn Keys"
* dialog. The value is in milliseconds (AIX defaults to 0.1s, ncurses 1s).
*/
ESCDELAY = 200;
if (g_getenv ("ESCDELAY") == NULL)
Copy link
Contributor

@egmontkob egmontkob Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the design here. Why introduce a brand new (and undocumented) environment variable when old_esc_mode_timeout is already there in .config/mc/ini?

Copy link
Member Author

@zyv zyv Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I understand that there is a dire need for more explanations :(

According to my understanding, the story is as follows: ncurses has its own environment variable called ESCDELAY that is used to control the value of an internal variable ESCDELAY (in addition to our KEYBOARD_KEY_TIMEOUT_US).

In our code for ncurses, we "disable" their ESCDELAY by setting it to 200 ms (see the comment), apparently to avoid double delay. Since we want to change ESCDELAY programmatically, we need to check whether this static (not environment!) variable is exported and change it, overriding whatever the user did with the environment variable.

Some time later (from ncurses 5.7 on, if my understanding is correct), a new way was introduced to set this value programmatically: set_escdelay(). The old way with the variable still works, no idea if it will be supported indefinitely or not, but probably it will.

My thinking, however, was to use the new way, but override it only if the ESCDELAY environment variable is not already set by the user. If it is, then probably there is a reason for that. If there is no reason, it's the user’s fault anyways.

The users of old ncurses before 5.7 from circa 2008 would now have to set the ESCDELAY environment variable explicitly to avoid double delays. For the users of new ncurses, nothing changes per default, but they gain the ability to adjust ESCDELAY if desired using ncurses’ standard mechanism, and I get to remove a few lines of disgusting code.

My own explanation brings me to the next idea though: as mentioned in #4096, it seems that our KEYBOARD_KEY_TIMEOUT_US mechanism for ncurses (as opposed to S-Lang), also causes the delay when Meta/Alt is pressed...

What about the ncurses built-in ESCDELAY? Does it only apply to ESC or also to Alt/Meta? If it really only applies to ESC, then maybe we could disable our own old escape stuff for ncurses, and use ESCDELAY instead, thus fixing the discrepancy?

Is ESCDELAY a general "portable" curses or only ncurses thing? I actually feel immediate regret for opening this can of worms now :(

Can it be that our KEYBOARD_KEY_TIMEOUT_US emulation was added for S-Lang? Does anyone know the full story?

Copy link
Member Author

@zyv zyv Oct 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I set our KEYBOARD_KEY_TIMEOUT_US to 1 (effectively disabling it) and ESCDELAY to 1000 for the ncurses build, then Alt+h works immediately, but Esc waits for 1 second just like for the S-Lang build, leading to consistent behavior.

KEYBOARD_KEY_TIMEOUT_US=1 ESCDELAY=1000 ~/src/mc/master/build/install/bin/mc

Anyways, it seems one would need to understand what and why is done in get_key_code to conclude on how to fix #4096 correctly, and there is no way I can get to that anytime soon :(

I think my patch here is still an improvement and can be taken...

Copy link
Contributor

@egmontkob egmontkob Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have surely missed that ESCDELAY is not only a global variable in ncurses's code, but is also an environment variable it takes into account.

I'd argue that if old_esc_mode is enabled then the mc-specific old_esc_mode_timeout should override the cross-app env var ESCDELAY.

That is: if old_esc_mode is enabled then install our old_esc_mode_timeout, otherwise let ncurses do whatever it wants to do. I can't see a reason for yet another hardcoded value, whether that's 200 or 100, to be present – other than the said config option old_esc_mode_timeout having some default value.

That would be the simplest I could imagine. Let me guess: I've missed something and this approach doesn't handle something correctly, right? :)

Copy link
Member Author

@zyv zyv Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be the simplest I could imagine. Let me guess: I've missed something and this approach doesn't handle something correctly, right? :)

Yes, I think that you've missed a part of my explanation.

According to my understanding (of the original comment), if we set ESCDELAY to zero, "things will occasionally break", so it has to be set to a small non-zero value (e.g. 100 or 200 ms).

But I think you have a point that instead of calling set_escdelay unconditionally, it should be called only if old_esc_mode is active. However, this presents another challenge. This mode can be turned on and off when mc is running. Previously, ESCDELAY is disabled unconditionally at the screen library initialization stage, so that's not an issue. If it is done conditionally, then it should be done in the old_esc_mode toggle handler.

I do now have a deeper question though: can we just activate our own stuff (which obviously is not working fully correctly under ncurses in that not only Esc, but also Alt is delayed) for S-Lang only and for ncurses use its built-in ESCDELAY stuff. But that is a question for me that is too hard to be answered with the time I can allocate to it...

So the bottom line is, I guess I should just remove the if for this change?

Copy link
Contributor

@egmontkob egmontkob Oct 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation!

I have to admit, I don't care enough about this issue to really understand all the details and all the design decisions, so please don't block on me, feel free to go ahead with whatever you feel is the best :)

set_escdelay (100);
#endif

tcgetattr (STDIN_FILENO, &mode);
Expand Down
13 changes: 1 addition & 12 deletions m4.include/mc-with-screen-ncurses.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,18 +107,7 @@ AC_DEFUN([mc_WITH_NCURSES], [
AS_IF([test x"$ncurses_term_h_found" != xyes],
[AC_MSG_ERROR([NCurses(w) term.h header file not found])])

dnl If ncurses exports the ESCDELAY variable it should be set to 0
dnl or you'll have to press Esc three times to dismiss a dialog box.
AC_CACHE_CHECK([for ESCDELAY variable], [mc_cv_ncurses_escdelay],
[AC_LINK_IFELSE(
[AC_LANG_PROGRAM([[extern int ESCDELAY;]],[[ESCDELAY = 0;]])],
[mc_cv_ncurses_escdelay=yes], [mc_cv_ncurses_escdelay=no])
])

AS_IF([test x"$mc_cv_ncurses_escdelay" = xyes],
[AC_DEFINE([HAVE_ESCDELAY], [1], [Define if NCurses(w) has ESCDELAY variable])])

AC_CHECK_FUNCS(resizeterm)
AC_CHECK_FUNCS([resizeterm set_escdelay])

AC_DEFINE([HAVE_NCURSES], [1], [Define to use NCurses for screen management])
AC_DEFINE_UNQUOTED([NCURSES_LIB_DISPLAYNAME], ["$screen_msg"], [Define NCurses library display name])
Expand Down
Loading