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

if the control modifier is pressed, don't insert the character #252

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

waf
Copy link
Owner

@waf waf commented Mar 28, 2023

Currently, if the user sets up a keybinding like Ctrl-Alt-Space, a literal space will be typed into the prompt before firing the keybinding.

The fix is to not insert the character if Control is pressed. We still insert if Shift or Alt is pressed as these can result in valid letters.

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #252 (e5995af) into main (2ccf136) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@          Coverage Diff          @@
##            main    #252   +/-   ##
=====================================
  Coverage   85.1%   85.1%           
=====================================
  Files         45      45           
  Lines       3851    3851           
  Branches     594     594           
=====================================
+ Hits        3278    3279    +1     
+ Misses       435     433    -2     
- Partials     138     139    +1     
Impacted Files Coverage Δ
src/PrettyPrompt/Panes/CodePane.cs 98.6% <100.0%> (+0.6%) ⬆️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@waf waf merged commit e996326 into main Mar 28, 2023
@waf waf deleted the fix-bug-in-control-keybindings branch March 28, 2023 14:13
@waf
Copy link
Owner Author

waf commented Jun 24, 2023

This caused a regression. On non-qwerty layouts (like AZERTY, and probably others), when the user tries to type a curly brace, which on that keyboard layout is accomplished by typing ALT-4 (AltGr 4), Console.ReadKey receives that as CTRL-ALT-4. So the logic in the PR description does not hold -- assuming CTRL is safe to filter is incorrect.

waf added a commit that referenced this pull request Jun 24, 2023
Previously, PR #252 was attempting to fix the scenario where users can
have a Control-Alt-Space keybinding; the keybinding would be fired but
the space key would be erroneously typed.

However, that PR broke AltGr handling. It assumed that filtering Ctrl
to detect keybindings would be OK, and Alt keys would be unaffected.
This is not correct, though, because AltGr is represented via Ctrl-Alt,
so it actually broke every AltGr based character, like curly-brace on
AZERTY keyboards.

This commit undoes the original fix for PR #252 and fixes it in a better
way. Instead of assuming Ctrl is a keybinding, we use the actual
configured keybindings in the filter, rather than assuming Ctrl
represent a keybinding.
@waf waf mentioned this pull request Jun 24, 2023
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.

None yet

1 participant