Ticket #4096: use set_escdelay interface instead of ESCDELAY var

[zyv]

Proposed changes

Refs: #4096

This doesn't solve the ESC / Meta confounding on ncurses, but it switches to a new interface and aligns the default with AIX (100 ms or 0.1 s).

[mc-worker]

Well, gh pr review 4798 --approve is not enough. My approval is not shown in the Reviewers list. I have to make it via web interface.

[zyv]

Well, gh pr review 4798 --approve is not enough. My approval is not shown in the Reviewers list. I have to make it via web interface.

Maybe you tried it with your other account that you use via SSH to push?

[ossilator]

the commit message could use some verbosity - reliance on external systems for full context is unhelpful.

the patch looks reasonable.

[ossilator]

"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.

[egmontkob]

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?

[zyv]

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?

[zyv]

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...

[egmontkob]

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? :)

[zyv]

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?

[egmontkob]

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