Skip to content

[KBM] Simultaneous shortcut invocation in Keyboard Manager#32125

Closed
gokcekantarci wants to merge 16 commits intomicrosoft:mainfrom
gokcekantarci:KBM-Simultaneously-Used-Shortcuts
Closed

[KBM] Simultaneous shortcut invocation in Keyboard Manager#32125
gokcekantarci wants to merge 16 commits intomicrosoft:mainfrom
gokcekantarci:KBM-Simultaneously-Used-Shortcuts

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:

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
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@gokcekantarci
Copy link
Contributor Author

/azp run

@azure-pipelines
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
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stefansjfw
Copy link
Contributor

/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).

@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
@github-actions

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.

@cinnamon-msft cinnamon-msft added the Priority-1 Bug that is high priority label Mar 14, 2025
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.

Gokce is no longer contributing to PowerToys. At the time of developing this code, it created unintended issues.
My review is that it added too much code and added special cases which made the code harder to maintain and created the additional issues. It seems to create many more paths in the code than should be needed to fix this issue. My advise here is to scratch this one and give it another try on a more recent state of the code.

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. Priority-1 Bug that is high priority

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

9 participants