Fixing escape key getting filtered on Windows

[Neko-Box-Coder]

This should fix #2947

I will put a comment once I have tested this on a Windows and Linux machine.

[Neko-Box-Coder]

Just tested both on Windows and Linux. It indeed fixes the problem for Windows and doesn't affect Linux.
Someone else can test this if needed. Otherwise I think we can merge this small fix @dmaluka @JoeKar

[Neko-Box-Coder]

I have changed the test to simulate windows behavior to get it pass. I have searched the codebase on how we are handling escape for a few hours but just left me scratching my head to be honest.

I don't understand why it breaks for Linux when rune is set to 27 (that special case) because it is just additional information, we still have the key code set to escape anyway.

And somehow Windows is the exact opposite which requires that extra rune.

I guess it is because we are doing exact match so that additional rune information causes it to not match? I have no idea.

I am open to any suggestions, I have tested it both on Linux and Windows so it is working.

[JoeKar]

This is nothing we should handle within micro (at least not here with a GOOS check), because it's exactly what tcell is used for...it abstracts the terminal/-input.

From my perspective the correct way would be to handle it consistent within tcell, e.g. by...
https://github.com/zyedidia/tcell/blob/450b22f579b33147c4013c18db0fb33391d6e01d/console_win.go#L644-L650

...setting the NewEventKey(KeyEscape, 0, ModNone, "") in case krec.ch == vkEscape like here...
https://github.com/zyedidia/tcell/blob/450b22f579b33147c4013c18db0fb33391d6e01d/tscreen.go#L1574
...which works too.

@gdamore:
Hopefully it's ok to add you once more here...
Am I right that the upstream ESC key handling is still slightly different between Linux and Windows?
At least I can't find a big difference between the fork and your main regarding this.

[Neko-Box-Coder]

I didn't know we are using zyedida's tcell until you mentioned it. Why are we not using upstream's tcell?

Not having escape working out of the box is quite a big turn off I think and for me this is a major issue (For example cannot abort a command or have to choose between overwrite or discard what I have when the file gets updated by something else).

I would like to at least get this sorted when we release v2.0.14, given that we have (seem to be) 1 item left before releasing.

I don't mind creating a PR for the tcell we are using as long as it can get merged quickly by zyedidia, though I don't know how likely this will be.

I agree this should be handled by tcell and should not have a special case.

But if there are no other options that can resolve this before the release, would this workaround be good enough for now and we can create an issue(s) to resolve this properly?

[JoeKar]

I fully agree, that the current ESC behavior within the windows builds is fully broken. It doesn't work at (most probably) every place where it should. Especially in the moments a prompt is active, which gives the advice to press esc. I tested it yesterday by my own and was quite surprised that it's broken over such a long time span.

You provided a workaround...which works...but the problem with such "fixes" is that they sometimes stay longer as they should especially when the generalization is already spotted at a different location (at least from my perspective).

As you discovered the workaround needs an adaption within the tests too, which isn't that nice.

For me the most important question is:
Should tcell treat these key codes (!= KeyRune) in general with or without the related rune/character?

@dmaluka:
What's your opinion about the workaround provided with this PR?

[dmaluka]

I would like to at least get this sorted when we release v2.0.14, given that we have (seem to be) 1 item left before releasing.

Don't worry, 1 item left doesn't mean that v2.0.14 release is gonna happen soon. We don't know when will it happen.

[gdamore]

So I'm a little confused here. There shouldn't really be a difference between Linux and Windows, except that in the case of Linux we have to scan for a bit because it might be a lone escape key, or it might be part of an escape sequence. (There is a plan to use some new style reporting that doesn't suffer this ambiguity, but I just haven't had time to do the work.)

Certainly, applications should not have to have different handling for this key.

[gdamore]

In fact, looking at this block makes me very sad.

I understand why it exists... because Control key handling is so different between Windows and legacy TTY based terminals. I intend to fix all that, but the change will probably be breaking for some applications.

[gdamore]

In fact, this whole block seems misguided if I understand it correctly. Codes < 32 (the space character) should not be considered as possible control sequences (mixed with the control key) -- they are in fact sometimes control keys (e.g. Control-I is also the TAB sequence). This is the problem with these sequences ... on UNIX ttys you cannot easily discriminate between someone pressing Control-I (for example) or hitting TAB.

Modern terminals do have a reporting scheme that fixes this, but it is incompatible with legacy apps, and tcell has not been updated for the new style reporting.

I apologize for this mess -- had I had a clearer picture about this years ago, I would not have conflated these.

I will disentangle them, but not today.

Having said that, code 27 is ESCAPE, and corresponds to Control-] -- you should not allow anyone to bind a key to that. It will break lots of stuff. :-)

Note that some key bindings, such as Control-J -- will be impossible to make work portably because of the legacy conflation (in that case it's a new line). Again I hope to fix that, but it will only be supported in "new" terminals that support the new reporting. (This conflation does not exist for Windows.)

[dmaluka]

I think it is misleading that we are looking specifically at this code != 27 check here, in this Ctrl- handling block, not at the other code != 27 below, at line 198. Our issue is not about Ctrl- keys. I believe this code != 27 check here is not really needed and can be safely removed.

The issue is about the other code != 27 below at line 198. If we remove that check, the Esc key stops working in micro on Unix.

And this is why it stops working: https://github.com/gdamore/tcell/blob/88b9c25c3c5ee48b611dfeca9a2e9cf07812c35e/tscreen.go#L1784

tcell sends lone Esc key as an EventKey with the rune value set to 0.

Whereas for other key codes below 32, tcell sends an EventKey with the rune value same as the key code: https://github.com/gdamore/tcell/blob/88b9c25c3c5ee48b611dfeca9a2e9cf07812c35e/tscreen.go#L1633

This is why micro has to treat Esc as a special case even regardless of multi-byte escape sequences.

And this is where is the difference in tcell behavior between Unix and Windows. I have no Windows machine to verify that, but it looks like on Windows tcell has no specical case for the Esc key, it is sent with rune == keycode like other keys: https://github.com/gdamore/tcell/blob/88b9c25c3c5ee48b611dfeca9a2e9cf07812c35e/console_win.go#L794

Now, I can think of various ways to fix it:

Fix it in tcell:

• send Esc in Unix with rune == keycode, like in Windows
• or send Esc in Windows with rune == 0, like in Unix
• maybe the cleanest way is just to send all non-KeyRune key events with rune == 0, in both Unix and Windows? (what's the meaning of this rune value for non-rune keys anyway?)

Or fix it in micro (if, for example, changing tcell behavior is undesirable due to bug-to-bug compatibility for existing apps):

• Just use the workaround implemented in this PR.
• Or ignore tcell's rune field for non-KeyRune key events.

[dmaluka]

Ok... tcell documentation seems to make it perfectly clear what should we do:

// Rune returns the rune corresponding to the key press, if it makes sense.
// The result is only defined if the value of Key() is KeyRune.
func (ev *EventKey) Rune() rune {

Quickly baked PR, seems to work correctly on Linux, not tested on Windows: #3337

[JoeKar]

ditto

[Neko-Box-Coder]

@JoeKar
So currently the proposed solution is to create different event key depending on OS which we match later when user invokes any event.

Alternatively, I can try to handle the escape case when we process the event like what I proposed similarly in #3305 where I tried different combinations (i.e. both the escape keycode or the escape rune or both) unless we match something.

In which case we can avoid an OS check all together and I don't need to modify the test file.

However, this still doesn't change the fact that we are handling special cases and the more I think about it, the less I am sure if this is an upstream problem as tcell is just spitting out things on what it receives without modifying it (mostly)

I am happy to do whatever to get this merged quickly.
I do apologize to be frequently replying and raising issues/PRs as I am an impatient guy 😅

Would the solution I mentioned above be better or do we need more time for people to decide what to do with this?

[JoeKar]

So currently the proposed solution is to create different event key depending on OS which we match later when user invokes any event.

No, I proposed that micro should receive the same key under both OSs and that this should be abstracted by tcell.
tcell should generate it either with or without rune for both, but not different between those.
I already tried it without the rune in Windows, where it then worked without further modification of micro.

[gdamore]

If this is different between windows and everything else it's a bug in tcell. ESC should never be a rune. I will check as soon as I can. • Garrett

…

On May 24, 2024 at 10:57 AM -0700, Jöran Karl ***@***.***>, wrote: > So currently the proposed solution is to create different event key depending on OS which we match later when user invokes any event. No, I proposed that micro should receive the same key under both OSs and that this should be abstracted by tcell. tcell should generate it either with or without rune for both, but not different between those. I already tried it without the rune in Windows, where it then worked without further modification of micro. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

[Neko-Box-Coder]

@JoeKar
Yep cool. I can apply what you proposed to tcell to fix this problem. But we will need to update tcell. I don't think I can get my PR merged quickly if I want to update the one we are using, unless @zyedidia is available to do so.

Otherwise should I try to get micro working with upstream tcell or do we want to fork our current tcell and apply the changes?

[JoeKar]

@JoeKar Yep cool. I can apply what you proposed to tcell to fix this problem.

You can patch your local copy of tcell with win-esc.patch.txt and rebuild micro.
I was waiting for the confirmation/correction upstream, to directly propose the original backport.

But we will need to update tcell. I don't think I can get my PR merged quickly if I want to update the one we are using, unless @zyedidia is available to do so.

Correct, only he can do the merges in the currently used tcell.

Otherwise should I try to get micro working with upstream tcell or do we want to fork our current tcell and apply the changes?

No, the upstream version wouldn't work oob, since the fork was done to add some additional changes not present upstream (e.g. the raw escape sequences).
Zachary already created a much newer fork (see micro-editor/tcell vs zyedidia/tcell) within the micro-editor organization. Maybe it's common in the near future, that we switch to that. But even there we can't perform merges yet.

[JoeKar]

Done with #3337, since tcell's documentation wasn't considered correctly and undefined runes have been used, as found out by @dmaluka.