textual icon indicating copy to clipboard operation
textual copied to clipboard

Fix Alt key combinations and more

Open rndusr opened this issue 1 year ago • 15 comments

  • [ ] Docstrings on all new or modified functions / classes
  • [ ] Updated documentation
  • [ ] Updated CHANGELOG.md (where appropriate)

This allows you to use key combinations like alt+x, alt+ctrl+d, alt+enter, etc.

The environment variable ESCDELAY specifies the number of milliseconds to wait for more characters if an escape sequences is incomplete. ncurses uses the same variable for the same purpose.

ANSI_SEQUENCES_KEYS are customized for different terminals based on the TERM environment variable. This should fix various incompatiblities between terminals.

I've also included #3406 because it conflicts with the commits in this PR. It makes _character_to_key() return the original character if unicodedata.name() fails to find a key name instead of raising an exception. This happens for some control characters, for example, with alt+tab in xterm with metaSendsEscape: false, which sends U+89 CHARACTER TABULATION WITH JUSTIFICATION.

rndusr avatar Oct 20 '23 15:10 rndusr

I forgot: The environment variable TEXTUAL_DEBUG can now be set to a file path where debug_log() messages will be logged. It can also be set to "logging" to use a logging logger. TEXTUAL_DEBUG=1 still works, but I've changed the file name to textual_debug.log, which make it more obvious where this file is coming from.

rndusr avatar Oct 20 '23 16:10 rndusr

Hey @rndusr, thanks for your PR, we appreciate the time you took to work on this!

If you take a look at #3440, you'll find that @davep is currently doing some important and long overdue work that is highly likely to overlap and clash with what you've done here. I don't expect us to be able to look at your PR before that work is concluded, so we appreciate your patience!

Additionally, is there an issue or a (Discord/GitHub) discussion that tracks what your PR proposes to fix/change/add? If there is, would you mind linking it here? That will make it so much easier for us to understand and review your work.

rodrigogiraoserrao avatar Oct 23 '23 11:10 rodrigogiraoserrao

Hey, thanks for getting back to me.

I haven't opened an issue about this, only the discussion I linked above. I could swear I saw an issue about the bug where pressing any Alt key combination (e.g. Alt+x) would ignore any further keypress events until Escape is pressed (which this PR also fixes), but I couldn't find it.

But I'll be happy to answer any questions. The tests are quite thorough and should also answer some questions.

rndusr avatar Oct 23 '23 13:10 rndusr

Just to update: there's a few other things going on that are keeping us busy at the moment, and we'd like to give this PR a proper review as it does look interesting. Meanwhile though, as I scan over it, there seems to be a few different things in here, with some sequences being removed, etc, but which isn't clearly detailed in the description.

Do you think you could take a wee bit of time to write a description that walks us through the changes made, and the decisions behind them?

davep avatar Oct 30 '23 10:10 davep

Sure. First of all, I just saw that I misremembered linking to the relevant discussion: https://github.com/Textualize/textual/discussions/3327 Sorry about that.

I know this PR does multiple things, I don't like it either, but to get the keybindings to work they were either necessary (e.g. some of my changes to _ansi_sequences.py) or very helpful (e.g. the new debugging features that allowed me to catch debug_log() messages when running tests).

The changes I made to _ansi_sequences.py are a bit messy and redundant, but I left them like this to document my thought process. This may have been a bad idea. I can rebase them if you want.

I've added message bodies to the two commits that didn't have them. But the changes to XTermParser are extensive and I don't think I can (or should) explain everything. The other commit messages should already explain what they do.

I'd be happy to explain more and answer specific questions.

I can also add doc strings, maybe update the FAQ (https://textual.textualize.io/FAQ/#why-do-some-key-combinations-never-make-it-to-my-app) and stuff like that if you want. I just didn't want to do all that not knowing if this PR gets approved.

rndusr avatar Oct 30 '23 12:10 rndusr

Giving this a very quick spin with tkrec (macOS, kitty), and something I noticed is that in main Textual, ctrl++home results in \x1b[1;13H, whereas with this PR it just results in H. I'm seeing a similar effect for the same combination with some other navigation keys. Is this expected behaviour?

davep avatar Oct 30 '23 13:10 davep

No, that's definitely not intentional. But I don't know of any definitive way to look up what sequence a specific terminal on key events, so things like that will have to be fixed slowly when someone notices it.

Is ⌘ the macOS name for Alt/Meta?

Does this it work with textual keys?

On my Linux system, I get \x1b[1;7H for Alt+Ctrl+Home.

rndusr avatar Oct 30 '23 14:10 rndusr

Is ⌘ the macOS name for Alt/Meta?

Normally called "command" or "cmd", but often mapped to alt or meta.

Does this it work with textual keys?

It's similar in textual keys. On main, pressing esc afterwards to flush the buffer:

Screenshot 2023-10-30 at 14 56 21

which is what I'd expect given what I saw in tkrec.

davep avatar Oct 30 '23 14:10 davep

Is ⌘ the macOS name for Alt/Meta?

Normally called "command" or "cmd", but often mapped to alt or meta.

What if it isn't mapped to alt or meta?

Why alt OR meta? Does/Should textual distinguish between those two? I thought the keyboards with different alt and meta keys are all in museums.

Does this it work with textual keys?

It's similar in textual keys. On main, pressing esc afterwards to flush the buffer:

No, I meant in the feature/alt-keybindings branch. I haven't tried tkrec yet and don't know if it produces different key events.

Regarding the "H" for the Cltr+Alt+Home sequence: This is most likely because my PR emits one key event for each individual character if an escape sequence is not recognized and not private. The original implementation did this for all unknown sequences.

But now I think this is all wrong and unknown sequences should simply be ignored. For example, assuming textual didn't support F1 (\x1OP) and the user pressed F1, the application would behave like the user pressed Escape, O and P, which would be confusing to the user (best case scenario) or call functions the user didn't want to call, which might have all kinds of dramatic effects.

What do you think?

rndusr avatar Oct 30 '23 16:10 rndusr

Windows terminal doesn't, but other terminals on Windows might.

As long as they report to behave like xterm, we don't have to care about that.

Are there any other environment variables we can use to detect Windows Terminal?

rndusr avatar Oct 30 '23 16:10 rndusr

What if it isn't mapped to alt or meta?

Why alt OR meta? Does/Should textual distinguish between those two? I thought the keyboards with different alt and meta keys are all in museums.

I think I'm failing to follow the questions and observations here; perhaps you could address this more generally in the description of what the PR seeks to achieve?

No, I meant in the feature/alt-keybindings branch. I haven't tried tkrec yet and don't know if it produces different key events.

I didn't test textual keys with this PR yet; as I say it might take a wee while before we get to properly dive into the changes made here; perhaps you could evaluate this and add what you find so it makes it easier for us to review the intent and impact of the PR?

But now I think this is all wrong and unknown sequences should simply be ignored. For example, assuming textual didn't support F1 (\x1OP) and the user pressed F1, the application would behave like the user pressed Escape, O and P, which would be confusing to the user (best case scenario) or call functions the user didn't want to call, which might have all kinds of dramatic effects.

Currently that is what happens, although I think it's fair to say that the final outcome for such a situation is still to be decided (I'd agree that having unintended inputs is a bad thing). You'll note that recently I added a very simple debug hook for capturing these to help discover and record unknown sequences.

davep avatar Oct 30 '23 18:10 davep

I think I'm failing to follow the questions and observations here; perhaps you could address this more generally in the description of what the PR seeks to achieve?

I'm trying to understand what the Command key on macOS does exactly. You said it is often mapped to Alt or Meta, but on modern keyboards Alt and Meta are different names for the same key.

But it doesn't really matter. If Command is just another name for Alt, things just work, and if it doesn't, I can't doing anything about it because I don't have macOS.

No, I meant in the feature/alt-keybindings branch. I haven't tried tkrec yet and don't know if it produces different key events.

I didn't test textual keys with this PR yet; as I say it might take a wee while before we get to properly dive into the changes made here; perhaps you could evaluate this and add what you find so it makes it easier for us to review the intent and impact of the PR?

I can't evaluate this because I can't reproduce the behaviour you reported. I was trying to find out how to reproduce your Alt+Ctrl+Home issue.

rndusr avatar Oct 30 '23 19:10 rndusr

What's the current status here? #3440 is merged. How can I help?

I just tested with 0.46 and any Alt key combinations still break the whole application until the user presses Escape. Why is it not a higher priority to fix this?

I dont want to sound like a customer demanding service and I'm sure the number of other pressing issues is still higher than my worst guess. But TUIs are usually keyboard-driven, so I genuinely don't understand how this is acceptable, even for a beta release.

rndusr avatar Dec 18 '23 10:12 rndusr

@rndusr There is ongoing work on the keys at the moment. Half of the team are on a end of year break at the moment, so we will pick it up in January.

willmcgugan avatar Dec 18 '23 10:12 willmcgugan

OK. Thank you for the quick reply. Looking forward to getting this merged.

rndusr avatar Dec 18 '23 11:12 rndusr

So January has gone by. Any news?

plotski avatar Mar 18 '24 16:03 plotski

Sorry for the delay, but I'm afraid we can't accept this. Appreciate the effort you have gone to, but there is too much going on here. There are some quite extensive changes and its not clear what the impact will be.

What I would suggest is that you enumerate the various problems being solved here, and open up Issues to help us understand the requirements and potential solution. Then we can tackle each one independently.

willmcgugan avatar Mar 19 '24 14:03 willmcgugan