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

fix control-character parsing in windows #629

Merged
merged 4 commits into from
Feb 27, 2022
Merged

fix control-character parsing in windows #629

merged 4 commits into from
Feb 27, 2022

Conversation

WindSoilder
Copy link
Contributor

Sorry for introducing a new bug in #619

The bug is: when user enters ctrl-g, the result will be G with modifier CONTROL. And we need to convert from upper case G to lowercase g.

This pr is trying to fix it.

@archseer
Copy link
Contributor

But does this fully fix the issue, what it ctrl shift g is pressed (ctrl-G)? Could some unit tests be added for the various cases?

@superlou
Copy link

superlou commented Feb 16, 2022

With the PR:

Event: Key(KeyEvent { code: Char('g'), modifiers: NONE })            <-- g
Event: Key(KeyEvent { code: Char('G'), modifiers: SHIFT })           <-- shift+g
Event: Key(KeyEvent { code: Char('g'), modifiers: CONTROL })         <-- ctrl+g
Event: Key(KeyEvent { code: Char('g'), modifiers: SHIFT | CONTROL }) <-- ctrl+shift+g

Since shift+G returns Char('G'), is ctrl+shift+G also supposed to return Char('G')?

@WindSoilder
Copy link
Contributor Author

WindSoilder commented Feb 16, 2022

Could some unit tests be added for the various cases?

I'll try to find where to add this, but it may takes some time.

Since shift+G returns Char('G'), is ctrl+shift+G also supposed to return Char('G')?

For this case, I've compared between this version and 0.22, and tried it on Unix, it seems that they all interpreted as Char('G'), when cap lock is on, shift+G returns Char('g'), I can't find an issue talking about this, so I supposed it's the expected behavior?

sholderbach added a commit to sholderbach/reedline that referenced this pull request Feb 26, 2022
crossterm 0.23 seems to have introduce a regression on windows for
decoding keybindings with `ctrl` that uppercases the char.

Tracking issue crossterm-rs/crossterm#636

Might be fixed by crossterm-rs/crossterm#629
sholderbach added a commit to sholderbach/reedline that referenced this pull request Feb 26, 2022
crossterm 0.23 seems to have introduce a regression on windows for
decoding keybindings with `ctrl` that uppercases the char.

Tracking issue crossterm-rs/crossterm#636

Might be fixed by crossterm-rs/crossterm#629
sholderbach added a commit to nushell/reedline that referenced this pull request Feb 26, 2022
crossterm 0.23 seems to have introduce a regression on windows for
decoding keybindings with `ctrl` that uppercases the char.

Tracking issue crossterm-rs/crossterm#636

Might be fixed by crossterm-rs/crossterm#629
@TimonPost
Copy link
Member

TimonPost commented Feb 26, 2022

Not sure what has desired behavior there. Seems like that if SHIFT is pressed with other control keys the character should be just capitalized right?

@superlou
Copy link

superlou commented Feb 26, 2022

On Linux Mint, I get:

Event: Key(KeyEvent { code: Char('g'), modifiers: NONE })            <-- g
Event: Key(KeyEvent { code: Char('G'), modifiers: SHIFT })           <-- shift+g
Event: Key(KeyEvent { code: Char('g'), modifiers: CONTROL })         <-- ctrl+g
                                                                     <-- ctrl+shift+g
Event: Key(KeyEvent { code: Char('G'), modifiers: SHIFT | ALT })     <-- alt+shift+g

Unfortunately, I don't get ctrl+shift+G, though something in the desktop environment might be catching it. However, alt+shift+g capitalizes the char code. However, the alt+shift+g case makes me exepct ctrl+shift+g should behave similarly. Is there a document or wikipage somewhere defining how crossterm's intent?

@sophiajt
Copy link
Contributor

Maybe it's just me, but I'd expect Char('g') for all cases, and the modifier SHIFT to tell you if it should be capital or not. This treats the keyboard more like the "raw mode" keyboard interface where we want to know what keys were pressed and then interpret those events later on.

@TimonPost
Copy link
Member

When pressing SHIFT an uppercase is printed in all applications, we decided in the past that crossterm should also do this. Similarly, with capslock. When capslock is enabled the user will receive SHIFT + capitalized char. However, I do get your point, more or less a preference thing.

@TimonPost
Copy link
Member

TimonPost commented Feb 26, 2022

On ubuntu, it also don't get SHIFT + CONTROL keys. Tho Unix terminals are more limited in their event support. Windows has a easier API for reading input.

src/event/sys/windows/parse.rs Outdated Show resolved Hide resolved
@TimonPost TimonPost merged commit e920d0c into crossterm-rs:master Feb 27, 2022
@TimonPost
Copy link
Member

Thanks.

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

Successfully merging this pull request may close these issues.

5 participants