Skip to content

[KBM] Fixed Ctrl key state handling during and after shortcuts invoked by AltGr#31923

Merged
stefansjfw merged 13 commits intomicrosoft:mainfrom
gokcekantarci:KBM]-Alt-Right-Shortcut-Reset
Aug 21, 2024
Merged

[KBM] Fixed Ctrl key state handling during and after shortcuts invoked by AltGr#31923
stefansjfw merged 13 commits intomicrosoft:mainfrom
gokcekantarci:KBM]-Alt-Right-Shortcut-Reset

Conversation

@gokcekantarci
Copy link
Contributor

@gokcekantarci gokcekantarci commented Mar 14, 2024

Summary of the Pull Request

Shortcuts involving the AltGr key does not behave as expected. The problem is that when AltGr(ctrl + alt) + Action Key is pressed, the ctrl release event immediately follows.

Also, in some cases, when the AltGr key is released, it behaves as if the ctrl key is still pressed.

PR Checklist

Detailed Description of the Pull Request / Additional comments

After pressing the Ctrl + AltGr + Action keys, it was observed that the Ctrl release event did not occur. In AltGr + Action key, the Ctrl release event comes first. To prevent this, a control was created with the "isAltRightKeyInvoked" boolean variable. The first Ctrl release event is skipped.

During the tests, it was seen that Ctrl was still pressed while AltGr was released. Added a check to prevent this. Example test case:
ctrl left + alt right + b -> ctrl left --- alt gr + b + a to select all then release all keys and try to press a key.

Validation Steps Performed

  • ctrl left + alt right + a -> b
  • ctrl left + alt right + backspace -> delete
  • ctrl left + b -> ctrl left --- ctrl left + b + a to select all
  • ctrl left + alt right + b -> ctrl left --- alt gr + b + a to select all
  • ctrl left + alt right + 9 -> shift left + '<' --- alt gr + 9 to '>'
  • KBM tests in release tests checked
  • Keyboard changed to US keyboard to test Alt Right only not AltGr(Ctrl left + Alt Right)

@gokcekantarci
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gokcekantarci
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@crutkas crutkas added the Needs-Review This Pull Request awaits the review of a maintainer. label Jun 19, 2024
Copy link
Contributor

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given it a test and ran into a weird bug where it seems Ctrl(Left) got stuck.

I did the following mapping, as per the example:
image

After that I wrote a bunch of stuff on notepad++. and tried the mapping:
1 - Press AltGr key.
2 - Press Backspace key.
3 - Release Backspace key.
4 - Press left arrow key.
5 - Release left arrow key.
6 - Release Alt Gr.

The left Ctrl key got stuck even though I'm not pressing anything. PTAL.

@gokcekantarci gokcekantarci mentioned this pull request Jul 28, 2024
11 tasks
isAltRightKeyInvoked = true;

// Check if the left control is pressed when right Alt key (AltGr) is pressed. If it is release it.
if ((data->wParam == WM_KEYUP || data->wParam == WM_SYSKEYUP) && ii.GetVirtualKeyState(VK_LCONTROL))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? This is not good behavior. If I hold down Left Ctrl and then press and release Right Alt, Left Ctrl is being released even though I'm still holding it down.

// Check if the right Alt key (AltGr) is pressed.
if (data->lParam->vkCode == VK_RMENU)
{
isAltRightKeyInvoked = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be both KEYUP or KEYDOWN. Should isAltRightKeyInvoked be set to true on KEYUP?


// Prevents the unintended release of the Ctrl part when AltGr is pressed. AltGr acts as both Ctrl and Alt being pressed.
// After triggering a shortcut involving AltGr, the system might attempt to release the Ctrl part. This code ensures Ctrl remains pressed, maintaining the AltGr state correctly.
if (isAltRightKeyInvoked && data->lParam->vkCode == VK_LCONTROL && it->first.GetActionKey() != VK_LCONTROL && (data->wParam == WM_KEYUP || data->wParam == WM_SYSKEYUP))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it->first.GetActionKey() != VK_LCONTROL action key can't be VK_LCONTROL if I'm not wrong?

bool isMatchOnChordEnd = false;
bool isMatchOnChordStart = false;

static bool isAltRightKeyInvoked = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why static?

@stefansjfw
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@stefansjfw stefansjfw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested different altgr and ctrl combinations. Looks good

@stefansjfw stefansjfw merged commit a163bbe into microsoft:main Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs-Review This Pull Request awaits the review of a maintainer.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants