search

jtdaugherty / brick

A declarative Unix terminal UI library written in Haskell
Other
1.6k stars 165 forks source link

Keybindings config character uppercase #512

Closed xsebek closed 3 months ago

xsebek commented 3 months ago

I accidentally wrote a keybinding in uppercase, like Ctrl-T, and it turns out this means it parses successfully, with no errors or warnings, but simply does not work --- both Ctrl-t and Ctrl-Shift-t do nothing. I suspect it turns into a keybinding which is impossible to ever see, because Ctrl-Shift-t is not the same as Ctrl-T. Well, I don't know, perhaps it is possible to get Ctrl-T by turning on caps lock and then typing Ctrl-t? In any case it seems like some kind of warning here would be nice.

@byorgey in https://github.com/swarm-game/swarm/pull/1979#issuecomment-2198366068

I am also confused by the behavior. Neither the Brick nor Vty documentation specifies what the case should be.

I suspect this code: https://github.com/jtdaugherty/brick/blob/79c9317b4388886ee45fa367ca18e800a9733386/src/Brick/Keybindings/Parse.hs#L126-L128 Should use the lowercase t instead of the original s text:

        pKey t
          | [c] <- T.unpack t = return (Vty.KChar c)

Either way, I tried configuring a keybinding with Ctrl and Shift and could not get it to run:

; these do not seem to work
pause = S-C-P,S-C-p,C-P,S-P,S-p
; without shift it works
pause = C-p
jtdaugherty commented 3 months ago

Thanks for this report!

It turns out that it appears impossible to get an event from the terminal with an uppercase character when modifiers are present, such as Ctrl-T. Shift is not generally even recognized as a discrete modifier, so I'm frankly not sure why that's even in vty. (It's probably been there since long before I took over maintenance.)

The parser should normalize the character to lowercase whenever modifiers are present. So the patch is a little different; the first pKey case needs to conditionally normalize the key character to lowercase only if mods is non-empty. That normalization step probably needs to happen in at least one other place, too. I'll work on a patch.

jtdaugherty commented 3 months ago

The patch is now on master - 6929129e9c856121bad66b08c6f8a8d71a06e1bf. Care to give it a try?

xsebek commented 3 months ago

@jtdaugherty works for me - the uppercase versions now conflict with the lowercase ones. 👍

I am surprised that shift is not recognized as a modifier. Maybe it is recognized only on some terminals/platforms? But that is a vty issue and it is not as important. 🙂