Skip to content
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.

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

Help (shift+?) keybinding broken on master #837

Closed
Tracked by #848
nickolay opened this issue Feb 25, 2023 · 5 comments · Fixed by #855
Closed
Tracked by #848

Help (shift+?) keybinding broken on master #837

nickolay opened this issue Feb 25, 2023 · 5 comments · Fixed by #855

Comments

@nickolay
Copy link
Contributor

After 2c3ae58 pressing Shift+/ (i.e. "?") to invoke help doesn't work for me on Windows (in cmd.exe).

In

if input_options.contains(InputOptions::HELP) && self.key_bindings.help.contains(&event) {
the incoming event has the SHIFT modifier enabled, while the configured self.key_bindings.help does not.

@MitMaro MitMaro added the bug label Mar 6, 2023
@MitMaro MitMaro self-assigned this Mar 6, 2023
@MitMaro
Copy link
Owner

MitMaro commented Mar 6, 2023

Thanks for the report! This one is surprisingly tricky...

Sadly, Windows treats "Shift" + "Key" differently than other systems. Assuming a US QWERTY layout, when you press "Shift" + "/" for a "?", a "Shift" + "?" is provided on Windows. On other systems, it is normalized to just "?".

I thought I had handled this inconsistency a few times now, by trying to normalize the input, but I keep breaking different use cases. My latest attempt to address this, was to remove the "Shift" modifier on all ASCII uppercase letters. However, "?" isn't an upper case letter, so obviously that doesn't work.

I think a decent solution it to:

  • expand the default keybindings, to include "shift" variants on Windows, this will address the default behaviour problems. This is tricky, since ideally the help wouldn't show these additional bindings.
  • expand the normalization to handle some common keys from the most common keyboard layouts. While most keyboard layout change the position of various punctuation, they tend to keep the keys very similar. This will reduce confusion if folks decide to use custom keybindings.
  • document that on Windows, if using a custom or lesser used keyboard layout, that some keybindings may require two bindings, one with shift and one without.

@nickolay
Copy link
Contributor Author

nickolay commented Mar 6, 2023

What's wrong with removing the SHIFT modifier from the incoming event before trying to find the matching keybinding? That's what you did before upgrading crossterm last year and it seemed to work well.

It looks like recognizing SHIFT modifier requires support from the terminal as well as the application, possibly not worth it for girt's use case?

@MitMaro
Copy link
Owner

MitMaro commented Mar 6, 2023

What's wrong with removing the SHIFT modifier from the incoming event before trying to find the matching keybinding? That's what you did before upgrading crossterm last year and it seemed to work well.

The only real issue is a theoretical keybinding, where Shift + Char is wanted. That was my reasoning behind limiting it to ASCII uppercase characters. It's partially due to me not having or using non-US QWERTY keyboards, and being unsure how they register the shift modifier. But as you stated, getting Shift to be recognized is difficult across platform. You are probably correct, in that it's not worth it. Let me think on it.

@MitMaro MitMaro mentioned this issue Jul 15, 2023
14 tasks
@MitMaro
Copy link
Owner

MitMaro commented Jul 16, 2023

Coming back to this issue, after a bit of a hiatus.

You are probably correct, in that it's not worth it. Let me think on it.

You are correct, it's not worth the effort. I want to do a further refactor of how keybindings are handled, since it is currently fairly fragile (mostly around using Strings for modifiers), which makes this kind of issue harder to fix.

However, I would like to get 2.3.0 released, and this is a blocker. To address the immediate issue, I am going to remove the Shift modifier from non-printable characters, and strip the shift modifier from any character events.

@nickolay
Copy link
Contributor Author

Seems to work fine for me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants