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

[KBM] Simultaneous shortcut invocation in Keyboard Manager #32125

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

gokcekantarci
Copy link
Contributor

@gokcekantarci gokcekantarci commented Mar 29, 2024

Summary of the Pull Request

Simultaneous shortcut invoke operations are allowed.

Note: This branch opened from #31923 to avoid code conflicts.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Some unit tests have been changed according to the changes made.

ShortcutDisable_ShouldNotDisableShortcutSuperset_AfterShortcutWasDisabled

  • When Ctrl + A invoked those keys are released for invoking shortcut (disabled). After new B key is pressed those are still released.
  • Checking Ctrl + B and Ctrl + A + B shortcuts. Those are not exist. So only B key is pressed.
  • isShortcutInvoked should be true because we are not resetting that. Because Ctrl, A keys are still pressed.

ShortcutDisable_ShouldResetIsOriginalActionKeyPressed_OnPressingAnotherKey

  • Function name and test condition are changed because isOriginalActionKeyPressed is not reset when a new key is pressed after shortcut is invoked.

Validation Steps Performed

The following 3 shortcuts were used while testing:

  • Win (Left) + E
  • Win (Left) + S
  • Win (Left) + V

While testing, all combinations of these shortcuts were tried:

  • Win (Left) + E + S
  • Win (Left) + E + V
  • Win (Left) + S + V
  • Win (Left) + S + E
  • Win (Left) + V + E
  • Win (Left) + V + S
  • Win (Left) + E + S + V, V + S + E, S + V + E ...

While testing, the keys were tested with different press, keep pressed and release states:

  • Win (Left) + E + Hold V pressed
  • Win (Left) + E, release and press V consecutive
  • Win (Left) + E, release E, press E, press V
  • Win (Left) + E, release E, release Win (Left), press Win (Left) + E + V
  • Win (Left) + E + Hold V pressed, release V and E, press V + Hold E pressed
  • Win (Left) + release and press(hold or consecutive) different combinations of E + S + V

It was tested by making the following assignments to the shortcuts:

  • Win (Left) + E -> Disable

  • Win (Left) + S -> Disable

  • Win (Left) + V -> Disable

  • Win (Left) + E -> A

  • Win (Left) + S -> B

  • Win (Left) + V -> C

  • Win (Left) + E -> Ctrl (Left) + A

  • Win (Left) + S -> Ctrl (Left) + V

  • Win (Left) + V -> Ctrl (Left) + C

  • Win (Left) + E -> Send Text (qwe)

  • Win (Left) + S -> Send Text (asd)

  • Win (Left) + V -> Send Text (zxc)

  • Win (Left) + E -> Run Program (calculator.exe)

  • Win (Left) + S -> Run Program (notepad++.exe)

  • Win (Left) + V -> Run Program (chrome.exe)

  • Win (Left) + E -> Open URI (https://www.google.com/)

  • Win (Left) + S -> Open URI (https://github.com/microsoft/PowerToys)

  • Win (Left) + V -> Open URI (https://learn.microsoft.com/en-us/windows/win32/inputdev/virtual-key-codes)

It is also tested with common modifier and action keys:

Set shortcuts as below and test shortcuts together

  • Ctrl (Left) + A -> Q

  • Shift (Left) + A -> W

  • Alt (Left) + A -> E

  • Ctrl (Left) + Shift (Left) + A -> T

  • Ctrl (Left) + Alt (Left) + A -> Y

  • Shift (Left) + Alt (Left) + A -> U

  • Ctrl (Left) + Shift (Left) + Alt (Left) + A -> J

Please test with Alt gr too.

  • Altgr (Ctrl Left + Alt Right) + E

  • Altgr (Ctrl Left + Alt Right) + S

  • Altgr (Ctrl Left + Alt Right) + V

  • Altgr (Ctrl Left + Alt Right) + A -> Q

  • Shift Right + A -> W

  • Altgr (Ctrl Left + Alt Right) + Shift Right + A -> E

@gokcekantarci gokcekantarci marked this pull request as draft April 5, 2024 13:15

This comment has been minimized.

This comment has been minimized.

@gokcekantarci
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gokcekantarci gokcekantarci marked this pull request as ready for review April 19, 2024 14:01
@stefansjfw
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stefansjfw
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@gokcekantarci
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Jay-o-Way
Copy link
Collaborator

this does actually fix #10393

#18459 (comment)

@bbartels
Copy link

@gokcekantarci @stefansjfw Was is blocking this from being merged? Anything I could help out with? Am dying to use this 😄

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

This comment has been minimized.

@andrew-landsverk-win
Copy link

Hello! What is the status on this PR? Really looking to use this feature :)

@tonytranwisetech
Copy link

Looking forward to this and #32534 to be merged.

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: No status
Development

Successfully merging this pull request may close these issues.

7 participants